Last Comment Bug 708553 - Hovered element state is not relinquished when entering and exiting fullscreen mode
: Hovered element state is not relinquished when entering and exiting fullscree...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
:
Mentors:
Depends on:
Blocks: 545812 708814
  Show dependency treegraph
 
Reported: 2011-12-08 01:45 PST by Rob Hawkes [:rob]
Modified: 2012-05-18 14:36 PDT (History)
13 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (681 bytes, patch)
2012-02-20 20:07 PST, PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
cpearce: review-
Details | Diff | Splinter Review
Patch: reset hover content when prescontext destroyed (1.12 KB, patch)
2012-05-16 20:47 PDT, PTO until Sep 5 NZ time; Chris Pearce (:cpearce)
bzbarsky: review+
Details | Diff | Splinter Review

Description Rob Hawkes [:rob] 2011-12-08 01:45:28 PST
The hover state of the element that is clicked to envoke fullscreen mode (eg. adding a black border around a button) is not relinguished after entering fullscreen even though the mouse may not be over the element (the border remains). This behaviour continues to exist even upon exiting fullscreen mode (the border remains) and can only be resolved by placing the mouse over the original element and removing it again to reset its hover state (no more border).


Demo:

This behaviour happens for me with any code but the easiest example to show is the cross-browser demo: http://html5-demos.appspot.com/static/fullscreen.html


Steps to reproduce:

1. Create an obvious hover state for the button or element used to envoke fullscreen mode, or use the demo link above
2. Hover over said element, notice hover state (black border on the above demo), and click to envoke fullscreen
3. Notice hover state of recently clicked element has not been reset
4. Exit fullscreen mode
5. Notice hover state of recently clicked element has not been reset
6. Place mouse over element and then remove it from the element
7. Notice hover state has been relinquished
Comment 1 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2011-12-08 18:17:59 PST
Though F11 full-screen mode correctly relinquishes :hover state. Interesting...
Comment 2 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2011-12-08 18:21:58 PST
Thanks for filing the bug Rob.
Comment 3 Timothy Nikkel (:tnikkel) 2012-01-13 19:32:16 PST
Do we just need to dispatch a synth mouse move so hover gets updated properly?
Comment 4 Jared Wein [:jaws] (please needinfo? me) 2012-01-23 20:16:16 PST
cpearce, tn: Do you have some tips for David for fixing this bug?
Comment 5 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2012-01-23 20:25:48 PST
Not sure, I haven't looked into this. It would be worth trying Timothy Nickel's suggestion from comment 3 (I guess after we call SetFullScreen [1]). It's a mystery to me why this doesn't occur in F11 full-screen mode though.

Olli Petay or bz may also have some ideas.

[1] http://mxr.mozilla.org/mozilla-central/source/content/base/src/nsDocument.cpp#8483
Comment 6 Olli Pettay [:smaug] 2012-01-23 22:44:25 PST
:hover/:active etc state handling is bz' and dbaron's territory
Comment 7 Timothy Nikkel (:tnikkel) 2012-01-23 23:14:34 PST
More details for how to do comment 3. To schedule a synth mouse move to be dispatched you want the presshell (nsIPresShell) of the document and then call SynthesizeMouseMove on the presshell. Let me know if you need more info.
Comment 8 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-01-24 16:34:13 PST
Why doesn't our existing synthetic mouse move code handle this case?
Comment 9 Boris Zbarsky [:bz] 2012-02-03 22:30:15 PST
That's my question too.  There should be reflow happening when we enter fullscreen mode, and that shuld trigger posting of synth mouse events.  Is that happening?
Comment 10 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2012-02-15 20:15:59 PST
I suspect I'm hitting this bug, so I tried adding a call to SynthesizeMouseMove on the PresShell after we enter fullscreen and it doesn't fix this bug. By adding logging I can tell there's already a SynthesizeMouseMove happening on the corresponding PresContext anyway.
Comment 11 Timothy Nikkel (:tnikkel) 2012-02-15 20:23:12 PST
Can you remind me what we do with the DOM and styles and such in order to achieve fullscreen mode?
Comment 12 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2012-02-15 20:34:10 PST
The full-screen element has these styles applied to it:
http://mxr.mozilla.org/mozilla-central/source/layout/style/ua.css#249

And ancestors, including containing iframes and ancestors in parent frames, get these styles applied to them:
http://mxr.mozilla.org/mozilla-central/source/layout/style/full-screen-override.css#39
http://mxr.mozilla.org/mozilla-central/source/layout/style/ua.css#264

We apply the styles before we set the top-level window into fullscreen mode.
Comment 13 Timothy Nikkel (:tnikkel) 2012-02-15 20:54:38 PST
It appears this is a pre-existing problem with the old fullscreen mode (f11) as just that doesn't update hover properly.
Comment 14 Timothy Nikkel (:tnikkel) 2012-02-15 20:58:04 PST
It seems like the second part of the transition, when the tabbar/urlbar is slide off screen, is when the problem happens as hover does seem to be updated during the first part (going fullscreen, getting rid of the titlebar & menubar).
Comment 15 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2012-02-19 14:58:34 PST
So if I turn on DEBUG_MOUSE_LOCATION in nsPresShell.cpp and observe the calls to PresShell::ProcessSynthMouseMoveEvent() [1], it appears that when we enter fullscreen mPresShell::mMouseLocation is not being updated to reflect the new screen size, i.e. we're constantly calling ProcessSynthMouseMoveEvent, but mMouseLocation is the same before and after the transition to fullscreen mode.

Once we've entered fullscreen and moved the mouse, mMouseLocation gets updated to reflect the correct position, but even then the :hover state isn't updated on the formerly hovered element until you mouseover said element.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsPresShell.cpp#5372
Comment 16 Timothy Nikkel (:tnikkel) 2012-02-19 15:16:54 PST
I guess the OS won't send a mouse move if our window moves or changes size? That makes sense. Maybe when we get one of these size/move events in the widget code we should also grab the current mouse position and send a 'fake' mouse event so that we can update the mouse position.
Comment 17 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2012-02-20 15:16:34 PST
I'd better take this one...
Comment 18 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2012-02-20 20:07:59 PST
Created attachment 599040 [details] [diff] [review]
Patch

Turns out the problem is that the <browser> element is getting a position:fixed rule applied to it when we enter fullscreen mode, which is causing a reframe, which causes the nsEventStateManager to be recreated, and the new nsEventStateManager doesn't have references to the old one's hover element, so we don't reset the hover state when the mouse moves thanks to a synthetic mouse event.

