Upgrading Material Menu before added to DOM doesn't work

[timothylrobb]

I have found that when dynamically creating a Material Menu and upgrading it before adding it to the DOM fails due to line 1114 in material.js.

forEl = document.getElementById(forElId);

Because the element hasn't been added to the DOM, it cannot be retrieved this way. All other parts of the menu get upgraded, but because of this line, the button does not get the events required to trigger the menu to display.

I created my own MaterialMenu class based on this but instead of sending the upgradeElement function the <UL> element, I designed the class to accept a parent <div> container that contained the <UL> and the <BUTTON> elements. Then upgraded it like this:

componentHandler.upgradeElement(menuContainer, "MaterialMenuTim");

It works great and is able to get references both to the <UL> and the <BUTTON> so that everything can get upgraded.

Please consider changing the class so that it can receive both elements and upgrade without requiring the element being added to the DOM during the creation process. This allows devs to fully create a view dynamically before displaying it.

[surma]

@timothylrobb Would you mind opening a PR so we can look at the code?

[Garbee]

The issue is on line 117 of the menu JS.

Nothing we can do about this in 1.x though as changing how the markup is structured and where the classes need to be is a breaking change. If there is some way to refactor this so developers don't need to modify their code at all and it would still work, then we can look into that.

[timothylrobb]

Sorry it took me so long to update this. To get around this in a dynamic application, I've come up with the following solution:

1. Create your Menu Elements with Javascript. (Both the menu and the button to go with the menu.)
2. Call the upgradeElement function on the Menu and the upgradeElement on the button using the appropriate class names. "MaterialMenu" and "MaterialButton" respectively.
3. Add the 'click' and 'keydown' events to the button element to have it toggle the menu on those event: btn.addEventListener('click', menuelement.MaterialMenu.handleForClick_.bind(menuelement.MaterialMenu));

btn.addEventListener('keydown', menuelement.MaterialMenu.handleForKeyboardEvent_.bind(menuelement.MaterialMenu));

1. Not sure if this is required, but it's part of the init function, so... Add a reference to the button to the menuelement instance of MaterialMenu:

menuelement.MaterialMenu.forElement_ = btn;

This 3rd and 4th step should finalize element upgrade so that the button does toggle the menu.

[timothylrobb]

A possible permanent fix for this would to update the init function to locate the forElement in two different ways.

The current way is to use document.getElementById(forElementID), which works if the elements have already been added to the DOM, but you could also have it find the forElement by going from this.element_.parentElement.parentElement.getElementsByTagName("button"); and then scanning through the array of buttons to find the one with the correct ID. Once identified, you can set forEl equal to the button element found.

Here is how I would rewrite the Menu Init function:

MaterialMenu.prototype.init = function () {
    if (this.element_) {
        // Create container for the menu.
        var container = document.createElement('div');
        container.classList.add(this.CssClasses_.CONTAINER);
        this.element_.parentElement.insertBefore(container, this.element_);
        this.element_.parentElement.removeChild(this.element_);
        container.appendChild(this.element_);
        this.container_ = container;
        // Create outline for the menu (shadow and background).
        var outline = document.createElement('div');
        outline.classList.add(this.CssClasses_.OUTLINE);
        this.outline_ = outline;
        container.insertBefore(outline, this.element_);
        // Find the "for" element and bind events to it.
        var forElId = this.element_.getAttribute('for');
        var forEl = null;
        if (forElId) {
            forEl = document.getElementById(forElId);

            // ***** START OF NEW CODE *****
            if (!forEl) {
                var buttonsFound = this.element_.parentElement.parentElement.getElementsByTagName("button");
                for (var btn = 0; btn < buttonsFound.length; btn++) {
                      if (buttonsFound[btn].getAttribute("id") === forElId) {
                          forEl = buttonsFound[btn];
                      }
                }
            }
            // ***** END OF NEW CODE *****

            if (forEl) {
                this.forElement_ = forEl;
                forEl.addEventListener('click', this.handleForClick_.bind(this));
                forEl.addEventListener('keydown', this.handleForKeyboardEvent_.bind(this));
            }
        }
        var items = this.element_.querySelectorAll('.' + this.CssClasses_.ITEM);
        this.boundItemKeydown = this.handleItemKeyboardEvent_.bind(this);
        this.boundItemClick = this.handleItemClick_.bind(this);
        for (var i = 0; i < items.length; i++) {
            // Add a listener to each menu item.
            items[i].addEventListener('click', this.boundItemClick);
            // Add a tab index to each menu item.
            items[i].tabIndex = '-1';
            // Add a keyboard listener to each menu item.
            items[i].addEventListener('keydown', this.boundItemKeydown);
        }
        // Add ripple classes to each item, if the user has enabled ripples.
        if (this.element_.classList.contains(this.CssClasses_.RIPPLE_EFFECT)) {
            this.element_.classList.add(this.CssClasses_.RIPPLE_IGNORE_EVENTS);
            for (i = 0; i < items.length; i++) {
                var item = items[i];
                var rippleContainer = document.createElement('span');
                rippleContainer.classList.add(this.CssClasses_.ITEM_RIPPLE_CONTAINER);
                var ripple = document.createElement('span');
                ripple.classList.add(this.CssClasses_.RIPPLE);
                rippleContainer.appendChild(ripple);
                item.appendChild(rippleContainer);
                item.classList.add(this.CssClasses_.RIPPLE_EFFECT);
            }
        }
        // Copy alignment classes to the container, so the outline can use them.
        if (this.element_.classList.contains(this.CssClasses_.BOTTOM_LEFT)) {
            this.outline_.classList.add(this.CssClasses_.BOTTOM_LEFT);
        }
        if (this.element_.classList.contains(this.CssClasses_.BOTTOM_RIGHT)) {
            this.outline_.classList.add(this.CssClasses_.BOTTOM_RIGHT);
        }
        if (this.element_.classList.contains(this.CssClasses_.TOP_LEFT)) {
            this.outline_.classList.add(this.CssClasses_.TOP_LEFT);
        }
        if (this.element_.classList.contains(this.CssClasses_.TOP_RIGHT)) {
            this.outline_.classList.add(this.CssClasses_.TOP_RIGHT);
        }
        if (this.element_.classList.contains(this.CssClasses_.UNALIGNED)) {
            this.outline_.classList.add(this.CssClasses_.UNALIGNED);
        }
        container.classList.add(this.CssClasses_.IS_UPGRADED);
    }
};

