From 17dd72fb55aaa071bf1077a02e45b9f2f44d1ed2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philip=20J=C3=A4genstedt?= Date: Fri, 8 Dec 2017 13:13:49 +0000 Subject: [PATCH] Reland "Make elements outside of the fullscreen element inert" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 > Reviewed-by: Mounir Lamouri > Reviewed-by: Dominic Mazzoni > 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 Reviewed-by: Mounir Lamouri Reviewed-by: Dominic Mazzoni Cr-Commit-Position: refs/heads/master@{#522771} --- .../accessibility/fullscreen_browsertest.cc | 108 ++++++++++++++++++ content/test/BUILD.gn | 5 +- .../data/accessibility/fullscreen/links.html | 19 +++ ...-screen-contentEditable-crash-expected.txt | 1 - .../fullscreen/keyboard-focus.html | 56 +++++++++ third_party/WebKit/Source/core/dom/Node.cpp | 18 ++- .../Source/core/fullscreen/Fullscreen.cpp | 5 + 7 files changed, 206 insertions(+), 6 deletions(-) create mode 100644 content/browser/accessibility/fullscreen_browsertest.cc create mode 100644 content/test/data/accessibility/fullscreen/links.html create mode 100644 third_party/WebKit/LayoutTests/fullscreen/keyboard-focus.html diff --git a/content/browser/accessibility/fullscreen_browsertest.cc b/content/browser/accessibility/fullscreen_browsertest.cc new file mode 100644 index 0000000000000..a93f137234f19 --- /dev/null +++ b/content/browser/accessibility/fullscreen_browsertest.cc @@ -0,0 +1,108 @@ +// Copyright 2017 The Chromium Authors. All rights reserved. +// Use of this source code is governed by a BSD-style license that can be +// found in the LICENSE file. + +#include "base/logging.h" +#include "content/browser/accessibility/browser_accessibility.h" +#include "content/browser/accessibility/browser_accessibility_manager.h" +#include "content/browser/web_contents/web_contents_impl.h" +#include "content/public/test/browser_test_utils.h" +#include "content/public/test/content_browser_test.h" +#include "content/public/test/content_browser_test_utils.h" +#include "content/shell/browser/shell.h" +#include "content/test/accessibility_browser_test_utils.h" +#include "testing/gtest/include/gtest/gtest.h" + +namespace content { + +class AccessibilityFullscreenBrowserTest : public ContentBrowserTest { + public: + AccessibilityFullscreenBrowserTest() = default; + ~AccessibilityFullscreenBrowserTest() override = default; + + protected: + BrowserAccessibility* FindButton(BrowserAccessibility* node) { + if (node->GetRole() == ui::AX_ROLE_BUTTON) + return node; + for (unsigned i = 0; i < node->PlatformChildCount(); i++) { + if (BrowserAccessibility* button = FindButton(node->PlatformGetChild(i))) + return button; + } + return nullptr; + } + + int CountLinks(BrowserAccessibility* node) { + if (node->GetRole() == ui::AX_ROLE_LINK) + return 1; + int links_in_children = 0; + for (unsigned i = 0; i < node->PlatformChildCount(); i++) { + links_in_children += CountLinks(node->PlatformGetChild(i)); + } + return links_in_children; + } +}; + +namespace { + +// FakeFullscreenDelegate simply stores the latest requested mod and reports it +// back, which is all that is required for the renderer to enter fullscreen. +class FakeFullscreenDelegate : public WebContentsDelegate { + public: + FakeFullscreenDelegate() = default; + ~FakeFullscreenDelegate() override = default; + + void EnterFullscreenModeForTab(WebContents*, const GURL&) override { + is_fullscreen_ = true; + } + + void ExitFullscreenModeForTab(WebContents*) override { + is_fullscreen_ = false; + } + + bool IsFullscreenForTabOrPending(const WebContents*) const override { + return is_fullscreen_; + } + + private: + bool is_fullscreen_ = false; + DISALLOW_COPY_AND_ASSIGN(FakeFullscreenDelegate); +}; + +} // namespace + +IN_PROC_BROWSER_TEST_F(AccessibilityFullscreenBrowserTest, + IgnoreElementsOutsideFullscreenElement) { + ASSERT_TRUE(embedded_test_server()->Start()); + + FakeFullscreenDelegate delegate; + shell()->web_contents()->SetDelegate(&delegate); + + AccessibilityNotificationWaiter waiter( + shell()->web_contents(), ui::kAXModeComplete, ui::AX_EVENT_LOAD_COMPLETE); + GURL url( + embedded_test_server()->GetURL("/accessibility/fullscreen/links.html")); + NavigateToURL(shell(), url); + waiter.WaitForNotification(); + + WebContentsImpl* web_contents = + static_cast(shell()->web_contents()); + BrowserAccessibilityManager* manager = + web_contents->GetRootBrowserAccessibilityManager(); + + // Initially there are 3 links in the accessiblity tree. + EXPECT_EQ(3, CountLinks(manager->GetRoot())); + + // Enter fullscreen by finding the button and performing the default action, + // which is to click it. + BrowserAccessibility* button = FindButton(manager->GetRoot()); + ASSERT_NE(nullptr, button); + manager->DoDefaultAction(*button); + + // Upon entering fullscreen, the page will change the button text to "Done". + WaitForAccessibilityTreeToContainNodeWithName(web_contents, "Done"); + + // Now, the two links outside of the fullscreen element are gone. + EXPECT_EQ(1, CountLinks(manager->GetRoot())); +} + +} // namespace content diff --git a/content/test/BUILD.gn b/content/test/BUILD.gn index cb6e92e6072df..c1b6081950132 100644 --- a/content/test/BUILD.gn +++ b/content/test/BUILD.gn @@ -676,6 +676,7 @@ test("content_browsertests") { "../browser/accessibility/dump_accessibility_browsertest_base.h", "../browser/accessibility/dump_accessibility_events_browsertest.cc", "../browser/accessibility/dump_accessibility_tree_browsertest.cc", + "../browser/accessibility/fullscreen_browsertest.cc", "../browser/accessibility/hit_testing_browsertest.cc", "../browser/accessibility/site_per_process_accessibility_browsertest.cc", "../browser/accessibility/snapshot_ax_tree_browsertest.cc", @@ -1888,7 +1889,9 @@ test("content_unittests") { # Screen capture unit tests. if (is_linux || is_mac || is_win) { deps += [ "//third_party/libyuv" ] - sources += [ "../browser/media/capture/web_contents_video_capture_device_unittest.cc" ] + sources += [ + "../browser/media/capture/web_contents_video_capture_device_unittest.cc", + ] if (use_aura) { sources += [ "../browser/media/capture/cursor_renderer_aura_unittest.cc" ] } diff --git a/content/test/data/accessibility/fullscreen/links.html b/content/test/data/accessibility/fullscreen/links.html new file mode 100644 index 0000000000000..ca318b7b2480c --- /dev/null +++ b/content/test/data/accessibility/fullscreen/links.html @@ -0,0 +1,19 @@ + +link before target +
+ + link inside target +
+link after target + diff --git a/third_party/WebKit/LayoutTests/fullscreen/full-screen-contentEditable-crash-expected.txt b/third_party/WebKit/LayoutTests/fullscreen/full-screen-contentEditable-crash-expected.txt index 2799b65139a67..a2f48eb445fd3 100644 --- a/third_party/WebKit/LayoutTests/fullscreen/full-screen-contentEditable-crash-expected.txt +++ b/third_party/WebKit/LayoutTests/fullscreen/full-screen-contentEditable-crash-expected.txt @@ -1,3 +1,2 @@ -CONSOLE WARNING: line 22: The behavior that Selection.addRange() merges existing Range and the specified Range was removed. See https://www.chromestatus.com/features/6680566019653632 for more details. Pass if there is no crash. Click anywhere to test manually. diff --git a/third_party/WebKit/LayoutTests/fullscreen/keyboard-focus.html b/third_party/WebKit/LayoutTests/fullscreen/keyboard-focus.html new file mode 100644 index 0000000000000..ea5fe64d289f1 --- /dev/null +++ b/third_party/WebKit/LayoutTests/fullscreen/keyboard-focus.html @@ -0,0 +1,56 @@ + +Elements not contained by the fullscreen element are inert + + + + +
+
+ +
+
+ + +
+
+ +
+ diff --git a/third_party/WebKit/Source/core/dom/Node.cpp b/third_party/WebKit/Source/core/dom/Node.cpp index 5ea9e04b15522..8c98836bddf2d 100644 --- a/third_party/WebKit/Source/core/dom/Node.cpp +++ b/third_party/WebKit/Source/core/dom/Node.cpp @@ -82,6 +82,7 @@ #include "core/frame/LocalFrameClient.h" #include "core/frame/LocalFrameView.h" #include "core/frame/UseCounter.h" +#include "core/fullscreen/Fullscreen.h" #include "core/html/HTMLDialogElement.h" #include "core/html/HTMLFrameOwnerElement.h" #include "core/html/HTMLSlotElement.h" @@ -914,10 +915,19 @@ bool Node::IsInert() const { DCHECK(!ChildNeedsDistributionRecalc()); - const HTMLDialogElement* dialog = GetDocument().ActiveModalDialog(); - if (dialog && this != GetDocument() && - !FlatTreeTraversal::ContainsIncludingPseudoElement(*dialog, *this)) { - return true; + if (this != GetDocument()) { + // TODO(foolip): When fullscreen uses top layer, this can be simplified to + // just look at the topmost element in top layer. https://crbug.com/240576. + // Note: It's currently appropriate that a modal dialog element takes + // precedence over a fullscreen element, because it will be rendered on top, + // but with fullscreen merged into top layer that will no longer be true. + const Element* modal_element = GetDocument().ActiveModalDialog(); + if (!modal_element) + modal_element = Fullscreen::FullscreenElementFrom(GetDocument()); + if (modal_element && !FlatTreeTraversal::ContainsIncludingPseudoElement( + *modal_element, *this)) { + return true; + } } if (RuntimeEnabledFeatures::InertAttributeEnabled()) { diff --git a/third_party/WebKit/Source/core/fullscreen/Fullscreen.cpp b/third_party/WebKit/Source/core/fullscreen/Fullscreen.cpp index 41340537efe2d..a2767e34eed53 100644 --- a/third_party/WebKit/Source/core/fullscreen/Fullscreen.cpp +++ b/third_party/WebKit/Source/core/fullscreen/Fullscreen.cpp @@ -903,6 +903,11 @@ void Fullscreen::FullscreenElementChanged(Element* old_element, // TODO(foolip): This should not call |UpdateStyleAndLayoutTree()|. GetDocument()->UpdateStyleAndLayoutTree(); + + // Any element not contained by the fullscreen element is inert (see + // |Node::IsInert()|), so changing the fullscreen element will typically + // change the inertness of most elements. Clear the entire cache. + GetDocument()->ClearAXObjectCache(); } void Fullscreen::Trace(blink::Visitor* visitor) {