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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 1

[Dão Gottwald [:dao]]

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

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: 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.

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

(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

[Chris Pearce [:cpearce (Not reading bugmail)]]

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 5

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: 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.

Comment 7

[Chris Pearce [:cpearce (Not reading bugmail)]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ddad75e027ab

Comment 8

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

https://hg.mozilla.org/mozilla-central/rev/ddad75e027ab