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

Status: RESOLVED FIXED

(Calendar :: Calendar Frontend, 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 [:mschroeder]]

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 [:mschroeder]]

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 [:mschroeder]]

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 [:mschroeder]]

Attachment: Patch v2

Comment 21

[Martin Schröder [:mschroeder]]

(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 [:mschroeder]]

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 [:mschroeder]]

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.

Comment 26

[ISHIKAWA, Chiaki]

Mochitest is here to stay since December 2019.
During local mochitest run with FULL DEBUG version of TB, I noticed a few warnings related to the issue in this bugzilla.

Here is a summary printed by local shell script to summarize the errors/warnings in the log.
I inserted the bugzilla number so that I will check this bugzilla until these warnings are sorted out.

========================================
Deprecated:  (checked since Apri 23, 2015)
modulo process id and timestamp
https://bugzilla.mozilla.org/show_bug.cgi?id=758493
 Bug 758493 Warning: Use of Mutation Events (DOMAttrModified) is deprecated. Use MutationObserver instead.
========================================

     10 14:15.26 INFO Console message: [JavaScript Warning: "Use of Mutation Events is deprecated. Use MutationObserver instead." {file: "chrome://calendar/content/calendar-daypicker.js" line: 27}]
      5  3:15.60 INFO Console message: [JavaScript Warning: "Use of Mutation Events is deprecated. Use MutationObserver instead." {file: "chrome://calendar/content/calendar-event-dialog-attendees.js" line: 44}]

There are now only two places where these are (dynamically ?) executed.: Not sure if the warning is printed at the time of parsing or execution.
https://searchfox.org/comm-central/source/calendar/base/content/calendar-daypicker.js#27

      this.setAttribute("disable-on-occurrence", "true");

      this.addEventListener("DOMAttrModified", this.onModified);  <==== This
    }

    onModified(aEvent) {
      if (aEvent.attrName == "checked") {
        let event = document.createEvent("Events");
        event.initEvent("select", true, true);
        this.calendar.dispatchEvent(event);
      }
    }
  }

https://searchfox.org/comm-central/source/calendar/base/content/dialogs/calendar-event-dialog-attendees.js#44

  // first of all, attach all event handlers
  window.addEventListener("resize", onResize, true);
  window.addEventListener("rowchange", onRowChange, true);
  window.addEventListener("DOMAttrModified", onAttrModified, true);   <=== 
  window.addEventListener("timebar", onTimebar, true);
  window.addEventListener("timechange", onTimeChange, true

onAttrModified seems to be the one defined at
https://searchfox.org/comm-central/source/calendar/base/content/dialogs/calendar-event-dialog-attendees.js#847

I don't have to file a separate bugzilla for these warnings from mochitest, correct?

Comment 27

[Geoff Lankow (:darktrojan)]

There's only two left and I'm about to kill one in bug 1639763. I'll do the other here.

Comment 28

[Geoff Lankow (:darktrojan)]

Attachment: 758493-calendar-daypicker-1.diff

We don't even need this event listener, it works just as well without – the command event bubbles further up the DOM tree and is caught by another listener.

Without the listener all this particular binding does is add some attributes to itself, so let's just do that in the .xhtml file and get rid of the binding. Also some more-modern CSS to get rid of some unnecessary attributes while we're here.

Comment 29

[Paul Morris [:pmorris]]

Comment on attachment 9150924 [details] [diff] [review]
758493-calendar-daypicker-1.diff

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

Looks good.  Nice to be able to drop the CE and use the newer CSS.  I haven't tried applying the patch due to current bustage, but seems straightforward enough, so r+.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/b5646e5ec9f3
Remove calendar-daypicker binding and last use of DOMAttrModified in calendar. r=pmorris a=me