Warning: Use of Mutation Events (DOMAttrModified) is deprecated. Use MutationObserver instead.
Categories
(Calendar :: Calendar Frontend, defect)
Tracking
(Not tracked)
People
(Reporter: Fallen, Assigned: darktrojan)
References
(Blocks 1 open bug)
Details
Attachments
(1 file, 2 obsolete files)
29.20 KB,
patch
|
pmorris
:
review+
|
Details | Diff | Splinter Review |
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•12 years ago
|
||
Should we move to the new Mutation Observers (https://developer.mozilla.org/en/DOM/DOM_Mutation_Observers)?
Reporter | ||
Comment 2•12 years ago
|
||
Yes, thats the goal. Thanks for the link!
Comment 3•12 years ago
|
||
http://mxr.mozilla.org/comm-central/search?string=DOMAttrModified&find=/calendar/
Updated•12 years ago
|
Comment 5•11 years ago
|
||
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.
Reporter | ||
Comment 6•11 years ago
|
||
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.
Reporter | ||
Comment 7•11 years ago
|
||
Not to forget, removeEventListener becomes observer.disconnect();
Comment 8•11 years ago
|
||
> for each (let m in mutations) {
Please use |for (let m of mutations) {|.
Updated•10 years ago
|
Updated•9 years ago
|
Updated•9 years ago
|
Updated•7 years ago
|
Comment 10•6 years ago
|
||
Martin, is this something you can prepare still for 6.2?
Comment 11•6 years ago
|
||
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.
Updated•6 years ago
|
Updated•6 years ago
|
Comment 12•6 years ago
|
||
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•6 years ago
|
||
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.
Reporter | ||
Comment 14•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
Sorry, I could not get Tb + Calendar to build on my Mac, so no chance for me to finish this.
Comment 19•6 years ago
|
||
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•6 years ago
|
||
Comment 21•6 years ago
|
||
(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•6 years ago
|
||
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?
Updated•6 years ago
|
Comment 23•6 years ago
|
||
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).
Updated•5 years ago
|
Comment 25•5 years ago
|
||
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•4 years ago
|
||
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);
}
}
}
// 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?
Assignee | ||
Comment 27•4 years ago
|
||
There's only two left and I'm about to kill one in bug 1639763. I'll do the other here.
Assignee | ||
Comment 28•4 years ago
|
||
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•4 years ago
|
||
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+.
Assignee | ||
Updated•4 years ago
|
Comment 30•4 years ago
|
||
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
Assignee | ||
Updated•4 years ago
|
Description
•