| Summary: | Keyboard focus should exit fullscreen. | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jeremy Jones <jeremyj-wk> | ||||||||||||||
| Component: | WebKit2 | Assignee: | Jeremy Jones <jeremyj-wk> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | benjamin, bfulgham, cdumez, cmarcelo, commit-queue, dbates, eric.carlson, esprehn+autocc, ews-watchlist, jer.noble, jonlee, kangil.han, rniwa, webkit-bug-importer | ||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Jeremy Jones
2018-05-14 10:47:32 PDT
Created attachment 340330 [details]
Patch
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? (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 + Created attachment 341281 [details]
Patch
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 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 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 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 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
Created attachment 341332 [details]
fix sim experiment
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. Created attachment 341792 [details]
Patch
Comment on attachment 341792 [details] Patch Clearing flags on attachment: 341792 Committed r232427: <https://trac.webkit.org/changeset/232427> All reviewed patches have been landed. Closing bug. |