Should not be possible to focus something behind fullscreen element |
||||||
Issue descriptionChrome Version: 64.0.3275.0 OS: Mac (but applies to all Blink platforms) What steps will reproduce the problem? (1) Open attached file, fullscreen.html (2) Press the spacebar to enter/exit fullscreen mode (3) Press Tab and Shift+Tab to change focus. What is the expected result? In fullscreen mode, it shouldn't be possible to put input focus on a control that's behind the fullscreen element. Maybe the document body is okay. If focus lands on something like the omnibox it should pop up visually. What happens instead? It's possible to tab to controls outside of the fullscreen element. Keyboard-only users will be completely lost and confused when in fullscreen mode.
,
Nov 23 2017
Clues prided by dmazzoni@ over email: Check out Node::IsInert() as a starting point. It checks to see if there's an active modal dialog, and if so and the Node is not within its subtree, the node is inert. The first thing I'd try would be to update that to take fullscreen into account. Next look at HTMLDialogElement::showModal, I think you'll want to do something similar when entering fullscreen - probably call InertSubtreesChanged(), and maybe also something similar to SetFocusForDialog to place focus in the fullscreen subtree. Finally handle exiting fullscreen - something like HTMLDialogElement::close.
,
Nov 23 2017
The bots aren't green yet, but review is up: https://chromium-review.googlesource.com/c/chromium/src/+/788052
,
Nov 25 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6218872807f9d8f9e64630c7cefe0ab18954585a commit 6218872807f9d8f9e64630c7cefe0ab18954585a Author: Philip Jägenstedt <foolip@chromium.org> Date: Sat Nov 25 00:25:30 2017 Remove a bit of dead code in InertSubtreesChanged (for <dialog>) Because of how Document::ClearAXObjectCache() is implemented, Document::ExistingAXObjectCache() will always return nullptr immediately after, so the interesting code was unreachable. The code is from https://codereview.chromium.org/70213002. Bug: 787867 Change-Id: I08c8eed89f92749f7f12162e9d41ee17b4d172c9 Reviewed-on: https://chromium-review.googlesource.com/787910 Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org> Commit-Queue: Dominic Mazzoni <dmazzoni@chromium.org> Cr-Commit-Position: refs/heads/master@{#519169} [modify] https://crrev.com/6218872807f9d8f9e64630c7cefe0ab18954585a/third_party/WebKit/Source/core/html/HTMLDialogElement.cpp
,
Nov 27 2017
Tested this issue on Windows 7, Mac 10.12.6 & Ubuntu 14.04 using latest canary-64.0.3278.0 as per the below steps: Mac Behavior: -------------- 1. Downloaded attached html file 2. Launched chrome 3. Open above html file 4. Press tab multiple times and observe the focus is moved from one element to other. 5. Press spacebar & observed fullscreen mode 5. Now press tab & observed the focus 6. When focus is on 'After fullscreen Second' , then press Tab (multiple times too-to observe the focus) 7. Observed focus is going to the controls which are behind the fullscreen element (new tab,bookmarks bar,extensions....etc) Working as intended on Windows 7 & ubuntu 14.04. Focus is not going to the controls which are behind the fullscreen elements. Please find the attached screencast( Mac & Windows) for reference. foolip@, Could you please take a look and confirm the fix and expected behavior. Thanks..!
,
Nov 27 2017
jmukthavaram@, I'm working on the fix in https://chromium-review.googlesource.com/c/chromium/src/+/788052
,
Nov 27 2017
Do we understand why the bug is not reproducible on Windows? Should we test this page on Android with a keyboard attached?
,
Nov 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f3535b851736a4163abee41073c753d7cdf714d5 commit f3535b851736a4163abee41073c753d7cdf714d5 Author: Philip Jägenstedt <foolip@chromium.org> Date: Thu Nov 30 18:46:52 2017 Make elements outside of the fullscreen element inert There is no spec for this, but an old open spec issue: https://github.com/whatwg/fullscreen/issues/15 Bug: 787867 Change-Id: Icde796405fca96e910480aef6f0d6be835f7a27a Reviewed-on: https://chromium-review.googlesource.com/788052 Commit-Queue: Philip Jägenstedt <foolip@chromium.org> Reviewed-by: Mounir Lamouri <mlamouri@chromium.org> Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org> Cr-Commit-Position: refs/heads/master@{#520620} [add] https://crrev.com/f3535b851736a4163abee41073c753d7cdf714d5/content/browser/accessibility/fullscreen_browsertest.cc [modify] https://crrev.com/f3535b851736a4163abee41073c753d7cdf714d5/content/test/BUILD.gn [add] https://crrev.com/f3535b851736a4163abee41073c753d7cdf714d5/content/test/data/accessibility/fullscreen/links.html [modify] https://crrev.com/f3535b851736a4163abee41073c753d7cdf714d5/third_party/WebKit/LayoutTests/fullscreen/full-screen-contentEditable-crash-expected.txt [add] https://crrev.com/f3535b851736a4163abee41073c753d7cdf714d5/third_party/WebKit/LayoutTests/fullscreen/keyboard-focus.html [modify] https://crrev.com/f3535b851736a4163abee41073c753d7cdf714d5/third_party/WebKit/Source/core/dom/Node.cpp [modify] https://crrev.com/f3535b851736a4163abee41073c753d7cdf714d5/third_party/WebKit/Source/core/fullscreen/Fullscreen.cpp
,
Nov 30 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/36ed6cc7e73169695409ff40d1d7e95ec3cb0c1b commit 36ed6cc7e73169695409ff40d1d7e95ec3cb0c1b Author: Philip Jägenstedt <foolip@chromium.org> Date: Thu Nov 30 20:17:21 2017 Revert "Make elements outside of the fullscreen element inert" This reverts commit f3535b851736a4163abee41073c753d7cdf714d5. Reason for revert: AccessibilityFullscreenBrowserTest.IgnoreElementsOutsideFullscreenElement is flaky on linux-chromeos-rel Original change's description: > Make elements outside of the fullscreen element inert > > There is no spec for this, but an old open spec issue: > https://github.com/whatwg/fullscreen/issues/15 > > Bug: 787867 > Change-Id: Icde796405fca96e910480aef6f0d6be835f7a27a > Reviewed-on: https://chromium-review.googlesource.com/788052 > Commit-Queue: Philip Jägenstedt <foolip@chromium.org> > Reviewed-by: Mounir Lamouri <mlamouri@chromium.org> > Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org> > Cr-Commit-Position: refs/heads/master@{#520620} TBR=dmazzoni@chromium.org,mlamouri@chromium.org,foolip@chromium.org Change-Id: I5e042244565404b7735f4e5fecc4d1194ed2a402 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 787867 Reviewed-on: https://chromium-review.googlesource.com/801517 Reviewed-by: Philip Jägenstedt <foolip@chromium.org> Commit-Queue: Philip Jägenstedt <foolip@chromium.org> Cr-Commit-Position: refs/heads/master@{#520661} [delete] https://crrev.com/412f82011c88b2467b929b8e1b1dae0e7098138f/content/browser/accessibility/fullscreen_browsertest.cc [modify] https://crrev.com/36ed6cc7e73169695409ff40d1d7e95ec3cb0c1b/content/test/BUILD.gn [delete] https://crrev.com/412f82011c88b2467b929b8e1b1dae0e7098138f/content/test/data/accessibility/fullscreen/links.html [modify] https://crrev.com/36ed6cc7e73169695409ff40d1d7e95ec3cb0c1b/third_party/WebKit/LayoutTests/fullscreen/full-screen-contentEditable-crash-expected.txt [delete] https://crrev.com/412f82011c88b2467b929b8e1b1dae0e7098138f/third_party/WebKit/LayoutTests/fullscreen/keyboard-focus.html [modify] https://crrev.com/36ed6cc7e73169695409ff40d1d7e95ec3cb0c1b/third_party/WebKit/Source/core/dom/Node.cpp [modify] https://crrev.com/36ed6cc7e73169695409ff40d1d7e95ec3cb0c1b/third_party/WebKit/Source/core/fullscreen/Fullscreen.cpp
,
Nov 30 2017
I won't look at this more today, but if anyone has any ideas about why this might be flaky on linux-chromeos-rel I'd love to hear them, and if just disabling the test on OS_CHROMEOS would be too terrible to consider. (It seems quite unlikely to me that the source of the flakiness is something related to the changes themselves.)
,
Nov 30 2017
If we want to make M64, I would look at the test later. If we think M64 is no longer an option, maybe taking care of the test before landing is a better solution.
,
Dec 8 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/17dd72fb55aaa071bf1077a02e45b9f2f44d1ed2 commit 17dd72fb55aaa071bf1077a02e45b9f2f44d1ed2 Author: Philip Jägenstedt <foolip@chromium.org> Date: Fri Dec 08 13:13:49 2017 Reland "Make elements outside of the fullscreen element inert" This is a reland of f3535b851736a4163abee41073c753d7cdf714d5 The IgnoreElementsOutsideFullscreenElement test was originally flaky, due to changing textContent not reliably causing the accessibility tree to be updated. See https://crbug.com/793078 . Original change's description: > Make elements outside of the fullscreen element inert > > There is no spec for this, but an old open spec issue: > https://github.com/whatwg/fullscreen/issues/15 > > Bug: 787867 > Change-Id: Icde796405fca96e910480aef6f0d6be835f7a27a > Reviewed-on: https://chromium-review.googlesource.com/788052 > Commit-Queue: Philip Jägenstedt <foolip@chromium.org> > Reviewed-by: Mounir Lamouri <mlamouri@chromium.org> > Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org> > Cr-Commit-Position: refs/heads/master@{#520620} Bug: 787867 , 793078 Change-Id: I5ddc56a8783dce77acf02728afc9ed621145e222 Reviewed-on: https://chromium-review.googlesource.com/810584 Commit-Queue: Philip Jägenstedt <foolip@chromium.org> Reviewed-by: Mounir Lamouri <mlamouri@chromium.org> Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org> Cr-Commit-Position: refs/heads/master@{#522771} [add] https://crrev.com/17dd72fb55aaa071bf1077a02e45b9f2f44d1ed2/content/browser/accessibility/fullscreen_browsertest.cc [modify] https://crrev.com/17dd72fb55aaa071bf1077a02e45b9f2f44d1ed2/content/test/BUILD.gn [add] https://crrev.com/17dd72fb55aaa071bf1077a02e45b9f2f44d1ed2/content/test/data/accessibility/fullscreen/links.html [modify] https://crrev.com/17dd72fb55aaa071bf1077a02e45b9f2f44d1ed2/third_party/WebKit/LayoutTests/fullscreen/full-screen-contentEditable-crash-expected.txt [add] https://crrev.com/17dd72fb55aaa071bf1077a02e45b9f2f44d1ed2/third_party/WebKit/LayoutTests/fullscreen/keyboard-focus.html [modify] https://crrev.com/17dd72fb55aaa071bf1077a02e45b9f2f44d1ed2/third_party/WebKit/Source/core/dom/Node.cpp [modify] https://crrev.com/17dd72fb55aaa071bf1077a02e45b9f2f44d1ed2/third_party/WebKit/Source/core/fullscreen/Fullscreen.cpp
,
Dec 8 2017
,
Dec 14 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b078ac342807ec59b7b994d8e59145d698337e72 commit b078ac342807ec59b7b994d8e59145d698337e72 Author: Philip Jägenstedt <foolip@chromium.org> Date: Thu Dec 14 10:13:04 2017 Reland "Make elements outside of the fullscreen element inert" This is a reland of f3535b851736a4163abee41073c753d7cdf714d5 The IgnoreElementsOutsideFullscreenElement test was originally flaky, due to changing textContent not reliably causing the accessibility tree to be updated. See https://crbug.com/793078 . Original change's description: > Make elements outside of the fullscreen element inert > > There is no spec for this, but an old open spec issue: > https://github.com/whatwg/fullscreen/issues/15 > > Bug: 787867 > Change-Id: Icde796405fca96e910480aef6f0d6be835f7a27a > Reviewed-on: https://chromium-review.googlesource.com/788052 > Commit-Queue: Philip Jägenstedt <foolip@chromium.org> > Reviewed-by: Mounir Lamouri <mlamouri@chromium.org> > Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org> > Cr-Commit-Position: refs/heads/master@{#520620} Bug: 736832 , 787867 , 793078 Change-Id: I5ddc56a8783dce77acf02728afc9ed621145e222 Reviewed-on: https://chromium-review.googlesource.com/810584 Commit-Queue: Philip Jägenstedt <foolip@chromium.org> Reviewed-by: Mounir Lamouri <mlamouri@chromium.org> Reviewed-by: Dominic Mazzoni <dmazzoni@chromium.org> Cr-Original-Commit-Position: refs/heads/master@{#522771}(cherry picked from commit 17dd72fb55aaa071bf1077a02e45b9f2f44d1ed2) Reviewed-on: https://chromium-review.googlesource.com/826943 Reviewed-by: Philip Jägenstedt <foolip@chromium.org> Cr-Commit-Position: refs/branch-heads/3282@{#223} Cr-Branched-From: 5fdc0fab22ce7efd32532ee989b223fa12f8171e-refs/heads/master@{#520840} [add] https://crrev.com/b078ac342807ec59b7b994d8e59145d698337e72/content/browser/accessibility/fullscreen_browsertest.cc [modify] https://crrev.com/b078ac342807ec59b7b994d8e59145d698337e72/content/test/BUILD.gn [add] https://crrev.com/b078ac342807ec59b7b994d8e59145d698337e72/content/test/data/accessibility/fullscreen/links.html [modify] https://crrev.com/b078ac342807ec59b7b994d8e59145d698337e72/third_party/WebKit/LayoutTests/fullscreen/full-screen-contentEditable-crash-expected.txt [add] https://crrev.com/b078ac342807ec59b7b994d8e59145d698337e72/third_party/WebKit/LayoutTests/fullscreen/keyboard-focus.html [modify] https://crrev.com/b078ac342807ec59b7b994d8e59145d698337e72/third_party/WebKit/Source/core/dom/Node.cpp [modify] https://crrev.com/b078ac342807ec59b7b994d8e59145d698337e72/third_party/WebKit/Source/core/fullscreen/Fullscreen.cpp
,
Aug 27
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by dmazz...@chromium.org
, Nov 22 20172.7 KB
2.7 KB View Download