Last Comment Bug 691583 - Dispatch event when restricted key input occurs in DOM full-screen mode
: Dispatch event when restricted key input occurs in DOM full-screen mode
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla10
Assigned To: Chris Pearce (:cpearce)
:
Mentors:
Depends on: 729830 694204
Blocks: 545812 684625
  Show dependency treegraph
 
Reported: 2011-10-03 16:37 PDT by Chris Pearce (:cpearce)
Modified: 2012-02-22 21:08 PST (History)
6 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch: dispatch showfullscreenwarning chrome event when restricted key pressed in full-screen mode (13.24 KB, patch)
2011-10-19 15:54 PDT, Chris Pearce (:cpearce)
bugs: review-
Details | Diff | Splinter Review
Patch v2 (26.14 KB, patch)
2011-10-22 21:06 PDT, Chris Pearce (:cpearce)
bugs: review+
Details | Diff | Splinter Review
Patch v3 (26.08 KB, patch)
2011-10-24 20:12 PDT, Chris Pearce (:cpearce)
cpearce: review+
Details | Diff | Splinter Review
Patch: change EOL to unix (16.72 KB, patch)
2011-10-24 20:14 PDT, Chris Pearce (:cpearce)
no flags Details | Diff | Splinter Review

Description Chris Pearce (:cpearce) 2011-10-03 16:37:47 PDT
Change DOM full-screen mode to instead show the "Press ESC to leave full-screen" warning message when restricted key input occurs, rather than exiting full-screen.
Comment 1 Chris Pearce (:cpearce) 2011-10-19 15:54:45 PDT
Created attachment 568234 [details] [diff] [review]
Patch: dispatch showfullscreenwarning chrome event when restricted key pressed in full-screen mode

The secteam decided it was sufficient to just display a warning saying how to exit full-screen when a restricted key was pressed, rather than exit full-screen mode on restricted key press. So this patch changes behaviour to dispatch "showfullscreenwarning" to chrome when a restricted key is pressed. My patch in bug 684625 will listen for this event and display the warning.

This patch also alters the test to reflect that most keys don't cause full-screen exit any more.
Comment 2 Chris Pearce (:cpearce) 2011-10-19 16:06:36 PDT
Patch v1 is based on top of the patch in bug 694204.
Comment 3 Olli Pettay [:smaug] 2011-10-20 05:54:01 PDT
Comment on attachment 568234 [details] [diff] [review]
Patch: dispatch showfullscreenwarning chrome event when restricted key pressed in full-screen mode


>+        } else if (IsFullScreenAndRestrictedKeyEvent(mCurrentEventContent, aEvent)) {
>+          // Restricted key press while in DOM full-screen mode. Dispatch
>+          // an event to chrome so it knows to show a warning message
>+          // informing the user how to exit full-screen.
>+          bool defaultActionEnabled = true;
>+          nsPIDOMWindow* outer = doc->GetWindow()->GetOuterWindow();
doc->GetWindow() is the outer window.
But actually, why is window the target of the event and not document.


>+          nsContentUtils::DispatchChromeEvent(doc,
>+                                              outer,
>+                                              NS_ConvertASCIItoUTF16("showfullscreenwarning"),
NS_LITERAL_STRING

>+                                              PR_TRUE, PR_TRUE, &defaultActionEnabled);
The last parameter is optional.


Could you call the event something like MozShowFullScreenWarning or something.
It is after all very mozilla specific.

Also, do we want to dispatch that even synchronously or would async work.
For async
nsRefPtr<nsPLDOMEvent> e = new nsPLDOMEvent(doc, NS_LITERAL_STRING("MozShowFullScreenWarning"), true, true);
e->PostDOMEvent();


I think I'd prefer async event if there isn't need for sync.


I assume there will be tests for the new event in some other bug?
Comment 4 Chris Pearce (:cpearce) 2011-10-22 21:06:15 PDT
Created attachment 568925 [details] [diff] [review]
Patch v2

Using nsPLDOMEvent and with chrome mochitest of the "MozShowFullScreenWarning" event.
Comment 5 Olli Pettay [:smaug] 2011-10-23 04:40:53 PDT
Comment on attachment 568925 [details] [diff] [review]
Patch v2

>+         nsIDocument *doc = mCurrentEventContent ?
>+                            mCurrentEventContent->OwnerDoc() : nsnull;
>+         if (doc &&
>+            doc->IsFullScreenDoc() &&
Missing space before doc->

Please make sure there are no windows line endings in the patch.
(Some of the fullscreen patches did have)
Comment 6 Chris Pearce (:cpearce) 2011-10-24 20:12:51 PDT
Created attachment 569271 [details] [diff] [review]
Patch v3

Patch ready for landing.
Comment 7 Chris Pearce (:cpearce) 2011-10-24 20:14:59 PDT
Created attachment 569272 [details] [diff] [review]
Patch: change EOL to unix

A few of the new test files I created had windows line endings, or even a mixture. This patch normalizes all the tests to Unix line endings.
Comment 8 Dão Gottwald [:dao] 2011-10-28 02:12:27 PDT
(In reply to Chris Pearce (:cpearce) (Mozilla Corporation) from comment #0)
> Change DOM full-screen mode to instead show the "Press ESC to leave
> full-screen" warning message when restricted key input occurs, rather than
> exiting full-screen.

Would we, without this patch, exit FS mode when hitting Ctrl? If so, does the patch enable shortcuts such as Ctrl+Tab? This would seem problematic to me.
Comment 9 Chris Pearce (:cpearce) 2011-10-28 02:25:49 PDT
Yes, this patch will enable ctrl+tab. I am planning to exit full-screen on change-tab (and alt+tab change-app) in bug 685402 to handle this. I haven't figured out the best way to detect change-tab and change-app yet, any ideas you have on how to implement this would be most welcome in bug 685402.
Comment 11 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-11-01 07:43:40 PDT
https://hg.mozilla.org/mozilla-central/rev/e269b5ec522a
Comment 12 Kyle Huey [:khuey] (khuey@mozilla.com) 2011-11-01 07:44:01 PDT
Also https://hg.mozilla.org/mozilla-central/rev/ad6054dd6e08
Comment 13 Jean-Yves Perrier [:teoli] 2011-11-21 23:50:17 PST
In order to write the associated documentation, I have a small question: is this event available to extension authors?

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