Bug 203288 – MediaQueryList should extend EventTarget

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Alexey Shvayka]

Initially, CSSOM View Module specification (https://www.w3.org/TR/2011/WD-cssom-view-20110804/#mediaquerylist) had a custom callback mechanism with addListener() and removeListener(), and the callback was invoked with the associated media query list as argument.
Now the normal event mechanism is used instead: https://www.w3.org/TR/cssom-view-1/#mediaquerylist.
For backwards compatibility, the addListener() and removeListener() methods are basically aliases for addEventListener() and removeEventListener(), respectively, and the "change" event masquerades as a MediaQueryList.

Other browsers have already shipped the change.
To do list for Safari:
1. Implement MediaQueryListEvent interface.
2. Make MediaQueryList subclass of EventTarget and emit MediaQueryListEvent.
3. Implement MediaQueryList::onchange attribute.

[Radar WebKit Bug Importer]

<rdar://problem/57417267>

[Alexey Shvayka]

Created attachment 384151 [details]
Patch

[Alexey Shvayka]

While this patch is ready for review, WPT tests are coming in https://github.com/web-platform-tests/wpt/pull/20401. I will re-sync wpt/css/cssom-view and rebase this patch as soon as it is merged.

[Sam Weinig]

Comment on attachment 384151 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=384151&action=review

> Source/WebCore/css/MediaQueryList.cpp:60
> +    addEventListener("change", listener.releaseNonNull());

The IDL declares the listener parameter as optional, so I don't think you can remove the null check if you are going to keep this listener.releaseNonNull(). Same with removeListener below.

[Alexey Shvayka]

Created attachment 387214 [details]
Patch

Bring back null checks and adjust expectations.

[Antti Koivisto]

Comment on attachment 387214 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=387214&action=review

Looks good in general.

> Source/WebCore/css/MediaQueryMatcher.cpp:116
> +    for (auto& mql : m_mediaQueryLists) {

WebKit coding style doesn't like abbreviations. This should be "mediaQueryList" or maybe just "list" if the context is clear. (the existing media query code is old and doesn't necessarily follow this)

> LayoutTests/imported/w3c/web-platform-tests/css/cssom-view/MediaQueryList-addListener-handleEvent-expected.txt:10
> +CONSOLE MESSAGE: line 90: [object Object]
>  
> -FAIL calls handleEvent method of event listener promise_test: Unhandled rejection with value: object "TypeError: Argument 1 ('listener') to MediaQueryList.addListener must be a function"
> -FAIL looks up handleEvent method on every event dispatch promise_test: Unhandled rejection with value: object "TypeError: Argument 1 ('listener') to MediaQueryList.addListener must be a function"
> -PASS doesn't look up handleEvent method on callable event listeners 
> -FAIL rethrows errors when getting handleEvent promise_rejects_exactly: function "function () { throw e }" threw object "TypeError: Argument 1 ('listener') to MediaQueryList.addListener must be a function" but we expected it to throw object "[object Object]"
> -PASS throws if handleEvent is falsy and not callable 
> -PASS throws if handleEvent is thruthy and not callable 
> +Harness Error (TIMEOUT), message = null
>  
> +PASS calls handleEvent method of event listener 
> +PASS looks up handleEvent method on every event dispatch 
> +PASS doesn't look up handleEvent method on callable event listeners 
> +TIMEOUT rethrows errors when getting handleEvent Test timed out
> +NOTRUN throws if handleEvent is falsy and not callable 
> +NOTRUN throws if handleEvent is thruthy and not callable 

Looks like one test started timing out and the rest are not run. It is not ok to rebase that.

[Antti Koivisto]

Comment on attachment 387214 [details]
Patch

r- due to test timeouts. This looks pretty good though, it would be nice to get it landed.

[Alexey Shvayka]

(In reply to Antti Koivisto from comment #7)
> Comment on attachment 387214 [details]
> Patch
> 
> r- due to test timeouts.

Thank you, Antti.

https://github.com/web-platform-tests/wpt/pull/21186 fixed TIMEOUT issue, yet the tests still remain false-positive.
https://github.com/web-platform-tests/wpt/pull/22384 fixes the tests.
I will sync WPT and update the patch as soon as it gets merged.

[Alexey Shvayka]

Created attachment 394720 [details]
Patch

Rename `mql`, adjust tests, and tweak ChangeLog.

[Darin Adler]

Comment on attachment 394720 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394720&action=review

Looks good. I am not sure we have enough test coverage for object lifetime issues.

> Source/WebCore/css/MediaQueryList.cpp:63
> +    addEventListener("change", listener.releaseNonNull());

eventNames().changeEvent rather than "change"

> Source/WebCore/css/MediaQueryList.cpp:71
> +    removeEventListener("change", *listener);

Ditto.

> Source/WebCore/css/MediaQueryList.h:26
>  #include <wtf/Forward.h>
>  #include <wtf/RefCounted.h>
>  #include <wtf/RefPtr.h>

None of these includes are needed now that we include EventTarget.h.

> Source/WebCore/css/MediaQueryList.h:43
> +    static Ref<MediaQueryList> create(ScriptExecutionContext&, MediaQueryMatcher&, Ref<MediaQuerySet>&&, bool matches);

I suggest this take a Document& instead of a ScriptExecutionContext&.

> Source/WebCore/css/MediaQueryList.h:58
> +    MediaQueryList(ScriptExecutionContext&, MediaQueryMatcher&, Ref<MediaQuerySet>&&, bool matches);

I suggest this take a Document& instead of a ScriptExecutionContext&.

> Source/WebCore/css/MediaQueryList.idl:26
> +    void addListener(EventListener? listener);
> +    void removeListener(EventListener? listener);

Is it important for compatibility that the listener argument be nullable? If not we should remove that because it’s peculiar and unnecessary.

> Source/WebCore/css/MediaQueryListEvent.h:29
> +#include <wtf/IsoMallocInlines.h>

I think these inlines might not be needed in the header if we moved the create functions out of the header. Not sure.

> Source/WebCore/css/MediaQueryListEvent.h:39
> +    static Ref<MediaQueryListEvent> create(const AtomString& type, const String& media, bool matches)
> +    {
> +        return adoptRef(*new MediaQueryListEvent(type, media, matches));
> +    }

I don’t think it’s valuable to have this inlined in the header. It would be better for runtime performance if the constructor was inlined in a non-inline create function, and better for compile speed to have less in the header.

> Source/WebCore/css/MediaQueryListEvent.h:42
> +        String media { "" };

emptyString(), not "", is more efficient

But also, is it important that media ia an empty string rather than a null string? Normally I’d expect a null string would suffice.

> Source/WebCore/css/MediaQueryListEvent.h:46
> +    static Ref<MediaQueryListEvent> create(const AtomString& type, const Init& init, IsTrusted isTrusted = IsTrusted::No)

Same thought about not inlining this.

> Source/WebCore/css/MediaQueryMatcher.cpp:118
> +        list->evaluate(evaluator, notify);

Since list is a weak pointer, it seems we would need to check for null here.

> Source/WebCore/css/MediaQueryMatcher.cpp:121
> +            auto event = MediaQueryListEvent::create(eventNames().changeEvent, list->media(), list->matches());
> +            list->dispatchEvent(event);

Do we get value from the local variable here? I think this would be better as a one-liner even if the line is a little longer.

[Darin Adler]

Comment on attachment 394720 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394720&action=review

>> Source/WebCore/css/MediaQueryListEvent.h:42
>> +        String media { "" };
> 
> emptyString(), not "", is more efficient
> 
> But also, is it important that media ia an empty string rather than a null string? Normally I’d expect a null string would suffice.

Could answer this question with tests, I think. If null string doesn’t work we should have a test that would fail if this is a null string.

Or Chris Dumez might just know this answer. Surely this has come up in other structures that are part of DOM bindings.

[Alexey Shvayka]

Created attachment 394740 [details]
Patch

Address Darin's comments.

[Darin Adler]

Comment on attachment 394740 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=394740&action=review

> Source/WebCore/css/MediaQueryListEvent.h:36
> +        String media { nullString() };

No need to write { nullString() } … that’s the default initialization for WTF::String.

[Alexey Shvayka]

Created attachment 394746 [details]
Patch

Drop nullString() and add optional EventListener tests.

[Alexey Shvayka]

(In reply to Darin Adler from comment #13)
> No need to write { nullString() } … that’s the default initialization for
> WTF::String.

Sorry!

Now MediaQueryListEvent::EventInit is consistent with other DOM bindings with its default values tested in web-platform-tests/css/cssom-view/MediaQueryListEvent.

(In reply to Darin Adler from comment #10)
> Is it important for compatibility that the listener argument be nullable? If
> not we should remove that because it’s peculiar and unnecessary.

Sadly it is required, both Chrome and Firefox accept .addListener(null | undefined), which I added tests for in fast/media/media-query-list-07.html. Zero argument .addListener() throwing TypeError is covered by web-platform-tests/css/cssom-view/idlharness.html.

Regarding test failures in web-platform-tests/css/cssom-view/MediaQueryList-addListener-handleEvent.html: those errors are visible in DevTools, but not caught correctly by "error" handler. It seemed like cross-realm issue at first, but it also reproduces in single-realm XPathNSResolver tests (web-platform-tests/domxpath/callback-interface.html). Chrome is also affected. I will open a bug as soon as I triage it a little more.

[Alexey Shvayka]

*** Bug 156730 has been marked as a duplicate of this bug. ***

[Alexey Shvayka]

Created attachment 396083 [details]
Patch

Rebase patch.

[Alexey Shvayka]

(In reply to Darin Adler from comment #10)
> Comment on attachment 394720 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=394720&action=review
> 
> Looks good. I am not sure we have enough test coverage for object lifetime
> issues.

Darin, could you please give a hint on how such a test may look like?
I am not sure MediaQueryList::~MediaQueryList() destructor is observable enough so it could be tested.
There are a few tests for wrappers like LayoutTests/crypto/crypto-gc.html, yet I am not positive we need one since MediaQueryList's wrapper is not special.
Is there something else we should do before landing?

[Darin Adler]

(In reply to Alexey Shvayka from comment #18)
> Darin, could you please give a hint on how such a test may look like?
> I am not sure MediaQueryList::~MediaQueryList() destructor is observable
> enough so it could be tested.
> There are a few tests for wrappers like LayoutTests/crypto/crypto-gc.html,
> yet I am not positive we need one since MediaQueryList's wrapper is not
> special.
> Is there something else we should do before landing?

I don't think there is anything we have to do before landing.

Here is what I can think of regarding lifetime:

- If we add a listener to an object that also captures that same object, we need to make sure that the object and the listener can still both be garbage collected and there is no cycle. That's what fast/dom/reference-cycle-leaks.html tests for. But I don't think we need a special test for MediaQueryList since it's just using a normal event listener.

- If we add a listener to an object and all other references to the object are deleted, we need to make sure that the listener still fires when the event should be dispatched and that when it does the object's wrapper still has custom properties (not a new wrapper). An attempt to test for something like this is http/tests/xmlhttprequest/event-listener-gc.html but on review that particular test doesn't seem to be correctly written. Garbage collection is not consistent enough; there is no guarantee a single object will be collected if we only run the test once. I don't know if have good tests for this. I don’t know what keeps a MediaQueryList alive when there are no references to it any more, but its existence can still be observed indirectly by whether events fire or not. This is probably the test that should be written, and there may be a real bug here.

- Generally it's an anti-pattern to have a RefCounted object that is held in a collection in another object that removes itself when it's destroyed. That's because whether the object should be iterated or not shouldn't really be decided by whether it happens to have been destroyed or not. I can't see any practical problem with this, but if we could think of what might go wrong we'd write that kind of test.

[Antti Koivisto]

Comment on attachment 396083 [details]
Patch

Sounds like everyone is ok to land this.

[EWS]

Committed r260243: <https://trac.webkit.org/changeset/260243>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 396083 [details].

[Antti Koivisto]

Thanks for the patch!

[Alexey Shvayka]

(In reply to Darin Adler from comment #19)

I appreciate the detailed write-up, thank you!
I will make sure to update http/tests/xmlhttprequest/event-listener-gc.html as soon as I get better understanding of GC design.

MQL seems to be kept alive by an event listener, same with other browsers:

```
;(onchange => {
  mql = matchMedia("(min-width: 600px)")
  mql.onchange = onchange
})(() => console.log("mql.onchange"))
```

> - Generally it's an anti-pattern to have a RefCounted object that is held in
> a collection in another object that removes itself when it's destroyed.

That is a great point, I will think through how we can decouple MediaQueryList from MediaQueryMatcher, or maybe set a flag in destructor and remove MQL from `matcher` during next evaluation round.

(In reply to Antti Koivisto from comment #22)
> Thanks for the patch!

Thank you for reviewing & landing this!