Closed Bug 694204 Opened 8 years ago Closed 8 years ago

ESC to exit DOM full-screen stops all loads in doc

Categories

(Core :: Layout, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla10

People

(Reporter: cpearce, Assigned: cpearce)

References

Details

Attachments

(1 file, 1 obsolete file)

We're going to use the ESC key to exit DOM full-screen mode (which is different to F11 browser full-screen mode).

Currently the browser's ESC keyhandler [1] calls BrowserStop() [2] when ESC is pressed, which causes all loads in the document's load group to be cancelled. This means any video which you were watching in the page has its load cancelled, and will display an error cross over it.

We should not cancel loads in the document when we exit full-screen mode on an escape key press. It breaks video, which is a primary use case for the full-screen API.

[1] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser-sets.inc#328
[2] http://mxr.mozilla.org/mozilla-central/source/browser/base/content/browser.js#2035
Does the code consuming ESC for exiting DOM full-screen mode call preventDefault and stopPropagation? Would doing so fix this bug?
Thanks for the tip Dão.

This patch prevents default behaviour so that pressing ESC to exit DOM full-screen mode doesn't cause in progress loads in <video> from being cancelled.
Assignee: nobody → chris
Attachment #566953 - Flags: review?(Olli.Pettay)
(In reply to Chris Pearce (:cpearce) (Mozilla Corporation) from comment #0)
> We're going to use the ESC key to exit DOM full-screen mode (which is
> different to F11 browser full-screen mode).

That inconsistency sounds strange. I assume you've asked UX devs about this.
The security guys were adamant that ESC is for escaping. The user should be able to mash ESC and be sure they're no longer in full-screen mode. F11 also exits full-screen, but we advertise that ESC is the key to use.
Comment on attachment 566953 [details] [diff] [review]
Patch: prevent default on esc key press

So, web page does get esc key event, but in this one case its preventDefault() would return true. I'd prefer if the web page wouldn't see the event at all in
this case (since the event state would be a bit strange).

So, make it aEvent->flags |= (NS_EVENT_FLAG_NO_DEFAULT |  
                              NS_EVENT_FLAG_ONLY_CHROME_DISPATCH);

And this needs some test.
Should be probably enough to add something to existing fullscreen tests
(there are some, right) to check that web page doesn't get the esc.
Attachment #566953 - Flags: review?(Olli.Pettay) → review+
Attached patch Patch v2Splinter Review
* Add chrome-only dispatch flag.
* Add flags to all ESC key events (UP and PRESS as well), rather than just DOWN.
* Change the full-screen exit on restricted key event to exit full-screen on the key UP of a restricted key, rather than a key down. This is so that we're still in full-screen mode when we do the check which adds the flags (the runnable to exit full-screen can run before HandleEventInternal() is called for the ESC key UP). Note this code is going to go away, we're going to switch to showing the you're-in-full-screen-warning instead of exiting full-screen on restricted key press (bug 691583).
* Add test to ensure ESC key events are suppressed.
Attachment #566953 - Attachment is obsolete: true
Attachment #566967 - Flags: review+
Component: General → Layout
Product: Firefox → Core
QA Contact: general → layout
Blocks: 691583
Depends on: 701442
No longer depends on: 701442
You need to log in before you can comment on or make changes to this bug.