If we just don't apply position:fixed to the xul elements, we avoid the reframe, and everything works as expected.
Comment 19 Boris Zbarsky [:bz] 2012-02-20 20:09:53 PST
Would you still run into issues if you trigger fullscreen on something in a subframe?  I don't see why XUL is special here...
Comment 20 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2012-02-20 20:12:17 PST
(In reply to Boris Zbarsky (:bz) from comment #19)
> Would you still run into issues if you trigger fullscreen on something in a
> subframe?  I don't see why XUL is special here...

Hmmm, yes, it appears so... What should the rule be?
Comment 21 Boris Zbarsky [:bz] 2012-02-20 20:16:50 PST
The right fix would seem to be to deal with preserving whatever state we need across reframes...
Comment 22 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-20 20:19:27 PST
I guess the patch is a bit of a workaround. It works because the <browser> element which goes fullscreen in the chrome document doesn't need any styling, since the chrome document responds to the fullscreen event by changing its layout to make the <browser> fullscreen anyway.

The underlying bug we're working around is that changing an iframe to position:fixed reframes the subdocument, which destroys and recreates the subdocument's presentation, which loses information about hover state. Maybe we should fix the underlying bug so that frame reconstruction doesn't destroy/recreate the subdocument presentation? That's possible I guess, but tricky. At least it would require view reparenting.
Comment 23 Timothy Nikkel (:tnikkel) 2012-02-21 22:44:20 PST
Keeping the old presentation around doesn't seem too bad, but if we are reframing because the iframe was made display: none how exactly do we determine that we should throw away the presentation?
Comment 24 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-28 04:23:42 PST
(In reply to Timothy Nikkel (:tn) from comment #23)
> Keeping the old presentation around doesn't seem too bad, but if we are
> reframing because the iframe was made display: none how exactly do we
> determine that we should throw away the presentation?

How about making the call to HideViewer from nsSubDocumentFrame::DestroyFrom happen off nsContentUtils::AddScriptRunner, and in that scriptrunner check to see if the <iframe> element has no frame before calling HideViewer?
Comment 25 Timothy Nikkel (:tnikkel) 2012-02-28 22:53:23 PST
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (away February 21 to March 17) from comment #24)
> How about making the call to HideViewer from nsSubDocumentFrame::DestroyFrom
> happen off nsContentUtils::AddScriptRunner, and in that scriptrunner check
> to see if the <iframe> element has no frame before calling HideViewer?

Wouldn't we need to do it at the next refresh driver tick because that is when we would actually do the async work to recreate the frame? That would make the bookkeeping to keep track of the work that needs to be done a little more involved.
Comment 26 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-02-29 16:32:43 PST
Just have the scriptrunner do a FlushPendingNotifications(Flush_Frames) before checking whether a frame exists. Sure, that disables lazy frame construction when a subdocument frame is destroyed, but that probably doesn't matter.
Comment 27 Timothy Nikkel (:tnikkel) 2012-02-29 23:06:48 PST
Or we could have add a refresh observer when a subdocument frame gets destroyed to do that work.
Comment 28 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-03-01 01:36:29 PST
Leaving the presentation unhidden for that long might be risky.
Comment 29 Timothy Nikkel (:tnikkel) 2012-03-02 00:18:37 PST
Hadn't thought of it that way. What kind of risks are you thinking of?
Comment 30 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2012-03-13 15:23:28 PDT
So... What's the best way to accomplish preserving the presentation across reframes? We've talked about only calling HideViewer() in a script runner if the subdocframe is display:none, but what should we do in the non-display:none case? What state do we need to stash, where and when should we stash it, and where should we restore it?
Comment 31 Timothy Nikkel (:tnikkel) 2012-03-14 00:34:53 PDT
Just off the top of my head we'd take the root view of the subdocument and stash it on the content node for the subdocument frame. We'd check before we called ShowViewer after reconstructing the subdocument frame if the content node has a stashed root view for a subdocument. We'd do like comment 26 and kick off a scriptrunner when we do the stashing, after the flush the scriptrunner would check if the content node for the subdocument frame had a frame, if it didn't it would unstash and call HideViewer.
Comment 32 Lozzy 2012-04-05 12:58:57 PDT
Possibly related: Youtube's WebM player has a full screen button with a title attribute. Clicking the full screen button triggers the tooltip, which only disappears upon input. So it sits there interrupting the video until you do something.

Not sure if that is in the scope of this bug or whether tooltips are too far unrelated to hover states.
Comment 33 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2012-04-09 15:11:30 PDT
Lozzy: I imagine tooltips are implemented using the hover/mouseover status, so I'd guess they're related. Thanks for commenting.
Comment 34 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2012-05-16 20:47:39 PDT
Created attachment 624637 [details] [diff] [review]
Patch: reset hover content when prescontext destroyed

Since I'm reliably informed that I won't get a chance to work on anything except OMG MOBILE VIDEO for a while, so I think we should take this simple fix rather than waiting for me to free up enough to cycles to fix the reframing code.

This patch just resets the hover state when the prescontext is destroyed. The synthetic mouse move after reframing ensures hover state is reapplied correctly after the reframe in the new presentation.
Comment 35 Boris Zbarsky [:bz] 2012-05-17 19:59:33 PDT
Comment on attachment 624637 [details] [diff] [review]
Patch: reset hover content when prescontext destroyed

r=me.
Comment 36 PTO until Sep 5 NZ time; Chris Pearce (:cpearce) 2012-05-17 20:12:21 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/2359a20631cc
Comment 37 :Ms2ger (⌚ UTC+1/+2) 2012-05-18 13:30:35 PDT
https://hg.mozilla.org/mozilla-central/rev/2359a20631cc

Note You need to log in before you can comment on or make changes to this bug.