Last Comment Bug 694204 - ESC to exit DOM full-screen stops all loads in doc
: ESC to exit DOM full-screen stops all loads in doc
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla10
Assigned To: Chris Pearce (:cpearce)
:
:
Mentors:
Depends on:
Blocks: 545812 691583
  Show dependency treegraph
 
Reported: 2011-10-12 17:07 PDT by Chris Pearce (:cpearce)
Modified: 2011-11-10 14:32 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch: prevent default on esc key press (1.34 KB, patch)
2011-10-13 15:10 PDT, Chris Pearce (:cpearce)
bugs: review+
Details | Diff | Splinter Review
Patch v2 (4.82 KB, patch)
2011-10-13 16:19 PDT, Chris Pearce (:cpearce)
cpearce: review+
Details | Diff | Splinter Review

Description Chris Pearce (:cpearce) 2011-10-12 17:07:53 PDT
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] 2011-10-13 00:47:55 PDT
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) 2011-10-13 15:10:16 PDT
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.
Comment 3 Olli Pettay [:smaug] 2011-10-13 15:13:46 PDT
(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) 2011-10-13 15:23:08 PDT
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] 2011-10-13 15:30:10 PDT
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) 2011-10-13 16:19:00 PDT
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.
Comment 8 Kyle Huey [:khuey] (Exited; not receiving bugmail, email if necessary) 2011-11-01 07:43:14 PDT
https://hg.mozilla.org/mozilla-central/rev/ddad75e027ab

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