Closed Bug 771043 Opened 8 years ago Closed 5 years ago

MediaQueryList doesn't dispatch events after entering responsive design mode

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: birtles, Assigned: bzbarsky)

References

Details

Attachments

(3 files, 2 obsolete files)

Attached image Inner SVG used in test case (obsolete) —
If you enter responsive design mode and don't refresh the page, then MediaQueryList on child content fails to dispatch events.

Steps to reproduce:

1. Open test case attachment
2 [review]. Open Web Console (Ctrl+Shift+K on Windows)
3. Open Responsive Design Mode (Ctrl+Shift+M on Windows)
4. Check console for notifications
5. Press rotate button
6. Check console for notifications
7. Press rotate button again
8. Check console for notifications

Expected results:

At steps (6) and (8) there should be messages indicating a matchMedia query event was received followed by a transition end. There probably shouldn't be one at step (4).

Actual results:

There are messages showing transitionend events were received at steps 4, 6, and 8 but no match media events.

If, after step (4), i.e. whilst in responsive design mode, one refreshes the page, the events are dispatched as expected. However, even after doing that, exiting and re-entering responsive design mode will cause the problem to re-occur.
Attachment #639224 - Attachment is obsolete: true
Attached file Test case
QA Contact: general
Blocks: 749628
came to re-report this (apparently). is there any planned movement on this? thanks!
I've just come across to this bug and came here to report.
Cameron, do you have cycles to look into this?
Flags: needinfo?(cam)
I don't, sorry; I'm busy this week and likely away for October.
Flags: needinfo?(cam)
OK, I see what's going on here.

The page is grabbing the MediaQueryList up front.  Then we enter responsive design mode and this creates a new prescontext.  Unfortunately, the MediaQueryList hangs off the prescontext, so now nothing inside Gecko has a pointer to that MediaQueryList anymore.

David, is there a reason we hung these off the prescontext, not the document?
Flags: needinfo?(dbaron)
I don't think so; I suspect I wasn't thinking about the distinction at the time and it seemed easier.  It's been a while, though.
Flags: needinfo?(dbaron)
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Two notes:

1)  A lot of this is just moving code from prescontext to document.

2)  If you'd prefer a web-platform-test here, let me know and I'll try to figure out how to do that.

Try push: https://tbpl.mozilla.org/?tree=Try&rev=00f690eeb849
Attachment #8494170 - Attachment is obsolete: true
Attachment #8494170 - Flags: review?(dbaron)
Comment on attachment 8494232 [details] [diff] [review]
Move MediaQueryList tracking from the prescontext to the document, so they will correctly outlive prescontext changes

nsIDocument.h:

>+   * Support for window.matchMedia()
>+   */
>+  already_AddRefed<mozilla::dom::MediaQueryList>
>+    MatchMedia(const nsAString& aMediaQueryList);
>+  const PRCList* MediaQueryLists() const {
>+    return &mDOMMediaQueryLists;
>+  }

This is probably more readable with a blank line between the two
method declarations (and maybe before the first as well, to match).

>+  // Our live MediaQueryLists
>+  PRCList               mDOMMediaQueryLists;

Ditch the excess whitespace.

MediaQueryList.cpp:

>+  if (!mDocument->GetShell() || !mDocument->GetShell()->GetPresContext()) {
>+    // XXXbz What's the right behavior here?  Spec doesn't say.
>+    return;
>+  }
>+
>+  mMatches = mMediaList->Matches(mDocument->GetShell()->GetPresContext(),
>+                                nullptr);

Maybe put the shell and/or prescontext in a variable to save
re-fetching?

