Dispatch event when restricted key input occurs in DOM full-screen mode

RESOLVED FIXED in mozilla10

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: cpearce, Assigned: cpearce)

Tracking

(Depends on: 1 bug, {dev-doc-needed})

Trunk
mozilla10
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

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

Comment 1

6 years ago
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.
Assignee: nobody → chris
Status: NEW → ASSIGNED
Attachment #568234 - Flags: review?(Olli.Pettay)
(Assignee)

Updated

6 years ago
Blocks: 684625
No longer depends on: 684625
(Assignee)

Comment 2

6 years ago
Patch v1 is based on top of the patch in bug 694204.
Depends on: 694204

Comment 3

6 years ago
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?
Attachment #568234 - Flags: review?(Olli.Pettay) → review-
(Assignee)

Comment 4

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

Using nsPLDOMEvent and with chrome mochitest of the "MozShowFullScreenWarning" event.
Attachment #568234 - Attachment is obsolete: true
Attachment #568925 - Flags: review?(Olli.Pettay)

Comment 5

6 years ago
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)
Attachment #568925 - Flags: review?(Olli.Pettay) → review+
(Assignee)

Comment 6

6 years ago
Created attachment 569271 [details] [diff] [review]
Patch v3

Patch ready for landing.
Attachment #568925 - Attachment is obsolete: true
Attachment #569271 - Flags: review+
(Assignee)

Comment 7

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

Updated

6 years ago
Summary: Show warning message when restricted key input occurs in DOM full-screen mode → Dispatch event when restricted key input occurs in DOM full-screen mode
(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.
(Assignee)

Comment 9

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

Comment 10

6 years ago
Patch v3: https://hg.mozilla.org/integration/mozilla-inbound/rev/e269b5ec522a
Standardize on Unix EOL: https://hg.mozilla.org/integration/mozilla-inbound/rev/ad6054dd6e08
Target Milestone: --- → mozilla10
https://hg.mozilla.org/mozilla-central/rev/e269b5ec522a
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Also https://hg.mozilla.org/mozilla-central/rev/ad6054dd6e08

Updated

6 years ago
Keywords: dev-doc-needed
In order to write the associated documentation, I have a small question: is this event available to extension authors?
Depends on: 729830
You need to log in before you can comment on or make changes to this bug.