1354441 - Update MediaQueryList to the latest version of the spec

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement)

Description

[Andrea Marchesini [:baku]]

See https://drafts.csswg.org/cssom-view/#mediaquerylist

Comment 1

[Andrea Marchesini [:baku]]

Attachment: mediaQueryList.patch

Comment 2

[Andrea Marchesini [:baku]]

Attachment: mediaQueryList.patch

Comment 3

[Jonathan Watt [:jwatt]]

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.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c467c6d0d17e
Update MediaQueryList to the latest version of the spec, r=jwatt

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c467c6d0d17e

Comment 6

[Boris Zbarsky [:bzbarsky]]

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?

Comment 7

[Andrea Marchesini [:baku]]

> 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.

Comment 8

[Chris Mills [:cmills]]

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!

Comment 9

[Kohei Yoshino [:kohei]]

Given that the `MediaQueryListEvent` interface has the `matches` and `media` properties, this change actually won't break site compatibility, right?

Comment 10

[Andrea Marchesini [:baku]]

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.

Comment 11

[Kohei Yoshino [:kohei]]

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.