With this code added, the proper dynamic usage of this would be similar to this:

var menuContainer = document.createElement("div");
var menuButton = document.createElement("button");
var menu = document.createElement("ul");

menuContainer.appendChild(menuButton);
menuContainer.appendChild(menu);

// Configure the button
menuButton.classList.add("mdl-button");
menuButton.classList.add("mdl-js-button");
menuButton.classList.add("mdl-button--icon");
menuButton.classList.add("mdl-js-ripple-effect"); // Because it's cool
menuButton.setAttribute("id", "myMenuButton");

// Let's create the HTML that goes inside the button
var buttonIcon = document.createElement("i");
buttonIcon.classList.add("material-icons");
buttonIcon.appendChild(document.createTextNode("more_vert"));
menuButton.appendChild(buttonIcon);

// Configure the menu
menu.classList.add("mdl-menu");
menu.classList.add("mdl-menu--bottom-left");
menu.classList.add("mdl-js-menu");
menu.classList.add("mdl-js-ripple-effect");
menu.setAttribute("for", "myMenuButton"); // Same as the ID assigned to the button

// Add some menu items
var menuItem1 = document.createElement("li");
menuItem1.classList.add("mdl-menu__item");
menuItem1.appendChild(document.createTextNode("Some Action");
menu.appendChild(menuItem1);

var menuItem2 = document.createElement("li");
menuItem2.classList.add("mdl-menu__item");
menuItem2.appendChild(document.createTextNode("Another Action");
menu.appendChild(menuItem2);

// Upgrade the Elements
componentHandler.upgradeElement(menuButton, "MaterialButton");
componentHandler.upgradeElement(menu, "MaterialMenu");  // This will only work if both the menu and the button are included in a container as done above.  If all goes well, and my code isn't full of bugs, the elements should correctly upgrade before being added to the DOM.

// Now you can append the menuContainer element to the DOM when you are ready to add it to the View.

[Garbee]

but you could also have it find the forElement by going from this.element_.parentElement.parentElement.getElementsByTagName("button");

Problem here, people may not have the exact structure, so this could then break. Still a BC concern changing from direct global query to accessing nodes by moving specifically up the DOM tree.

[timothylrobb]

True. That's why I recommend supporting both the global query and a dynamic solution. The dynamic solution that I built here creates the same kind of Menu tree as shown in the documentation and if developers stick to that structure, then it will work.

On the other hand, if deviation from this structure were required as you indicated, then perhaps further analysis would be required to support multiple circumstances.

Regardless, it's possible to get around this dynamically without any patches by manually adding the event listeners to the button to finish the component upgrade after the initial upgrade.

On Fri, Nov 13, 2015, 7:39 PM Jonathan Garbee notifications@github.com wrote:

but you could also have it find the forElement by going from this.element_.parentElement.parentElement.getElementsByTagName("button");

Problem here, people may not have the exact structure, so this could then break. Still a BC concern changing from direct global query to accessing nodes by moving specifically up the DOM tree.

— Reply to this email directly or view it on GitHub https://github.com/google/material-design-lite/issues/1491#issuecomment-156619930 .

[nakhodkin]

@timothylrobb Many thanks. Works great for me.

[brokenalarms]

Is this why I can't do any DOM diffing with the menu? This breaks the menu functionality after a second - looks like it does not get upgraded again after the diff.

var html = require('yo-yo')
const htmlView = () => html `<div class="mdl-layout mdl-js-layout mdl-layout--fixed-header">
      <header class="mdl-layout__header">
        <div class="mdl-layout__header-row">
          <span class="mdl-layout-title">app</span>

        </div>
      </header>
      <div class="mdl-layout__drawer">
        <span class="mdl-layout-title">Title</span>
        <nav class="mdl-navigation">
        </nav>
      </div>
      <main class="mdl-layout__content">
      </main>
    </div>
    </div>`
const firstView = htmlView()
document.body.appendChild(firstView)

setTimeout(function () {
  const newView = htmlView()
  html.update(firstView, newView)
}, 1000)

Apologies if this is one for the yo-yo/morphdom team, just let me know.

[brokenalarms]

Sorry, now read the various gotchas over upgradeDOM...won't upgrade because the same element is being morphed not reattached. Thanks!

[sgomes]

Closing, as any changes to this mechanism would be breaking, and thus out of scope. Please use one of the mechanisms for preventing auto-upgrade, and then trigger the update when attaching.