758493 - Warning: Use of Mutation Events (DOMAttrModified) is deprecated. Use MutationObserver instead.

Status: NEW

(Calendar :: Calendar Views, defect)

Description

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

The last AMO review suggests we get rid of the mutation events as fast as possible, I think we should try to do this indeed!

Comment 1

[Martin Schröder [:martinschroeder]]

Should we move to the new Mutation Observers (https://developer.mozilla.org/en/DOM/DOM_Mutation_Observers)?

Comment 2

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Yes, thats the goal. Thanks for the link!

Comment 3

[Stefan Sitter [:ssitter]]

http://mxr.mozilla.org/comm-central/search?string=DOMAttrModified&find=/calendar/

Comment 5

[Stefan Sitter [:ssitter]]

I looked at the documentation for MutationObserver but I got no handle on how to use it and replace our use of DOMAttrModified. Maybe someone else who has experience with MutationObserver could look at this so we don't get caught by surprise when Bug 831008 lands.

Comment 6

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

I think for example like this:

http://mxr.mozilla.org/comm-central/source/calendar/base/content/dialogs/calendar-event-dialog-attendees.js#30

> let observer = new MutationObserver(onAttrModified);
> let config = { attributes: true };
> observer.observe(window, config);


Then in the function

> function onAttrModified(mutations, observer) {
>    for each (let m in mutations) {
>       let value = m.target.getAttribute(m.attributeName);
>       ...
>    }
> }

I'm not quite sure if its needed to bend over backwards to get the originalTarget. If so, we probably need to do a bigger rewrite.

Comment 7

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Not to forget, removeEventListener becomes observer.disconnect();

Comment 8

[Masatoshi Kimura [:emk]]

>    for each (let m in mutations) {

Please use |for (let m of mutations) {|.

Comment 10

[[:MakeMyDay]]

Martin, is this something you can prepare still for 6.2?

Comment 11

[Martin Schröder [:martinschroeder]]

Attachment: Patch v1 (draft)

This is a draft of the patch, which I prepared months ago. I do not have a build to test all functionality at the moment, but I already know that there is at least an issue with the mode switching. It would be great to get some feedback until I can test this myself.

Comment 12

[Stefan Sitter [:ssitter]]

Quote from https://groups.google.com/d/msg/mozilla.dev.platform/YD4jeL00HHU/QenNmq8KCQAJ
> In bug 1460295, I intend to stop triggering DOMAttrModified event as well as the general event DOMSubtreeModified for changes on style attribute via CSSOM by default.

Might impact Lightning, assuming Lightning will work in Thunderbird 62 in the future.

Comment 13

[[:MakeMyDay]]

Comment on attachment 8966061 [details] [diff] [review]
Patch v1 (draft)

Review of attachment 8966061 [details] [diff] [review]:
-----------------------------------------------------------------

This looks ok-ish, though the patch needs to be de-bitrotted a little. I would appreciate if you can test it once the remaining wx related bugs landed (especially bug 1472883 and bug 1470044).

Philipp, can you take a look at this poatch, too, since you already commented earlier on the approach.

Comment 14

[Philipp Kewisch [:Fallen] [:📆][:🧩]]

Comment on attachment 8966061 [details] [diff] [review]
Patch v1 (draft)

Review of attachment 8966061 [details] [diff] [review]:
-----------------------------------------------------------------

Generally looks great, thanks for digging this out! A few minor code comments:

::: calendar/base/content/calendar-daypicker.xml
@@ +32,5 @@
>            this.setAttribute("type", "checkbox");
> +          this.mCheckedObserver = new MutationObserver(aMutations => {
> +              for (let mutation of aMutations) {
> +                  let event = document.createEvent("Events");
> +                  event.initEvent("select", true, true);

I think nowadays you can do 

let event = new UIEvent("select", { bubbles: true, cancelable: true });

::: calendar/base/content/dialogs/calendar-event-dialog-attendees.js
@@ +919,5 @@
> + *
> + * @param aMutations
> + */
> +function onHorizontalScrollbarCurposChange(aMutations) {
> +    aMutations.forEach(mutation => {

Would prefer for..of, I believe I once read forEach has more overhead. Same next function.

::: calendar/base/content/today-pane.js
@@ +418,3 @@
>       */
> +    onModeModified: function(aMutations) {
> +        aMutations.forEach(mutation => {

and here

Comment 15

[[:MakeMyDay]]

Comment on attachment 8966061 [details] [diff] [review]
Patch v1 (draft)

Review of attachment 8966061 [details] [diff] [review]:
-----------------------------------------------------------------

Lighnting is back on trunk, this patch needs a little debitrotting for the today-pane.js now. Do you have any cycles to finalize it?

::: calendar/base/content/calendar-daypicker.xml
@@ +30,5 @@
>        <constructor><![CDATA[
>            this.setAttribute("autoCheck", "true");
>            this.setAttribute("type", "checkbox");
> +          this.mCheckedObserver = new MutationObserver(aMutations => {
> +              for (let mutation of aMutations) {

Why are you iterating here over aMutations since you don't make any use of mutation?

@@ +41,3 @@
>        ]]></constructor>
> +      <destructor><![CDATA[
> +        this.mCheckedObserver.disconnect();

Indentation is too little here.

Comment 16

[[:MakeMyDay]]

Setting version to 6.2 since we would need this to get rid of the deprecation warning in 6.2 - the support stopped in from version 6.4 onwards, like mentioned in comment 12.

Comment 17

[[:MakeMyDay]]

Martin, can you update the poatch? We will have TB63 in a couple of days and it would be nice to have it fixed for that.

Comment 18

[Martin Schröder [:martinschroeder]]

Sorry, I could not get Tb + Calendar to build on my Mac, so no chance for me to finish this.

Comment 19

[[:MakeMyDay]]

Building on windows works flawlessly here (and I know on Linux it does as well). You're aware that c-c isn't the top directory any longer for building TB but instead neads to live in a |comm| subdirectory of m-c?

In the meanwhile, I'm looking to debitrot your patch, if you don't mind.

Comment 20

[Martin Schröder [:martinschroeder]]

Attachment: Patch v2

Comment 21

[Martin Schröder [:martinschroeder]]

(In reply to [:MakeMyDay] from comment #15)
> Comment on attachment 8966061 [details] [diff] [review]
> Patch v1 (draft)
[...]
> ::: calendar/base/content/calendar-daypicker.xml
> @@ +30,5 @@
> >        <constructor><![CDATA[
> >            this.setAttribute("autoCheck", "true");
> >            this.setAttribute("type", "checkbox");
> > +          this.mCheckedObserver = new MutationObserver(aMutations => {
> > +              for (let mutation of aMutations) {
> 
> Why are you iterating here over aMutations since you don't make any use of
> mutation?

If multiple mutation events happen at the same time there is just one call to the subscriber, so although it is unlikely to happen, it seems to be the right thing to do.

Comment 22

[[:MakeMyDay]]

Is (In reply to Martin Schröder [:martinschroeder] from comment #20)
> Created attachment 9016998 [details] [diff] [review]
> Patch v2

Is that intended for review now?

Comment 23

[Martin Schröder [:martinschroeder]]

Comment on attachment 9016998 [details] [diff] [review]
Patch v2

I still do not have a build to test the functionality (daypicker, mode switching with today pane, attendee dialog) at the moment (the os version on my mac is too old and I cannot update at the moment for other reasons).

Comment 25

[Martin Schröder [:martinschroeder]]

With all de-xbl work and new custom elements the last patch no longer applies. Custom elements also have an "attributeChangedCallback()" (together with "static get observedAttributes()") which might be better than the MutationObserver for some cases.