Closed Bug 758493 Opened 9 years ago Closed 4 months ago

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

Categories

(Calendar :: Calendar Frontend, defect)

Lightning 6.2
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 78.0

People

(Reporter: Fallen, Assigned: darktrojan)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 2 obsolete files)

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!
Yes, thats the goal. Thanks for the link!
Summary: Get rid of DOM Mutation events (DOMAttrModified), as they are deprecated → Warning: Use of Mutation Events (DOMAttrModified) is deprecated. Use MutationObserver instead.
Duplicate of this bug: 824353
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.
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.
Not to forget, removeEventListener becomes observer.disconnect();
>    for each (let m in mutations) {

Please use |for (let m of mutations) {|.
Assignee: nobody → mschroeder
Status: NEW → ASSIGNED
Assignee: mschroeder → nobody
Status: ASSIGNED → NEW
Duplicate of this bug: 1376512
Assignee: nobody → mschroeder
Status: NEW → ASSIGNED
Blocks: ltn62
Martin, is this something you can prepare still for 6.2?
Flags: needinfo?(mschroeder)
Attached patch Patch v1 (draft) (obsolete) — Splinter Review
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.
Flags: needinfo?(mschroeder)
Attachment #8966061 - Flags: feedback?(makemyday)
Keywords: site-compat
Keywords: site-compat
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 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.
Attachment #8966061 - Flags: feedback?(philipp)
Attachment #8966061 - Flags: feedback?(makemyday)
Attachment #8966061 - Flags: feedback+
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
Attachment #8966061 - Flags: feedback?(philipp) → feedback+
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.
Blocks: 1480039
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.
Version: Trunk → Lightning 6.2
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.
Flags: needinfo?(mschroeder)
Sorry, I could not get Tb + Calendar to build on my Mac, so no chance for me to finish this.
Flags: needinfo?(mschroeder)
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.
Attached patch Patch v2 (obsolete) — Splinter Review
Attachment #8966061 - Attachment is obsolete: true
(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.
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?
Flags: needinfo?(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).
Flags: needinfo?(mschroeder)
Attachment #9016998 - Flags: review?(makemyday)
Duplicate of this bug: 1548523
Attachment #9016998 - Flags: review?(makemyday)

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.

Assignee: mschroeder → nobody
Status: ASSIGNED → NEW

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?

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

Assignee: nobody → geoff
Status: NEW → ASSIGNED

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.

Attachment #9016998 - Attachment is obsolete: true
Attachment #9150924 - Flags: review?(paul)
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+.
Attachment #9150924 - Flags: review?(paul) → review+

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/b5646e5ec9f3
Remove calendar-daypicker binding and last use of DOMAttrModified in calendar. r=pmorris a=me

Status: ASSIGNED → RESOLVED
Closed: 4 months ago
Resolution: --- → FIXED
Target Milestone: --- → 78
You need to log in before you can comment on or make changes to this bug.