Closed Bug 708553 Opened 13 years ago Closed 12 years ago

Hovered element state is not relinquished when entering and exiting fullscreen mode

Categories

(Core :: DOM: Core & HTML, defect)

Product:
Core
Component:
DOM: Core & HTML
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla15

People

(Reporter: rob, Assigned: cpearce)

References

Details

Attachments

(1 file, 1 obsolete file)

Patch: reset hover content when prescontext destroyed
1.12 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review 
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
Though F11 full-screen mode correctly relinquishes :hover state. Interesting...
Thanks for filing the bug Rob.
Blocks: 545812
Do we just need to dispatch a synth mouse move so hover gets updated properly?
cpearce, tn: Do you have some tips for David for fixing this bug?
Assignee: nobody → david.c.seifried
Status: NEW → ASSIGNED
OS: Mac OS X → All
Hardware: x86 → All
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
:hover/:active etc state handling is bz' and dbaron's territory
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.
Why doesn't our existing synthetic mouse move code handle this case?
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?
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.
Can you remind me what we do with the DOM and styles and such in order to achieve fullscreen mode?
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.
It appears this is a pre-existing problem with the old fullscreen mode (f11) as just that doesn't update hover properly.
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).
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
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.
I'd better take this one...
Assignee: david.c.seifried → cpearce
Attached patch Patch (obsolete) — — Splinter Review 
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.
Attachment #599040 - Flags: review?(roc)
Would you still run into issues if you trigger fullscreen on something in a subframe?  I don't see why XUL is special here...
(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?
The right fix would seem to be to deal with preserving whatever state we need across reframes...
Attachment #599040 - Flags: review?(roc) → review-
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.
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?
(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?
(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.
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.
Or we could have add a refresh observer when a subdocument frame gets destroyed to do that work.
Leaving the presentation unhidden for that long might be risky.
Hadn't thought of it that way. What kind of risks are you thinking of?
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?
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.
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.
Lozzy: I imagine tooltips are implemented using the hover/mouseover status, so I'd guess they're related. Thanks for commenting.
No longer blocks: 716107
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.
Attachment #599040 - Attachment is obsolete: true
Attachment #624637 - Flags: review?(bzbarsky)
Comment on attachment 624637 [details] [diff] [review]
Patch: reset hover content when prescontext destroyed

r=me.
Attachment #624637 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/2359a20631cc
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
No longer blocks: CVE-2012-3988
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: