Commit 558f048b authored by Findit's avatar Findit

Revert "Reland "DL: Don't include skip-viewport-activation subtrees in the AX tree""

This reverts commit 0b25b17e.

Reason for revert:

Findit (https://goo.gl/kROfz5) identified CL at revision 742520 as the
culprit for flakes in the build cycles as shown on:
https://analysis.chromium.org/p/chromium/flake-portal/analysis/culprit?key=ag9zfmZpbmRpdC1mb3ItbWVyQwsSDEZsYWtlQ3VscHJpdCIxY2hyb21pdW0vMGIyNWIxN2VkYThlMWY0YjRiN2MxMDI0ZmRjZWQ2Mjg0Nzc5Y2RlYQw

Sample Failed Build: https://ci.chromium.org/b/8888021964038070960

Sample Failed Step: content_browsertests

Sample Flaky Test: All/DumpAccessibilityTreeTest.DisplayLockingViewportActivation/blink

Original change's description:
> Reland "DL: Don't include skip-viewport-activation subtrees in the AX tree"
> 
> This is a reland of c08274e9
> 
> The CL was previously reverted because the viewport activation test is
> flaky, probably due to uncertain timing of near-the-viewport activation.
> I've updated the test to only test for in-the-viewport activation,
> I'll investigate further on why it's flaky for near-the-viewport
> (manual testing shows activation happens consistently).
> 
> SHERIFFS: please don't revert this CL if it introduces a new flaky test.
> Instead, please disable them and I'll investigate.
> 
> Original change's description:
> > DL: Don't include skip-viewport-activation subtrees in the AX tree
> >
> > Now both render-subtree: skip-viewport-activation and skip-activation
> > subtrees are not in the AX subtree at all, as discussed in
> > https://github.com/WICG/display-locking/issues/102#issuecomment-564205445
> >
> > Now only render-subtree:invisible subtrees are in the AX tree, and they
> > are automatically marked as offscreen because the nodes are activated
> > when they are on screen.
> >
> > Bug: 1001930
> > Change-Id: I1de511672f60078a02b3ddf3d989f2da1c44ae9d
> > Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2028883
> > Reviewed-by: Alice Boxhall <aboxhall@chromium.org>
> > Reviewed-by: vmpstr <vmpstr@chromium.org>
> > Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
> > Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#740531}
> 
> Bug: 1001930
> Change-Id: Icdf43cff86ade7e7e8f2aee76933dfc51e3f022c
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2054649
> Reviewed-by: vmpstr <vmpstr@chromium.org>
> Reviewed-by: Alice Boxhall <aboxhall@chromium.org>
> Commit-Queue: Rakina Zata Amni <rakina@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#742520}


Change-Id: I9258a82fa5a36921963dda0d12ce6efde1895e4b
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 1001930
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2063667
Cr-Commit-Position: refs/heads/master@{#742651}
parent 39d39b0b
......@@ -2272,17 +2272,22 @@ IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest, DeleteSelectionCrash) {
RunHtmlTest(FILE_PATH_LITERAL("delete-selection-crash.html"));
}
IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest, DisplayLockingActivatable) {
RunDisplayLockingTest(FILE_PATH_LITERAL("activatable.html"));
}
IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest,
DisplayLockingNonActivatable) {
RunDisplayLockingTest(FILE_PATH_LITERAL("non-activatable.html"));
DisplayLockingActivatableActivated) {
RunDisplayLockingTest(FILE_PATH_LITERAL("activatable-activated.html"));
}
IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest,
DisplayLockingViewportActivation) {
RunDisplayLockingTest(FILE_PATH_LITERAL("viewport-activation.html"));
DisplayLockingNonActivatable) {
RunDisplayLockingTest(FILE_PATH_LITERAL("non-activatable.html"));
}
IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest, DisplayLockingAll) {
// crbug.com/1043480: disabled due to flakiness.
IN_PROC_BROWSER_TEST_P(DumpAccessibilityTreeTest, DISABLED_DisplayLockingAll) {
RunDisplayLockingTest(FILE_PATH_LITERAL("all.html"));
}
......
rootWebArea
++genericContainer ignored
++++genericContainer ignored
++++++genericContainer
++++++++genericContainer
++++++++++staticText name='child'
++++++++++++inlineTextBox name='child'
++++++++genericContainer
++++++++++staticText name='nested locked element!'
++++++++genericContainer
<!--
@BLINK-ALLOW:offscreen
-->
<div>
<div id="locked" rendersubtree="invisible skip-viewport-activation">
<div>child</div>
<div id="nested" rendersubtree="invisible skip-viewport-activation">nested locked element!</div>
<div id="nonActivatable" rendersubtree="invisible skip-activation">nested non activatable locked element</div>
</div>
</div>
<script>
// Force layout, then activate.
locked.getBoundingClientRect();
locked.scrollIntoView();
</script>
rootWebArea
++genericContainer ignored
++++genericContainer ignored
++++++genericContainer
++++++++staticText name='<newline> '
++++++++genericContainer
++++++++++staticText name='child'
++++++++staticText name='<newline> '
++++++++genericContainer
++++++++++staticText name='nested locked element!'
++++++++staticText name='<newline> '
++++++++genericContainer
++++++++staticText name='<newline> '
<!--
@BLINK-ALLOW:offscreen
-->
<div>
<div id="locked" rendersubtree="invisible skip-viewport-activation">
<div>child</div>
<div id="nested" rendersubtree="invisible skip-viewport-activation">nested locked element!</div>
<div id="nonActivatable" rendersubtree="invisible skip-activation">nested non activatable locked element</div>
</div>
</div>
......@@ -2,31 +2,19 @@ rootWebArea
++genericContainer ignored
++++genericContainer ignored
++++++genericContainer
++++++++staticText name='spacer so that everything below will be offscreen (and won't get viewport-activated)'
++++++++++inlineTextBox name='spacer so that everything below will be offscreen (and won't get viewport-activated)'
++++++genericContainer offscreen
++++++++genericContainer offscreen
++++++++++staticText offscreen name='child text will be in AX tree but without layout'
++++++++++++inlineTextBox offscreen name='child text will be in AX tree but without layout'
++++++++genericContainer offscreen
++++++++++staticText offscreen name='nested activatable locked element will be in AX tree but without layout'
++++++++++++inlineTextBox offscreen name='nested activatable locked element will be in AX tree but without layout'
++++++++staticText offscreen name='normal text 1'
++++++++++inlineTextBox offscreen name='normal text 1'
++++++++genericContainer offscreen
++++++++++staticText offscreen name='nested non-viewport-activatable locked element will not be in AX tree'
++++++++++++inlineTextBox offscreen name='nested non-viewport-activatable locked element will not be in AX tree'
++++++++staticText offscreen name='normal text 2'
++++++++++inlineTextBox offscreen name='normal text 2'
++++++++genericContainer offscreen
++++++++++staticText offscreen name='nested non-activatable locked element will not be in AX tree'
++++++++++++inlineTextBox offscreen name='nested non-activatable locked element will not be in AX tree'
++++++++staticText offscreen name='normal text 3'
++++++++++inlineTextBox offscreen name='normal text 3'
++++++++presentational ignored offscreen
++++++++++staticText offscreen name='role=presentation item will be in AX tree'
++++++++++++inlineTextBox offscreen name='role=presentation item will be in AX tree'
++++++++genericContainer ignored invisible offscreen name='visibility:hidden text will be in AX tree'
++++++++genericContainer ignored invisible offscreen
++++++++++staticText ignored invisible offscreen name='aria-hidden text will be in AX tree'
++++++++genericContainer
++++++++++staticText name='child'
++++++++++++inlineTextBox name='child'
++++++++genericContainer
++++++++++staticText name='nested locked element!'
++++++++++++inlineTextBox name='nested locked element!'
++++++++genericContainer
++++++++++staticText name='nested non activatable locked element'
++++++++++++inlineTextBox name='nested non activatable locked element'
++++++++presentational ignored
++++++++++listItem ignored
++++++++++++staticText name='role=presentation item'
++++++++++++++inlineTextBox name='role=presentation item'
++++++++genericContainer ignored invisible name='visibility:hidden text'
++++++++genericContainer ignored invisible
++++++++++staticText ignored invisible name='aria-hidden text'
......@@ -2,29 +2,20 @@
@BLINK-ALLOW:offscreen
-->
<div>
<div style="height:10000px;">spacer so that everything below will be offscreen (and won't get viewport-activated)</div>
<div id="locked" rendersubtree="invisible">
<div>child text will be in AX tree but without layout</div>
<div id="nested" rendersubtree="invisible">
nested activatable locked element will be in AX tree but without layout
</div>
normal text 1
<div id="nonViewportActivatable" rendersubtree="invisible skip-viewport-activation">
nested non-viewport-activatable locked element will not be in AX tree
</div>
normal text 2
<div id="nonActivatable" rendersubtree="invisible skip-activation">
nested non-activatable locked element will not be in AX tree
</div>
normal text 3
<div id="locked" rendersubtree="invisible skip-viewport-activation">
<div>child</div>
<div id="nested" rendersubtree="invisible skip-viewport-activation">nested locked element!</div>
<div id="nonActivatable" rendersubtree="invisible skip-activation">nested non activatable locked element</div>
<!--
TODO(rakina): Make display:none, visibility:hidden, aria-hidden nodes
in locked subtrees get ignored for accessibility/marked invisible.
-->
<div style="display:none;">display:none text will be in AX tree</div>
<div role="presentation">role=presentation item will be in AX tree</div>
<div style="visibility:hidden;">visibility:hidden text will be in AX tree</div>
<div aria-hidden="true">aria-hidden text will be in AX tree</div>
<div style="display:none;">display:none text</div>
<ul role="presentation">
<li>role=presentation item</li>
</ul>
<div style="visibility:hidden;">visibility:hidden text</div>
<div aria-hidden="true">aria-hidden text</div>
</div>
</div>
......@@ -32,6 +23,5 @@
// Force layout, then commit everything.
locked.removeAttribute("rendersubtree");
nested.removeAttribute("rendersubtree");
nonViewportActivatable.removeAttribute("rendersubtree");
nonActivatable.removeAttribute("rendersubtree");
</script>
......@@ -2,30 +2,27 @@ rootWebArea
++genericContainer ignored
++++genericContainer ignored
++++++genericContainer
++++++++staticText name='spacer so that everything below will be offscreen (and won't get viewport-activated)'
++++++++++inlineTextBox name='spacer so that everything below will be offscreen (and won't get viewport-activated)'
++++++genericContainer offscreen
++++++++staticText offscreen name='<newline> '
++++++++genericContainer offscreen
++++++++++staticText offscreen name='child text will be in AX tree but without layout'
++++++++staticText offscreen name='<newline> '
++++++++genericContainer offscreen
++++++++++staticText offscreen name='<newline> nested activatable locked element will be in AX tree but without layout<newline> '
++++++++staticText offscreen name='<newline> normal text 1<newline> '
++++++++genericContainer offscreen
++++++++staticText offscreen name='<newline> normal text 2<newline> '
++++++++genericContainer offscreen
++++++++staticText offscreen name='<newline> normal text 3<newline> '
++++++++staticText offscreen name='<newline> '
++++++++genericContainer invisible offscreen
++++++++++staticText offscreen name='display:none text will be in AX tree'
++++++++staticText offscreen name='<newline> '
++++++++staticText offscreen name='role=presentation item will be in AX tree'
++++++++staticText offscreen name='<newline> '
++++++++genericContainer invisible offscreen
++++++++++staticText offscreen name='visibility:hidden text will be in AX tree'
++++++++staticText offscreen name='<newline> '
++++++++genericContainer invisible offscreen
++++++++++staticText invisible offscreen name='aria-hidden text will be in AX tree'
++++++++staticText offscreen name='<newline> '
++++++++staticText name='<newline> '
++++++++genericContainer
++++++++++staticText name='child'
++++++++staticText name='<newline> '
++++++++genericContainer
++++++++++staticText name='nested locked element!'
++++++++staticText name='<newline> '
++++++++genericContainer
++++++++staticText name='<newline> '
++++++++staticText name='<newline> '
++++++++genericContainer invisible
++++++++++staticText name='display:none text'
++++++++staticText name='<newline> '
++++++++staticText name='<newline> '
++++++++genericContainer
++++++++++staticText name='role=presentation item'
++++++++staticText name='<newline> '
++++++++staticText name='<newline> '
++++++++genericContainer invisible
++++++++++staticText name='visibility:hidden text'
++++++++staticText name='<newline> '
++++++++genericContainer invisible
++++++++++staticText invisible name='aria-hidden text'
++++++++staticText name='<newline> '
......@@ -2,28 +2,19 @@
@BLINK-ALLOW:offscreen
-->
<div>
<div style="height:10000px;">spacer so that everything below will be offscreen (and won't get viewport-activated)</div>
<div id="locked" rendersubtree="invisible">
<div>child text will be in AX tree but without layout</div>
<div id="nested" rendersubtree="invisible">
nested activatable locked element will be in AX tree but without layout
</div>
normal text 1
<div id="nonViewportActivatable" rendersubtree="invisible skip-viewport-activation">
nested non-viewport-activatable locked element will not be in AX tree
</div>
normal text 2
<div id="nonActivatable" rendersubtree="invisible skip-activation">
nested non-activatable locked element will not be in AX tree
</div>
normal text 3
<div id="locked" rendersubtree="invisible skip-viewport-activation">
<div>child</div>
<div id="nested" rendersubtree="invisible skip-viewport-activation">nested locked element!</div>
<div id="nonActivatable" rendersubtree="invisible skip-activation">nested non activatable locked element</div>
<!--
TODO(rakina): Make display:none, visibility:hidden, aria-hidden nodes
in locked subtrees get ignored for accessibility/marked invisible.
-->
<div style="display:none;">display:none text will be in AX tree</div>
<div role="presentation">role=presentation item will be in AX tree</div>
<div style="visibility:hidden;">visibility:hidden text will be in AX tree</div>
<div aria-hidden="true">aria-hidden text will be in AX tree</div>
<div style="display:none;">display:none text</div>
<ul role="presentation">
<li>role=presentation item</li>
</ul>
<div style="visibility:hidden;">visibility:hidden text</div>
<div aria-hidden="true">aria-hidden text</div>
</div>
</div>
rootWebArea
++genericContainer ignored
++++genericContainer ignored
++++++genericContainer offscreen
++++++++staticText offscreen name='initial spacer, will initially make everything below this far away from the viewport'
++++++++++inlineTextBox offscreen name='initial spacer, will initially make everything below this far away from the viewport'
++++++genericContainer
++++++++staticText name='This text will get viewport-activated because it's in the viewport, and will be in AX tree with layout.'
++++++++++inlineTextBox name='This text will get viewport-activated because it's in the viewport, and will be in AX tree with layout.'
++++++++genericContainer
++++++++++staticText name='This too!'
++++++++++++inlineTextBox name='This too!'
++++++genericContainer
++++++++staticText name='spacer so that everything below will be offscreen (and won't get viewport-activated)'
++++++++++inlineTextBox name='spacer so that everything below will be offscreen (and won't get viewport-activated)'
++++++genericContainer
++++++++staticText offscreen name='<newline> This text will not get viewport-activated, and will be in AX tree but without layout.<newline> '
++++++genericContainer offscreen
++++++++staticText offscreen name='doneActivating'
++++++++++inlineTextBox offscreen name='doneActivating'
<!--
@BLINK-ALLOW:offscreen
@WAIT-FOR:doneActivating
-->
<div>
<div id="initialSpacer" style="height: 10000px;">
initial spacer, will initially make everything below this far away from the viewport
</div>
<div id="inViewport" rendersubtree="invisible">
This text will get viewport-activated because it's in the viewport, and will be in AX tree with layout.
<div id="nestedInViewport" rendersubtree="invisible">
This too!
</div>
</div>
<div style="height:10000px;">
spacer so that everything below will be offscreen (and won't get viewport-activated)
</div>
<div id="wayBelowViewport" rendersubtree="invisible">
This text will not get viewport-activated, and will be in AX tree but without layout.
</div>
<div id="statusDiv"></div>
</div>
<script>
inViewport.getBoundingClientRect();
inViewport.scrollIntoView();
// Viewport activation happens on the next frame, so we'll wait until then before ending.
window.requestAnimationFrame(() => {
window.requestAnimationFrame(() => {
statusDiv.innerText = "doneActivating";
});
});
</script>
......@@ -45,8 +45,7 @@ enum class DisplayLockActivationReason {
// Shorthands
kViewport = static_cast<uint16_t>(kSelection) |
static_cast<uint16_t>(kUserFocus) |
static_cast<uint16_t>(kViewportIntersection) |
static_cast<uint16_t>(kAccessibility),
static_cast<uint16_t>(kViewportIntersection),
kAny = static_cast<uint16_t>(kAccessibility) |
static_cast<uint16_t>(kFindInPage) |
static_cast<uint16_t>(kFragmentNavigation) |
......
......@@ -298,24 +298,23 @@ Element* DisplayLockUtilities::NearestLockedExclusiveAncestor(
return nullptr;
}
bool DisplayLockUtilities::IsInUnlockedOrActivatableSubtree(
const Node& node,
DisplayLockActivationReason activation_reason) {
bool DisplayLockUtilities::IsInNonActivatableLockedSubtree(const Node& node) {
if (!RuntimeEnabledFeatures::DisplayLockingEnabled(
node.GetExecutionContext()) ||
node.GetDocument().LockedDisplayLockCount() == 0 ||
node.GetDocument().DisplayLockBlockingAllActivationCount() == 0 ||
!node.CanParticipateInFlatTree()) {
return true;
return false;
}
for (auto* element = NearestLockedExclusiveAncestor(node); element;
element = NearestLockedExclusiveAncestor(*element)) {
if (!element->GetDisplayLockContext()->IsActivatable(activation_reason)) {
return false;
if (!element->GetDisplayLockContext()->IsActivatable(
DisplayLockActivationReason::kAny)) {
return true;
}
}
return true;
return false;
}
bool DisplayLockUtilities::IsInLockedSubtreeCrossingFrames(
......
......@@ -70,22 +70,8 @@ class CORE_EXPORT DisplayLockUtilities {
static Element* NearestLockedInclusiveAncestor(const LayoutObject& object);
static Element* NearestLockedExclusiveAncestor(const LayoutObject& object);
// Returns true if |node| is not in a locked subtree, or if it's possible to
// activate all of the locked ancestors for |activation_reason|.
static bool IsInUnlockedOrActivatableSubtree(
const Node& node,
DisplayLockActivationReason activation_reason =
DisplayLockActivationReason::kAny);
// Returns true if |node| is in a locked subtree, and at least one of its
// locked ancestors can't be activated with |activation_reason|. In other
// words, this node should be treated as if it's not in the tree for
// |activation_reason|.
static bool ShouldIgnoreNodeDueToDisplayLock(
const Node& node,
DisplayLockActivationReason activation_reason) {
return !IsInUnlockedOrActivatableSubtree(node, activation_reason);
}
// Whether this node has non-activatable locked exclusive ancestors or not.
static bool IsInNonActivatableLockedSubtree(const Node& node);
// Returns true if the element is in a locked subtree (or is self-locked with
// no self-updates). This crosses frames while navigating the ancestor chain.
......
......@@ -361,8 +361,7 @@ bool AXNodeObject::ComputeAccessibilityIsIgnored(
}
if (DisplayLockUtilities::NearestLockedExclusiveAncestor(*GetNode())) {
if (DisplayLockUtilities::ShouldIgnoreNodeDueToDisplayLock(
*GetNode(), DisplayLockActivationReason::kAccessibility)) {
if (DisplayLockUtilities::IsInNonActivatableLockedSubtree(*GetNode())) {
if (ignored_reasons)
ignored_reasons->push_back(IgnoredReason(kAXNotRendered));
return true;
......
......@@ -2,9 +2,6 @@ Tests accessibility values of display locked nodes
WebArea
generic
generic
text "spacer"
InlineTextBox
generic
text "locked"
generic
......@@ -20,7 +17,6 @@ WebArea
text "nested"
text "text"
generic
generic
generic
text "normal text"
InlineTextBox
......
(async function(testRunner) {
var {page, session, dp} = await testRunner.startHTML(
`
<div style='height: 10000px;'>spacer</div>
<div id='activatable' rendersubtree='invisible'>
var {page, session, dp} = await testRunner.startHTML(`
<div id='activatable' rendersubtree='invisible skip-viewport-activation'>
locked
<div id='child'>
child
<div id='grandChild'>grandChild</div>
</div>
<div id='invisible' style='display:none'>invisible</div>
<div id='nested' rendersubtree='invisible'>nested</div>
<div id='nested' rendersubtree='invisible skip-viewport-activation'>nested</div>
text
</div>
<div id='nonViewportActivatable' rendersubtree='invisible skip-viewport-activation'>nonViewportActivatable text</div>
<div id='nonActivatable' rendersubtree='invisible skip-activation'>nonActivatable text</div>
<div id='normal'>normal text</div>
`,
'Tests accessibility values of display locked nodes');
`, 'Tests accessibility values of display locked nodes');
const dumpAccessibilityNodesFromList =
(await testRunner.loadScript('../resources/accessibility-dumpAccessibilityNodesFromList.js'))(testRunner, session);
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment