Materialize: getWavesEffectElement throws error with any svg left/right click - Need to do null checking

[hewstone]

Since you wire up a click event in the body
document.body.addEventListener('mousedown', showEffect, false);

you need to add some additional null checks to the getWavesEffectElement function. Otherwise using libraries like google charts will cause errors to be thrown because you all use the same css class names and they have structured their elements different than you structured yours.

Please see comments below to see where null checks need to be added

 getWavesEffectElement(e) {
        if (TouchHandler.allowEvent(e) === false) {
            return null;
        }

        var element = null;
        var target = e.target || e.srcElement;

//added target 
        while (target && target.parentElement !== null) {
//added target.className
            if (target.className && !(target instanceof SVGElement) && target.className.indexOf('waves-effect') !== -1) {
                element = target;
                break;
            } 
//added target.classList
          else if (target.classList && target.classList.contains('waves-effect')) {
                element = target;
                break;
            }
            target = target.parentElement;
        }

        return element;
    }

[johnny3310]

I think this issue is caused by the SVGElement.parentElement value is undefined in IE,
so if you click on a svg element, it will pass through the while condition checking and pass undefined
to target variable.
I managed to write a work around code that worked for me. hope this may help.

 if(isIeBrowser()){
    if(window.SVGElement && window.SVGElement.prototype ){
      if (window.SVGElement.prototype.parentElement === undefined) {
        window.SVGElement.prototype.parentElement = null;
      }
    }
  }

[shanehoban]

+1

This works for me. The while loop on line 2120; this is the old code:

        while (target.parentElement !== null) {
            if (!(target instanceof SVGElement) && target.className.indexOf('waves-effect') !== -1) {
                element = target;
                break;
            } else if (target.className.indexOf('waves-effect') !== -1) {
                element = target;
                break;
            }
            target = target.parentElement;
        }

Here is the code that stops throwing that error for me. Granted it may disable waves effects on SVG but that doesn't matter to me.

        while (target.parentElement !== null && !(target instanceof SVGElement)) {
            if (target.className.indexOf('waves-effect') !== -1) {
                element = target;
                break;
            } else if (target.className.indexOf('waves-effect') !== -1) {
                element = target;
                break;
            }
            target = target.parentElement;
        }

[jacobq]

This problem is nearly 2 years old and looks like it has an easy fix. Is there anything I can do to help it land in a release? Perhaps the slow-down is because it is an upstream bug?

[DanielRuf]

Here is the code that stops throwing that error for me. Granted it may disable waves effects on SVG but that doesn't matter to me.

@jacobq Not sure if we have any clean fix yet. Does this disable the effect on the material icons?

If we have a good and tested fiy and you want to contribute then you can create a PR with the needed changes.

[shanehoban]

Does this disable the effect on the material icons?

Not sure why anyone would want to do this but the waves effect still works after my above fix. Tested it simply with this:

<i class="large material-icons waves-effect">insert_chart</i>

I don't fully understand how the material icons work but I don't see how they would play a part in it. I don't see any SVG reference in the DOM when they are loaded.

[DanielRuf]

@shanehoban sorry, I meant the svg version of the Material icons. You can download single svg icons from https://material.io/icons/ and insert the svg markup instead of the text used by the font.

Can you please test if the method still works on svg images or if the change completely disables it on svg elements?

A codepen could be also useful.

[DanielRuf]

Granted it may disable waves effects on SVG but that doesn't matter to me.

Does or doesn't it disable it the effect?

[shanehoban]

Oh I see, well I guess the fix I'm using would disable the waves-effect on them. It will disable waves-effect on any SVG for that matter via && !(target instanceof SVGElement)

[DanielRuf]

Oh I see, well I guess the fix I'm using would disable the waves-effect on them. It will disable waves-effect on any SVG for that matter via && !(target instanceof SVGElement)

Hm, that is not really an option.

[jacobq]

I'll do some testing & experimenting tomorrow then comment here, but I think the problem that I have is actually resolved by the newer version of the upstream package already so Materialize would just need to bundle the newer version.

[DanielRuf]

