Bug 185617 - Keyboard focus should exit fullscreen.
Summary: Keyboard focus should exit fullscreen.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Jeremy Jones
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2018-05-14 10:47 PDT by Jeremy Jones
Modified: 2018-06-01 16:23 PDT (History)
14 users (show)

See Also:


Attachments
Patch (1.48 KB, patch)
2018-05-14 10:51 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (7.02 KB, patch)
2018-05-25 07:59 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews122 for ios-simulator-wk2 (deleted)
2018-05-25 09:48 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews206 for win-future (12.74 MB, application/zip)
2018-05-25 14:19 PDT, EWS Watchlist
no flags Details
fix sim experiment (7.48 KB, patch)
2018-05-25 15:13 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff
Patch (5.11 KB, patch)
2018-06-01 15:09 PDT, Jeremy Jones
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jeremy Jones 2018-05-14 10:47:32 PDT
Keyboard focus should exit fullscreen.
Comment 1 Jeremy Jones 2018-05-14 10:49:23 PDT
rdar://problem/34697938
Comment 2 Jeremy Jones 2018-05-14 10:51:38 PDT
Created attachment 340330 [details]
Patch
Comment 3 Ryosuke Niwa 2018-05-21 15:19:24 PDT
Comment on attachment 340330 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=340330&action=review

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:4913
> +        node->document().webkitCancelFullScreen();

Surely, this can be tested?
Also, do we really want to do this on both macOS and iOS?
Comment 4 Jeremy Jones 2018-05-25 07:10:48 PDT
(In reply to Ryosuke Niwa from comment #3)
> Comment on attachment 340330 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340330&action=review
> 
> > Source/WebKit/WebProcess/WebPage/WebPage.cpp:4913
> > +        node->document().webkitCancelFullScreen();
> 
> Surely, this can be tested?
> Also, do we really want to do this on both macOS and iOS?

Working on a test.

We only want to do this on iOS. 

 #if PLATFORM(IOS)
+
+#if ENABLE(FULLSCREEN_API)
+        node->document().webkitCancelFullScreen();
+#endif
+
Comment 5 Jeremy Jones 2018-05-25 07:59:45 PDT
Created attachment 341281 [details]
Patch
Comment 6 Eric Carlson 2018-05-25 09:40:43 PDT
Comment on attachment 341281 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341281&action=review

> LayoutTests/fullscreen/full-screen-exit-on-keyboard-focus.html:50
> +                setTimeout(function() {
> +                    consoleWrite("Attempt to focus on editable content.");
> +                    runOnMouseDown(function() {});
> +                }, 500);

I don't understand this - why run it after 500ms, and what does runOnMouseDown with an empty lambda do?

> LayoutTests/fullscreen/full-screen-exit-on-keyboard-focus.html:77
> +<body Xonmousedown="doMouseDown()">

Xonmousedown?
Comment 7 EWS Watchlist 2018-05-25 09:48:32 PDT
Comment on attachment 341281 [details]
Patch

Attachment 341281 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7800224

New failing tests:
fullscreen/full-screen-exit-on-keyboard-focus.html
Comment 8 EWS Watchlist 2018-05-25 09:48:34 PDT
Created attachment 341295 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4
Comment 9 EWS Watchlist 2018-05-25 14:19:09 PDT
Comment on attachment 341281 [details]
Patch

Attachment 341281 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7802859

New failing tests:
http/tests/security/canvas-remote-read-remote-video-localhost.html
Comment 10 EWS Watchlist 2018-05-25 14:19:20 PDT
Created attachment 341328 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
Comment 11 Jeremy Jones 2018-05-25 15:13:00 PDT
Created attachment 341332 [details]
fix sim experiment
Comment 12 Ryosuke Niwa 2018-05-29 17:41:10 PDT
Comment on attachment 341281 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=341281&action=review

> LayoutTests/fullscreen/full-screen-exit-on-keyboard-focus.html:47
> +                setTimeout(function() {

I think we should expose some boolean indicating the fullscreen transition had finished or not via internals,
and then we can poll that value here instead of 500ms. I'm afraid the test will be flaky in some slow bots otherwise.
Namely, set this Document flag true in Document::webkitDidEnterFullScreenForElement
and clear it in Document::webkitWillEnterFullScreenForElement and Document::webkitWillExitFullScreenForElement

>> LayoutTests/fullscreen/full-screen-exit-on-keyboard-focus.html:77
>> +<body Xonmousedown="doMouseDown()">
> 
> Xonmousedown?

Plase remove this as we discussed in person.
Comment 13 Jeremy Jones 2018-06-01 15:09:02 PDT
Created attachment 341792 [details]
Patch
Comment 14 WebKit Commit Bot 2018-06-01 16:23:03 PDT
Comment on attachment 341792 [details]
Patch

Clearing flags on attachment: 341792

Committed r232427: <https://trac.webkit.org/changeset/232427>
Comment 15 WebKit Commit Bot 2018-06-01 16:23:05 PDT
All reviewed patches have been landed.  Closing bug.