Update MediaQueryList to the latest version of the spec

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
3 months ago

People

(Reporter: baku, Assigned: baku)

Tracking

({dev-doc-complete})

unspecified
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Comment 1

2 years ago
Posted patch mediaQueryList.patch (obsolete) — Splinter Review
Assignee: nobody → amarchesini
Attachment #8855772 - Flags: review?(jwatt)
Assignee

Comment 2

2 years ago
Attachment #8855772 - Attachment is obsolete: true
Attachment #8855772 - Flags: review?(jwatt)
Attachment #8855810 - Flags: review?(jwatt)
Assignee

Updated

2 years ago
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+

Comment 4

2 years ago
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/c467c6d0d17e
Status: NEW → RESOLVED
Closed: 2 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)
Assignee

Comment 7

2 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)
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

2 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)
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.
Duplicate of this bug: 1265622
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.