It was fixed in https://github.com/fians/Waves/commit/19f4b31a55ea845ba7055ee149671c3bbfe86ed0 https://github.com/fians/Waves/issues/145

[DanielRuf]

I think we should diff our waves.js 0.6.4 file against the original one, rebase it, apply the newest release of it, test it and release it with the next Materialize version.

[jacobq]

@DanielRuf I tried just copying and pasting latest waves code into the materialize code (https://github.com/jacobq/materialize/tree/update-waves) but that seemed to break functionality (guessing either the API changed or the waves code in materialize is modified (hope not; that sounds like a maintenance headache)), so instead I changed one line to circumvent the problem I'm having. This shouldn't have any negative effect, so if you think it might be a while before the underlying code gets an update I'd appreciate some kind of stop-gap like this.
See https://github.com/jacobq/materialize/tree/svg-click-fix-hack

[DanielRuf]

@jacobq Is this another issue? I thought it is about the svg element code.

[jacobq]

@DanielRuf Well, my problem matches the description of this issue exactly. When one clicks on the chartist SVG element an error occurs on that line where I show the breakpoint & my modified code because the SVG element's className property is not guaranteed to be a string. Some people talk about it being related to IE, but for me I am using only Chrome/webkit/Electron, and it happens.

[DanielRuf]

@jacobq and this is not (a better) fix? https://github.com/Dogfalo/materialize/issues/2005#issuecomment-310894652

I am currently not understanding why we discuss different solutions for the (same) problem.

[shanehoban]

@DanielRuf I don't think mine is better as it avoids all clicking on SVG elements.

[DanielRuf]

@DanielRuf I don't think mine is better as it avoids all clicking on SVG elements.

So the version of @jacobq is better and still works? Unfortunately I do not have the time to test this. Could you tests if this would solve your issue and still work in other cases?

Would be interesting to know and see if and how it is fixes in waves.js.

[jacobq]

@DanielRuf the earlier code referenced does not resolve my problem because it still includes target.className.indexOf without first ensuring that target.className is a string (otherwise 'undefined is not a function' error can occur). Perhaps there are multiple problems here and the solutions are address different ones. For sake of clarity, here is a JSBin demonstrating my problem.

[shanehoban]

Line 2124 of materialize js, this seems to stop the error, and wouldn't have any affect on SVGs otherwise

} else if ((target.className).toString().indexOf('waves-effect') !== -1) {

[Dogfalo]

Fixed in 741d47a092ba4f09ce841c7fa98def1ce0dcfd2c

[DetlefPeters]

I support @shanehoban 's proposed fix, because I encouter the same problems as him and @DanielRuf.
This is NOT a IE11-specific problem and affects at least Chrome, too.
Applying the fix from 741d47a did not solve the problem, adding the toString() conversion does!.

[jacobq]

@DetlefPeters Could you share a live example (e.g. jsbin / jsfiddle / codepen / etc.)? The latest materialize code (869f10bb00659cb2021440d48df2515220096c3f) seems to have resolved the problem I saw: https://output.jsbin.com/nivapom

[DetlefPeters]

In that jsbin of yours, I found the entire else if() block deleted from getWavesEffectElement. That is an approach I haven't found in the discussion here, but it does solve my problem, too, since the erratic statement was in the deleted else if condition.
Can you already tell me when a fixed version will be available via bower?

[tomscholz]

@DetlefPeters The fix is already in master. We don't give ETAs.

[jacobq]

@DetlefPeters if you're referring to the else if that was in here then that was part of the 741d47... fix mentioned above. Perhaps you mistakenly pulled the old dist files instead of rebuilding with grunt release?

[DetlefPeters]

@tomscholz, @jacobq My company only uses bower install for 3rd party libraries, so it looks as if we have to wait for the next release.

[jacobq]

@DetlefPeters that sounds like it'll work. Alternatively, you can create a fork, generate the dist files, and install that via bower using URL instead of package name, e.g. bower install https://github.com/org/repo.git#master. P.S. although it's a separate discussion, you may want to consider starting to migrate away from bower. See https://www.quora.com/Is-Bower-dying

[onaforeignshore]

I am getting the same error, but this time on another line.