Closed
Bug 1354441
Opened 8 years ago
Closed 8 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•8 years ago
|
||
Assignee: nobody → amarchesini
Attachment #8855772 -
Flags: review?(jwatt)
Assignee | ||
Comment 2•8 years ago
|
||
Attachment #8855772 -
Attachment is obsolete: true
Attachment #8855772 -
Flags: review?(jwatt)
Attachment #8855810 -
Flags: review?(jwatt)
![]() |
||
Comment 3•8 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•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
![]() |
||
Comment 6•8 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•8 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•8 years ago
|
Keywords: dev-doc-needed
Comment 8•8 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•8 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•8 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•8 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•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•