Add mutation observer to update styles on DOM changes

[samiheikki]

Fixes #46

---

This change is 

[platosha]

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @samiheikki)

---

src/vaadin-form-layout.html, line 289 at r1 (raw file):

          // Observe changes in form item's `colspan` attribute
          const config = {attributes: true, subtree: true};

subtree is probably an overkill, the form layout does only work on its direct children.

Would be nice to specify attributeFilter: ['colspan'], so that it would be only triggered by that attribute. Might require some additional filtering in the mutation callback too.

---

src/vaadin-form-layout.html, line 290 at r1 (raw file):

          // Observe changes in form item's `colspan` attribute
          const config = {attributes: true, subtree: true};
          this.__mutationObserver = new MutationObserver(this.updateStyles.bind(this));

Using updateStyles as a mutation observer callback is a bad idea:

• updateStyles is a Polymer.Element method that supports an arbitrary styles object as a first argument: https://github.com/Polymer/polymer/blob/7569d74dcb768693ae97639266e924f1d6e1f56c/lib/mixins/element-mixin.html#L613-L639. This is passed further to ShadyCSS to define / update custom CSS properties programmatically.
• MutationObserver’s callbacks receive sequence<MutationRecord> mutations as a first argument: https://dom.spec.whatwg.org/#callbackdef-mutationcallback

I can only guess what updateStyles does to mutations in this case. But whatever that is, treating mutations as styles is not useful and probably dangerous. Let us use some other method as a mutation observer callback, so that the argument is not silently passed down to ShadyCSS.

Hint: you might want to use _invokeUpdateStyles, which invokes updateStyles without any arguments. (It’s the only reason for this method to exist.)

---

src/vaadin-form-layout.html, line 290 at r1 (raw file):

          // Observe changes in form item's `colspan` attribute
          const config = {attributes: true, subtree: true};
          this.__mutationObserver = new MutationObserver(this.updateStyles.bind(this));

BTW I’m not sure if MutationObserver is reliable with ShadyDOM. Shady DOM has it’s own observation API. Last time I tried to do a similar task in a Vanilla JS component, I had to use it directly: https://github.com/webcomponents/shadydom/blob/e274f1ee776cfe19d73ba84351fea583846473f6/src/observe-changes.js#L59-L89

Typically, Polymer.FlattenedNodesObserver saves us from the most of that hassle: https://www.polymer-project.org/2.0/docs/api/classes/Polymer.FlattenedNodesObserver. However, it cannot observe attributes automatically.

I am not sure, the tests should tell better than me. My gut feeling is that this use case requires a two-level observer in this case:

• Polymer.FlattenedNodesObserver to observe for first-level flattened children of the form-layout
• Then, MutationObserver observing specifically for the colspan attribute on every child individually (only the attribute, not subtree, not childList).

---

src/vaadin-form-layout.html, line 291 at r1 (raw file):

          const config = {attributes: true, subtree: true};
          this.__mutationObserver = new MutationObserver(this.updateStyles.bind(this));
          this.__mutationObserver.observe(Polymer.dom(this).node, config);

Do not use Polymer.dom, it’s a legacy Polymer v1 API.

[samiheikki]

Thanks for the great review. I updated the PR based on your feedback.

[web-padawan]

Reviewable status: 0 of 5 files reviewed, 3 unresolved discussions (waiting on @platosha and @samiheikki)

---

src/vaadin-form-layout.html, line 290 at r1 (raw file):

Previously, platosha (Anton Platonov) wrote…

BTW I’m not sure if MutationObserver is reliable with ShadyDOM. Shady DOM has it’s own observation API. Last time I tried to do a similar task in a Vanilla JS component, I had to use it directly: https://github.com/webcomponents/shadydom/blob/e274f1ee776cfe19d73ba84351fea583846473f6/src/observe-changes.js#L59-L89

Typically, Polymer.FlattenedNodesObserver saves us from the most of that hassle: https://www.polymer-project.org/2.0/docs/api/classes/Polymer.FlattenedNodesObserver. However, it cannot observe attributes automatically.

I am not sure, the tests should tell better than me. My gut feeling is that this use case requires a two-level observer in this case:

• Polymer.FlattenedNodesObserver to observe for first-level flattened children of the form-layout
• Then, MutationObserver observing specifically for the colspan attribute on every child individually (only the attribute, not subtree, not childList).

@platosha updated the PR to use your approach with 2 observers, and added the test using dom-repeat to ensure it is indeed required to handle lazily stamped nodes correctly. PTAL

[platosha]

Reviewed 2 of 4 files at r2, 3 of 3 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @samiheikki)

[web-padawan]

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @samiheikki)

[web-padawan]

Reviewed 2 of 4 files at r2, 3 of 3 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @platosha)

---

src/vaadin-form-layout.html, line 290 at r1 (raw file):

Previously, platosha (Anton Platonov) wrote…

Using updateStyles as a mutation observer callback is a bad idea:

• updateStyles is a Polymer.Element method that supports an arbitrary styles object as a first argument: https://github.com/Polymer/polymer/blob/7569d74dcb768693ae97639266e924f1d6e1f56c/lib/mixins/element-mixin.html#L613-L639. This is passed further to ShadyCSS to define / update custom CSS properties programmatically.
• MutationObserver’s callbacks receive sequence<MutationRecord> mutations as a first argument: https://dom.spec.whatwg.org/#callbackdef-mutationcallback

I can only guess what updateStyles does to mutations in this case. But whatever that is, treating mutations as styles is not useful and probably dangerous. Let us use some other method as a mutation observer callback, so that the argument is not silently passed down to ShadyCSS.

Hint: you might want to use _invokeUpdateStyles, which invokes updateStyles without any arguments. (It’s the only reason for this method to exist.)

Done.

[platosha]

Reviewable status: complete! all files reviewed, all discussions resolved

[samiheikki]

Reviewable status: complete! all files reviewed, all discussions resolved

[web-padawan]

Reviewable status: complete! all files reviewed, all discussions resolved