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)

enhancement
Not set
normal

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)

Attached patch mediaQueryList.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Attachment #8855772 - Flags: review?(jwatt)
Attachment #8855772 - Attachment is obsolete: true
Attachment #8855772 - Flags: review?(jwatt)
Attachment #8855810 - Flags: review?(jwatt)
Blocks: 1354599
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
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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)
> 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)
Given that the `MediaQueryListEvent` interface has the `matches` and `media` properties, this change actually won't break site compatibility, right?
Flags: needinfo?(amarchesini)
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)
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.
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: