Closed
Bug 1451199
Opened 6 years ago
Closed 6 years ago
MediaQueryList is not consistent in its handling of change events
Categories
(Core :: DOM: CSS Object Model, enhancement)
Core
DOM: CSS Object Model
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
Details
Attachments
(1 file)
It treats addEventListener("change", stuff) differently from .onchange = stuff, because it hooks addEventListener but not SetOnChange. If we could switch it to using EventListenerAdded that would fix the problem, because event handler additions call that. David, do you see any problems with doing the RecomputeMatches bit after we have added the listener? It might make us fire the change event in some cases where we don't now (when a listener is added while there is a pending change to whether we match that gets synchronously flushed out by the flush that RecomputeMatches does, assuming such a sitauation can arise). But it looks like in general we can be in that situation and have the pending change _not_ get flushed out by that flush anyway... and so can other browsers.
Flags: needinfo?(dbaron)
Assignee | ||
Comment 1•6 years ago
|
||
This passes all our tests and fixes this bug. Without the fix, one of the two added tests times out due to never firing the event. Emilio, are you OK reviewing this? I'd probably ask dbaron except he's out for a few weeks here....
Attachment #8965227 -
Flags: review?(emilio)
Comment 2•6 years ago
|
||
Comment on attachment 8965227 [details] [diff] [review] Fix the handling of .onchange in MediaQueryList to match the handling of addEventListener('change') Review of attachment 8965227 [details] [diff] [review]: ----------------------------------------------------------------- Yeah, happy to review this. This patch looks good, thanks! ::: layout/style/test/test_bug1451199-1.html @@ +25,5 @@ > + is(l.matches, false, > + "Should not match portrait by the time we get notified"); > + SimpleTest.finish(); > + }; > + iframe.width = "200"; Should these live in WPT instead? I guess given the spec wording in https://drafts.csswg.org/cssom-view/#mediaquerylist-matches-state, this matches the specified behavior.
Attachment #8965227 -
Flags: review?(emilio) → review+
Assignee | ||
Comment 3•6 years ago
|
||
> Should these live in WPT instead? I trued reading the mediaquerylist spec, and the problem is that it does not specify behavior clearly enough to be able to write sane wpts for it, and insofar as it does I don't think we match the spec. Specifically, the events fire when "evaluate media queries and report changes" happens. But nothing in that spec says when it happens. That algorithm is invoked from HTML https://html.spec.whatwg.org/multipage/webappapis.html#update-the-rendering substep 7. So for example, flushing layout should not cause the event to fire (I _think_ it should only fire from a refresh tick), but does in Gecko. I didn't want to take on changing the entire model of when we fire this event right now, especially because I don't know whether other browsers match the spec in terms of when they do it, and if not what the web compat constraints are here...
Assignee | ||
Updated•6 years ago
|
Flags: needinfo?(dbaron)
Assignee | ||
Comment 4•6 years ago
|
||
I filed bug 1451717 on that last bit.
Pushed by bzbarsky@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/8052c4e8c9c6 Fix the handling of .onchange in MediaQueryList to match the handling of addEventListener('change'). r=emilio
Comment 6•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/8052c4e8c9c6
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in
before you can comment on or make changes to this bug.
Description
•