Open Bug 758493 Opened 8 years ago Updated 5 months ago

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

Categories

(Calendar :: Calendar Views, defect)

Lightning 6.2
defect
Not set

Tracking

(Not tracked)

People

(Reporter: Fallen, Unassigned)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 1 obsolete file)

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 v2Splinter 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
You need to log in before you can comment on or make changes to this bug.