Also, indent the nullptr one more space (if it's still wrapped).

> nsISupports*
> MediaQueryList::GetParentObject() const
> {
>-  if (!mPresContext) {
>+  if (!mDocument) {
>     return nullptr;
>   }
>-  return mPresContext->Document();
>+  return mDocument;
> }

Make the entire function body just "return mDocument;".


r=dbaron with that


Do we care about a window having more than one document in sequence?
Does that happen?  (XSLT?  document.open?)  (Even if it happens,
maybe we don't care.)
Attachment #8494232 - Flags: review?(dbaron) → review+
> Also, indent the nullptr one more space (if it's still wrapped).

It's not, but I added Emacs modelines to this file and its header to prevent tabs sneaking in like that again.

> Make the entire function body just "return mDocument;".

Oops.  Yes.

Addressed the other comments.

> Does that happen?

Only in one case, I think: replacement of initial about:blank by a newly loaded document that has the same origin.  This keeps the same Window but creates a new Document.

document.open has the reverse behavior: it keeps the same Document but creates a new Window.

I _could_ store these lists on the inner window instead of the document, I guess.  The spec doesn't actually describe what they're attached to; it probably should.

I _think_ XSLT creates both a new Document and a new Window...
I guess per a very literal reading of cssom this should live on the inner window.  Want me to just move it there?
Flags: needinfo?(dbaron)
That seems like excess work -- what you have now seems worth landing, and that seems not worth doing until the spec is clearer.
Flags: needinfo?(dbaron)
One other thought, though -- is there something that makes us send notifications if the characteristics of a new pres context are different from the old one?
Flags: needinfo?(bzbarsky)
The testcase suggests yes; I'll figure out what that something is exactly.
Blocks: 1011330
> The testcase suggests yes; I'll figure out what that something is exactly.

What's doing it is this stack:

#16 0x00000001076056cd in mozilla::dom::MediaQueryListListener::Call (this=0x126fbdcd0, list=@0x11f1743e0, aRv=@0x7fff5fbf1020, aExceptionHandling=mozilla::dom::CallbackObject::eReportExceptions) at MediaQueryListBinding.h:71
#17 0x00000001075eeeb6 in nsPresContext::MediaFeatureValuesChanged (this=0x126caf000, aShouldRebuild=nsPresContext::eRebuildStyleIfNeeded, aChangeHint=0) at nsPresContext.cpp:1912
#18 0x0000000107519e5f in nsPresContext::FlushPendingMediaFeatureValuesChanged (this=0x126caf000) at nsPresContext.h:271
#19 0x0000000107501692 in PresShell::FlushPendingNotifications (this=0x13245f400, aFlush={mFlushType = Flush_InterruptibleLayout, mFlushAnimations = true}) at nsPresShell.cpp:4201
#20 0x00000001075004cc in PresShell::FlushPendingNotifications (this=0x13245f400, aType=Flush_InterruptibleLayout) at nsPresShell.cpp:4110
#21 0x0000000107501204 in PresShell::HandlePostedReflowCallbacks (this=0x13245f400, aInterruptible=true) at nsPresShell.cpp:4078
#22 0x00000001074f9fc1 in PresShell::DidDoReflow (this=0x13245f400, aInterruptible=true, aWasInterrupted=false) at nsPresShell.cpp:8717
#23 0x00000001074f8afe in PresShell::ResizeReflowIgnoreOverride (this=0x13245f400, aWidth=6000, aHeight=3000) at nsPresShell.cpp:2072
#24 0x00000001074f8e58 in PresShell::ResizeReflow (this=0x13245f400, aWidth=6000, aHeight=3000) at nsPresShell.cpp:2013
#25 0x00000001069e6782 in nsViewManager::DoSetWindowDimensions (this=0x11d8b3800, aWidth=6000, aHeight=3000) at nsViewManager.cpp:192
#26 0x00000001069e68ea in nsViewManager::SetWindowDimensions (this=0x11d8b3800, aWidth=6000, aHeight=3000) at nsViewManager.cpp:212
#27 0x00000001074e2bfb in nsDocumentViewer::SetBounds (this=0x12cbeab40, aBounds=@0x126ca9a38) at nsDocumentViewer.cpp:1954
#28 0x0000000107c024ce in nsDocShell::SetPositionAndSize (this=0x126ca9800, x=0, y=0, cx=200, cy=100, fRepaint=false) at nsDocShell.cpp:5688
#29 0x0000000107c03047 in non-virtual thunk to nsDocShell::SetPositionAndSize(int, int, int, int, bool) () at nsDocShell.cpp:5672
#30 0x0000000106c739be in nsFrameLoader::UpdateBaseWindowPositionAndSize (this=0x11f4f1b00, aIFrame=0x11d140ad8) at nsFrameLoader.cpp:1851
#31 0x0000000106c7383a in nsFrameLoader::UpdatePositionAndSize (this=0x11f4f1b00, aIFrame=0x11d140ad8) at nsFrameLoader.cpp:1825
#32 0x00000001077251bf in nsSubDocumentFrame::ReflowFinished (this=0x11d140ad8) at nsSubDocumentFrame.cpp:802
#33 0x000000010772522c in non-virtual thunk to nsSubDocumentFrame::ReflowFinished() () at nsSubDocumentFrame.cpp:811
#34 0x00000001075011a5 in PresShell::HandlePostedReflowCallbacks (this=0x12f4d8800, aInterruptible=false) at nsPresShell.cpp:4069
#35 0x00000001074f9fc1 in PresShell::DidDoReflow (this=0x12f4d8800, aInterruptible=false, aWasInterrupted=false) at nsPresShell.cpp:8717
Flags: needinfo?(bzbarsky)
At least that covers the geometry end of things.
Hmph.  For some reason on Android media queries generally don't update sync.  See http://jsfiddle.net/zqw9gjxo/4/

Maybe the issue is that the interruptible-layout flush in the stack above doesn't happen there...

I guess I'll try to rework the test to not rely on that and instead assume notifications will be async.
Did that, and https://hg.mozilla.org/integration/mozilla-inbound/rev/fd343a8b5cfe
Target Milestone: mozilla35 → mozilla36
https://hg.mozilla.org/mozilla-central/rev/fd343a8b5cfe
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.