Material-design-lite: Button + Ripple initialization race condition

Created on 21 Apr 2015 · 13Comments · Source: google/material-design-lite

I discovered a bug, but haven't been able to reduce this to a minimal test case. I'll dig into this again in the next days, but perhaps this is already helpful.

When dynamically adding an mdl-button with ripple effect, it is possible to end up in a state where the mdl-button__ripple-container and its mdl-ripple are added, but rippleElement_ inside MaterialRipple is uninitialized. When triggering a ripple, it causes an exception in this line:

MaterialRipple.prototype.upHandler_ = function(event) {
  'use strict';

  // Don't fire for the artificial "mouseup" generated by a double-click.
  if (event && event.detail !== 2) {
    // **Uncaught TypeError: Cannot read property 'classList' of null**
    this.rippleElement_.classList.remove(this.CssClasses_.IS_VISIBLE);
  }
};

(Source)

Most other places in the file guard against this.element_ being null, but adding a fix here would only mask the underlying problem that the initialization is incomplete. The DOM, however, looks like this:

<span class="mdl-button__ripple-container"><span class="mdl-ripple"></span></span>

I'll try to narrow this down further, I think there's a very small window when the component handler races against the initial render of my layout, which happens via virtual-dom materialization.

bug

Source

passy

All 13 comments

Thanks for the detailed report, @passy. cc @jasonmayes && @sgomes

addyosmani on 22 Apr 2015

Which component handler function are you calling after programatically creating the new button element to upgrade and when (eg before/after DOM insertion)? Do you have a link to this part of your code to view?

jasonmayes on 22 Apr 2015

@jasonmayes I don't call any upgrade logic manually, so it be the initial upgrade on page load that races against the rendering to the DOM. Don't spend too much time on this. I thought someone might immediately get an idea where this comes from, but I'll try to find a reduced test case for this.

passy on 22 Apr 2015

@jasonmayes I'm afraid I still haven't found a reliable and minimal case, but I've got a better idea of what's going on now.

The problem is the that the MaterialRipple initialization happens before MaterialButton. This means that when MaterialRipple assigns the ripple element with this.rippleElement_ = this.element_.querySelector('.' + this.CssClasses_.RIPPLE); it gets null. When MaterialButton takes its turn, it adds the mdl-ripple element, but as the previous initialization is already done it remains in a broken state.

I'm not quite sure why the initialization order is like this. Do you have any ideas what could be done to prevent this?

passy on 24 Apr 2015

I threw this together to try and test:

<script>
    buttonAddedToPage = false;
    document.addEventListener('click', function() {
        if(!buttonAddedToPage) {
            var button = document.createElement('button'),
                    buttonClasses = ['mdl-button', 'mdl-js-button', 'mdl-js-ripple-effect'];
            buttonClasses.forEach(function (item) {
                button.classList.add(item);
            });
            button.innerHTML = 'Ripple button test';
            document.querySelector('.mdl-grid').appendChild(button);
            componentHandler.upgradeAllRegistered();
            buttonAddedToPage = true;
        }
    });
</script>

Is there anything I'm missing as far as class-names or structure? I'm just injecting a button into the first grid and upgrading everything so it is triggered. Seems to work fine, but I could be missing a class or something.

Garbee on 24 Apr 2015

Urgh, I was thinking way too complicated. Sorry for taking all your time. I've been using the files directly without build-step and didn't include them in the right order. Because component initialization reflects the component upgrade later the Ripple upgrade happened before the Button upgrade causing the issue. So it's not actually a race condition, just a dependency mismatch.

In case you'd still like to see the error in action, this is all you need to reproduce it:

<!doctype html>
<meta charset=utf-8>
<title>ripple me</title>

<link rel=stylesheet href=css/material.css>

<button class="mdl-button mdl-js-button mdl-js-ripple-effect">Push</button>

<script src=src/mdlComponentHandler.js></script>
<script src=src/third_party/rAF.js></script>
<script src=src/ripple/ripple.js></script>
<script src=src/button/button.js></script>

Feel free to close this. I'm still not convinced it's great to depend on the behavior of unrelated components, but this was clearly a user error and this is a valid design choice.

passy on 24 Apr 2015

Don't worry about it @passy. This raises a valid concern for customization that I should test and document. We need to make sure people only pulling in certain bits pull some things in the right order.

Garbee on 24 Apr 2015

Spoke too soon... @passy Just hit the same error myself and I'm pulling everything in the same order as upstream. I should be able to reduce it down now.

Also, my button is not dynamic but on page load.

Garbee on 25 Apr 2015

Ok, I'm back to confused on this. I had the error happening. Removed the ripple-effect class to test, errors persisted, added it back, now it works with or without the ripple-effect class.

Garbee on 25 Apr 2015

@Garbee In my project I loaded the scripts with async which also made the problem even harder to debug as it adds non-determinism to the project.

passy on 25 Apr 2015

Still not able to recreate this. I had an odd moment where it was reproducing then updated and it hasn't happened since. Going to go ahead and close this out to keep the tracker clean. If it is still being hit then comment again and this can be re-opened.

Garbee on 25 May 2015