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

RESOLVED FIXED in mozilla10



6 years ago
6 years ago


(Reporter: cpearce, Assigned: cpearce)


Dependency tree / graph

Firefox Tracking Flags

(Not tracked)



(1 attachment, 1 obsolete attachment)



6 years ago
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.

Does the code consuming ESC for exiting DOM full-screen mode call preventDefault and stopPropagation? Would doing so fix this bug?

Comment 2

6 years ago
Created attachment 566953 [details] [diff] [review]
Patch: prevent default on esc key press

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.

Comment 4

6 years ago
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 |  

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+

Comment 6

6 years ago
Created attachment 566967 [details] [diff] [review]
Patch v2

* 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+


6 years ago
Component: General → Layout
Product: Firefox → Core
QA Contact: general → layout


6 years ago
Blocks: 691583

Comment 7

6 years ago
Target Milestone: --- → mozilla10
Last Resolved: 6 years ago
Resolution: --- → FIXED


6 years ago
Depends on: 701442


6 years ago
No longer depends on: 701442
You need to log in before you can comment on or make changes to this bug.