Closed Bug 1451199 Opened 2 years ago Closed 2 years ago

MediaQueryList is not consistent in its handling of change events

Categories

(Core :: DOM: CSS Object Model, enhancement)

enhancement
Not set

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)
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 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+
> 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...
Flags: needinfo?(dbaron)
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
https://hg.mozilla.org/mozilla-central/rev/8052c4e8c9c6
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.