Closed
Bug 1354441
Opened 6 years ago
Closed 6 years ago
Update MediaQueryList to the latest version of the spec
Categories
(Core :: DOM: Core & HTML, enhancement)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: baku, Assigned: baku)
References
Details
(Keywords: dev-doc-complete)
Attachments
(1 file, 1 obsolete file)
26.83 KB,
patch
|
jwatt
:
review+
|
Details | Diff | Splinter Review |
Assignee | ||
Comment 1•6 years ago
|
||
Assignee: nobody → amarchesini
Attachment #8855772 -
Flags: review?(jwatt)
Assignee | ||
Comment 2•6 years ago
|
||
Attachment #8855772 -
Attachment is obsolete: true
Attachment #8855772 -
Flags: review?(jwatt)
Attachment #8855810 -
Flags: review?(jwatt)
![]() |
||
Comment 3•6 years ago
|
||
Comment on attachment 8855810 [details] [diff] [review] mediaQueryList.patch Review of attachment 8855810 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/style/MediaQueryList.cpp @@ +180,5 @@ > + > + mIsKeptAlive = toKeepAlive; > + > + if (toKeepAlive) { > + AddRef(); NS_ADDREF_THIS @@ +182,5 @@ > + > + if (toKeepAlive) { > + AddRef(); > + } else { > + Release(); NS_RELEASE_THIS ::: layout/style/test/test_media_query_list.html @@ -263,5 @@ > is(received_mql[1], mql, > "notification order (removal tests)"); > })(); > > - /* Bug 716751: null-dereference crash */ Seems like it might be worth modifying this rather than removing it, but then again it is a null-deref rather than something worse so maybe not.
Attachment #8855810 -
Flags: review?(jwatt) → review+
Pushed by amarchesini@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/c467c6d0d17e Update MediaQueryList to the latest version of the spec, r=jwatt
Comment 5•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c467c6d0d17e
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
![]() |
||
Comment 6•6 years ago
|
||
Was there an intent to implement/ship sent for this? This change is breaking backwards compat; the function argument will now be an event instead of the MediaQueryList that it used to be. Have we checked whether this is ok? What do other UAs do?
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 7•6 years ago
|
||
> Was there an intent to implement/ship sent for this? This change is
I'm sending the intent to implementship email right now.
Previously I checked what chrome does, and yes, it implements exactly the behavior I have introduced. They have MediaQueryListEvent and AddListener/RemoveListener as 'alias' of AddEventListener/RemoveEventListener.
Flags: needinfo?(amarchesini)
Updated•6 years ago
|
Keywords: dev-doc-needed
Comment 8•6 years ago
|
||
Documentation done! I have updated/checked the following (and subpages): https://developer.mozilla.org/en-US/docs/Web/API/MediaQueryList https://developer.mozilla.org/en-US/docs/Web/CSS/Media_Queries/Testing_media_queries https://developer.mozilla.org/en-US/docs/Web/API/Window/matchMedia I also wrote this (and subpages): https://developer.mozilla.org/en-US/docs/Web/API/MediaQueryListEvent https://github.com/mdn/dom-examples/blob/master/mediaquerylist/index.html And I added a note to the Fx55 release notes: https://developer.mozilla.org/en-US/Firefox/Releases/55#DOM_HTML_DOM Let me know if this all looks OK. Thanks!
Keywords: dev-doc-needed → dev-doc-complete
Comment 9•6 years ago
|
||
Given that the `MediaQueryListEvent` interface has the `matches` and `media` properties, this change actually won't break site compatibility, right?
Flags: needinfo?(amarchesini)
Assignee | ||
Comment 10•6 years ago
|
||
It breaks the website compatibility because the function parameter is a MediaQueryListEvent instead of a MediaQueryListener: instanceof will return a different result. Plus, MediaQueryListEvent doesn't have addListener/removeListener methods. The compatibility break is mitigated by matches and media attributes.
Flags: needinfo?(amarchesini)
Comment 11•6 years ago
|
||
Okay, so the only actual compatibility concern is any nested add/removeListener like this, but I guess it's a rare case: ```js let listener = mql => { // As of Firefox 55, mql is a MediaQueryListEvent // This still works mql.matches; // true or false mql.media; // '(max-width: 768px)' // This will stop working mql.removeListener(listener); // Remove the current listener mql.addListener( ... ); // Add another listener if necessary // Simple workaround (mql.target || mql).removeListener(listener); (mql.target || mql).addListener( ... ); }; window.matchMedia('(max-width: 768px)').addListener(listener); ``` I don't think a site compat note is not required for this change.
Updated•4 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•