Include hidden objects in accessible tree. [chromium/src : main]
Jacobo Aragunde Pérez (Gerrit)
Attention is currently required from: Nektarios Paisios, Aaron Leventhal.
Jacobo Aragunde Pérez would like Nektarios Paisios and Aaron Leventhal to review this change.
Include hidden objects in accessible tree.
Include display:none, visibility:hidden and aria-hidden elements in
the accessible tree.
We plan to use them to streamline the calculation of accessible names,
so it does not require additional DOM or layout tree traversals. We
previously attempted to include only those hidden nodes which were
required for accname calculation, but there were complications to
keep the tree up-to-date. This is much easier to maintain and it
doesn't seem to have a noticeable performance impact.
A sizeable number of DumpAccessibilityTreeTest expectations were
updated to include the newly included hidden nodes.
Original author was aleve...@chromium.org in:
http://crrev.com/c/3197971
Bug: 1255036
Change-Id: I899781b654dd2b1dd97473b92e26d95fa54376bc
---
M content/test/data/accessibility/css/content-visibility-hidden-check-failure-expected-blink.txt
M content/test/data/accessibility/html/custom-element-expected-blink.txt
M content/test/data/accessibility/css/content-visibility-to-hidden-expected-blink.txt
M content/test/data/accessibility/html/generated-content-after-hidden-input-expected-blink.txt
M content/test/data/accessibility/aria/hidden-described-by-expected-blink.txt
M content/test/data/accessibility/html/select-in-canvas-expected-blink.txt
M content/test/data/accessibility/regression/add-child-of-not-included-in-tree-chain-expected-blink.txt
M content/test/data/accessibility/html/modal-dialog-closed-expected-blink.txt
M content/test/data/accessibility/html/wbr-expected-blink.txt
M content/test/data/accessibility/html/accordion-expected-blink.txt
M content/test/data/accessibility/css/display-none-expected-blink.txt
M content/test/data/accessibility/css/dom-element-css-alternative-text-expected-blink.txt
M content/test/data/accessibility/html/svg-desc-in-group-expected-blink.txt
M content/test/data/accessibility/html/svg-symbol-with-role-expected-blink.txt
M content/test/data/accessibility/css/visibility-to-hidden-expected-blink.txt
M content/test/data/accessibility/display-locking/all-expected-blink.txt
M content/test/data/accessibility/html/details-expected-blink.txt
M content/test/data/accessibility/html/svg-elements-not-mapped-expected-blink.txt
M content/test/data/accessibility/aria/table-column-hidden-expected-blink.txt
M content/test/data/accessibility/html/meter-expected-blink.txt
M content/test/data/accessibility/html/svg-text-alternative-computation-expected-blink.txt
M content/test/data/accessibility/aria/hidden-labelled-by-expected-blink.txt
M third_party/blink/renderer/modules/accessibility/ax_object.cc
M content/test/data/accessibility/html/custom-element-with-aria-owns-inside-expected-blink.txt
M content/test/data/accessibility/aria/aria-undefined-literal-expected-blink.txt
M content/test/data/accessibility/css/alt-text-expected-blink.txt
M content/test/data/accessibility/html/summary-expected-blink.txt
M content/test/data/accessibility/html/modal-dialog-stack-expected-blink.txt
M content/test/data/accessibility/aria/aria-hidden-expected-blink.txt
M content/test/data/accessibility/aria/aria-hidden-single-descendant-visibility-hidden-expected-blink.txt
M content/test/data/accessibility/aria/hidden-expected-blink.txt
M content/test/data/accessibility/html/svg-title-in-group-expected-blink.txt
M content/test/data/accessibility/aria/aria-empty-string-expected-blink.txt
M content/test/data/accessibility/regression/content-visibility-label-expected-blink.txt
M content/test/data/accessibility/html/input-suggestions-source-element-expected-blink.txt
M content/test/data/accessibility/regression/missing-parent-expected-blink.txt
M content/test/data/accessibility/html/landmark-expected-blink.txt
M content/test/data/accessibility/aria/aria-owns-crash-expected-blink.txt
M content/test/data/accessibility/aria/aria-undefined-expected-blink.txt
M content/test/data/accessibility/css/visibility-expected-blink.txt
M content/test/data/accessibility/css/content-visibility-auto-aria-hidden-expected-blink.txt
M content/test/data/accessibility/regression/title-in-shadow-expected-blink.txt
M content/test/data/accessibility/aria/aria-hidden-single-descendant-display-none-expected-blink.txt
M content/test/data/accessibility/html/input-list-expected-blink.txt
M content/test/data/accessibility/html/table-column-remove-expected-blink.txt
M content/test/data/accessibility/html/custom-element-hidden-expected-blink.txt
M content/test/data/accessibility/display-locking/non-activatable-expected-blink.txt
M content/test/data/accessibility/html/svg-expected-blink.txt
M content/test/data/accessibility/html/svg-with-clickable-rect-expected-blink.txt
M content/test/data/accessibility/html/open-modal-expected-blink.txt
M content/test/data/accessibility/html/ruby-expected-blink.txt
51 files changed, 227 insertions(+), 28 deletions(-)
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Jacobo Aragunde Pérez (Gerrit)
Attention is currently required from: Nektarios Paisios, Aaron Leventhal.
1 comment:
File content/test/data/accessibility/display-locking/non-activatable-expected-blink.txt:
Patch Set #3, Line 6:
++++++++++staticText ignored invisible name='<newline> 'An interesting result of this patch is these kinds of empty text nodes getting added. If we change the test HTML to be visible, these nodes would not appear.
Not sure if it's worth doing something to prevent this, some existing tests already include whitespace nodes like this, for example: https://source.chromium.org/chromium/chromium/src/+/main:content/test/data/accessibility/html/details-expected-blink.txt
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Nektarios Paisios, Jacobo Aragunde Pérez.
Patch set 3:Code-Review +1
1 comment:
File content/test/data/accessibility/display-locking/non-activatable-expected-blink.txt:
Patch Set #3, Line 6:
++++++++++staticText ignored invisible name='<newline> '
An interesting result of this patch is these kinds of empty text nodes getting added. […]
Normally whitespace nodes like this are removed when they have a layout object, via IsTextRelevantForAccessibility() at (https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/accessibility/ax_object_cache_impl.cc;l=258
However, the layout object is required in order to do a good job determining this.I don't think we can easily remove them from the tree in the display:none case, because of the case where an aria-labelledby or aria-describedby point to a subtree that's display:none. We need to use the whitespace to know where to insert a ' ' in the accessible name.
If you agree, then I would say let's keep them since they aren't doing any real harm either, and are necessary for the accname computation.
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Nektarios Paisios (Gerrit)
Attention is currently required from: Jacobo Aragunde Pérez.
Patch set 3:Code-Review +1
2 comments:
Patchset:
LGTM with one nit.
File third_party/blink/renderer/modules/accessibility/ax_object.cc:
Patch Set #3, Line 2501:
if (!GetLayoutObject())Nit: Perhaps add a comment above this line to the effect:
"Nodes in display none subtrees, or nodes that are display locked, lack a layout object."
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Jacobo Aragunde Pérez.
Aaron Leventhal removed a vote from this change.
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Nektarios Paisios, Jacobo Aragunde Pérez.
2 comments:
Patchset:
Removing my +1 until we handle the content-visibility:auto case as described in my reply to Nektarios.
File third_party/blink/renderer/modules/accessibility/ax_object.cc:
Patch Set #3, Line 2501:
if (!GetLayoutObject())
Nit: Perhaps add a comment above this line to the effect: […]
Nektarios' request for comment made me realize we need special handling for content-visibility:auto here.
First, note that There are 4 ways to use CSS to hide something (we should add this as a comment to IsHiddenViaStyle()).
- display none is "destroy rendering state and don't do anything in the subtree"
- visibility hidden and visibility collapse are "don't visually show things, but still keep all of the rendering up to date"
- content-visibility: hidden is "don't show anything, skip all of the work, but don't destroy the work that was already there"
Then there's content-visibility:auto, which is painted when it's scrolled into the viewport, but its layout information is not updated when it isn't. In fact, it can have a layout object that is stale, if the node came into the viewport, and then leaves the viewport. See https://bugs.chromium.org/p/chromium/issues/detail?id=1259179
We want to include all content-visibility:auto elements in the accessibility tree, and we want to protect ourselves from making calls on their layout object in case the layout object is stale.
```
bool is_content_visibility_auto = DisplayLockUtilities::LockedAncestorPreventingLayout(node) && DisplayLockUtilities::ShouldIgnoreNodeDueToDisplayLock(
*GetNode(), DisplayLockActivationReason::kAccessibility) == false)
if (is_content_visibility_auto)
return true;
```I suggest we add that before !GetLayoutObject() block. Note that content-visibility: auto subtrees are treated as visible in IsHiddenViaStyle(), as we must make a guess since computed style is not available.
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Jacobo Aragunde Pérez (Gerrit)
File third_party/blink/renderer/modules/accessibility/ax_object.cc:
Patch Set #3, Line 2501:
if (!GetLayoutObject())
Nektarios' request for comment made me realize we need special handling for content-visibility:auto […]
I wrote some tests in https://crrev.com/c/3225679, trying to break this code in combination with a patch to do accessible tree traversal for accname I haven't submitted yet. Unfortunately, they all pass.
I can still move forward with these modifications for content-visibility:auto, but I would have preferred to have a failing test to detect this problem...
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Nektarios Paisios, Jacobo Aragunde Pérez.
Patch set 3:Code-Review +1
2 comments:
Patchset:
LGTM assuming we add the content-visibility changes
File third_party/blink/renderer/modules/accessibility/ax_object.cc:
Patch Set #3, Line 2501:
if (!GetLayoutObject())
I wrote some tests in https://crrev. […]
Please move forward with the suggested modifications. @vmpstr/jarhar/etc. will work on improvements to make sure we never get/use a stale layout object.
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Nektarios Paisios, Jacobo Aragunde Pérez.
1 comment:
File third_party/blink/renderer/modules/accessibility/ax_object.cc:
Patch Set #3, Line 2501:
if (!GetLayoutObject())
Please move forward with the suggested modifications. @vmpstr/jarhar/etc. […]
(They will be adding a DCHECK that triggers if GetLayoutObject() retrieves a stale layout object).
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Jacobo Aragunde Pérez (Gerrit)
Attention is currently required from: Nektarios Paisios.
1 comment:
File third_party/blink/web_tests/accessibility/canvas-fallback-content-labels.html:
Patch Set #4, Line 92:
// Both check_label1 and check_label2 didn't generate any accessibility nodes, now check_label2 does generate one.I need to check this case more deeply. The tree generated by check_label2, inside the canvas, used to be:
++++++canvas
++++++++staticText name='<newline> '
++++++++staticText name='<newline> Checkbox<newline> '
++++++++checkBoxWhile, after this patch, it's:
++++++canvas
++++++++staticText name='<newline> '
++++++++labelText htmlTag='label'
++++++++++staticText name='<newline> Checkbox<newline> '
++++++++++checkBoxThe tree generated by check_label1, outside canvas, is like this both before and after the patch:
++++++genericContainer htmlTag='div'
++++++++staticText name='Checkbox '
++++++++checkBox
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Nektarios Paisios, Jacobo Aragunde Pérez.
Patch set 4:Code-Review +1
1 comment:
File third_party/blink/web_tests/accessibility/canvas-fallback-content-labels.html:
Patch Set #4, Line 92:
// Both check_label1 and check_label2 didn't generate any accessibility nodes, now check_label2 does generate one.I need to check this case more deeply. […]
The label on a radio or checkbox can be ignored because of this code:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/accessibility/ax_node_object.cc;l=3477?q=ax_node_object.cc&ss=chromiumIn order to resolve the discrepency, we could just decide that labels are always included in the tree. That way it will be included whether it's in a canvas or not.
Side note: fallback content inside of a canvas is a horrible authoring technique, because there are no object bounds, and because the lack of layout objects leads to unusual codepaths.
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Jacobo Aragunde Pérez (Gerrit)
Attention is currently required from: Nektarios Paisios, Aaron Leventhal.
3 comments:
Patchset:
Please let me know about this cached_is_canvas_fallback_ idea. Thanks!
File third_party/blink/renderer/modules/accessibility/ax_object.cc:
Patch Set #4, Line 2501:
if (RuntimeEnabledFeatures::AccessibilityExposeDisplayNoneEnabled()) {This code used to be hidden under a runtime feature. Now it's always enabled and this is the only use of AccessibilityExposeDisplayNoneEnabled, so we possibly can remove it?
File third_party/blink/web_tests/accessibility/canvas-fallback-content-labels.html:
Patch Set #4, Line 92:
// Both check_label1 and check_label2 didn't generate any accessibility nodes, now check_label2 does generate one.
The label on a radio or checkbox can be ignored because of this code: […]
That's exactly what happens here, because the <label> inside the canvas does not have a layout object it hits the !GetLayoutObject() check we added to include display:none nodes.
My attempt to fix it was to detect if a node is inside a canvas fallback and save that as a boolean in the AXObject. It works, the test passes because I managed to generate the expected tree:
++++++canvas
++++++++staticText name='<newline> '
++++++++staticText name='<newline> Checkbox<newline> '
++++++++checkBox
It needs a bit of cleanup, but do you agree with the addition of cached_is_canvas_fallback_ in patchset 5? Could it be useful?
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Jacobo Aragunde Pérez (Gerrit)
Attention is currently required from: Nektarios Paisios, Aaron Leventhal.
Patch set 5:Commit-Queue +2
Nektarios Paisios (Gerrit)
Attention is currently required from: Jacobo Aragunde Pérez, Aaron Leventhal.
Patch set 5:Code-Review +1
2 comments:
Patchset:
Please let me know about this cached_is_canvas_fallback_ idea. […]
There is a good reason why we hide label objects: it's because when the label is present, a screen reader will announce both the label and the accessible name of the checkbox or radio button, causing the label to be spoken twice.
Having said that, we should make all label objects included in the tree, but ignored.
How about doing that in a followup
File third_party/blink/renderer/modules/accessibility/ax_object.cc:
Patch Set #4, Line 2501:
if (RuntimeEnabledFeatures::AccessibilityExposeDisplayNoneEnabled()) {
This code used to be hidden under a runtime feature. […]
Yes, we should remove the runtime flag if we are not using it any more.
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Jacobo Aragunde Pérez.
Patch set 5:Code-Review +1
1 comment:
Patchset:
There is a good reason why we hide label objects: it's because when the label is present, a screen r […]
I prefer the "labels are always included in the tree, even if ignored" approach, over cached_is_canvas_fallback.
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Jacobo Aragunde Pérez (Gerrit)
Attention is currently required from: Jacobo Aragunde Pérez.
Patch set 6:Commit-Queue +2
Jacobo Aragunde Pérez (Gerrit)
1 comment:
Patchset:
I prefer the "labels are always included in the tree, even if ignored" approach, over cached_is_canv […]
Implemented "labels are always included in the tree, even if ignored" at: https://crrev.com/c/3245335. I'm currently reviewing if there are more test expectations needing change.
We would need to land it before this CL, so it fixes the canvas-fallback-content-labels.html test.
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Nektarios Paisios (Gerrit)
Attention is currently required from: Jacobo Aragunde Pérez.
Patch set 7:Code-Review +1
1 comment:
Patchset:
Time to land?
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Jacobo Aragunde Pérez (Gerrit)
2 comments:
File content/test/data/accessibility/display-locking/non-activatable-expected-blink.txt:
Patch Set #3, Line 6:
++++++++++staticText ignored invisible name='<newline> '
Normally whitespace nodes like this are removed when they have a layout object, via IsTextRelevantFo […]
Ack
File third_party/blink/web_tests/accessibility/canvas-fallback-content-labels.html:
Patch Set #4, Line 92:
// Both check_label1 and check_label2 didn't generate any accessibility nodes, now check_label2 does generate one.
That's exactly what happens here, because the <label> inside the canvas does not have a layout objec […]
I have removed cached_is_canvas_fallback_ in patchset 7, and implemented the suggested alternative "labels are always included in the tree, even if ignored" at https://crrev.com/c/3245335
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Jacobo Aragunde Pérez (Gerrit)
1 comment:
Patchset:
I'll have to explicitly fix the failure in this test: CrossPlatformAccessibilityBrowserTest.NavigateInIframe
This is caused by (I think) unintentionally broken markup in the test:
<button>Before iframe</button>
<iframe src="inner1.html">
<button>After iframe</button>
Because the iframe tag is not closed, the button becomes a child of the iframe, and it's never rendered. This patch is exposing that non-existent button in the accessibility tree, overwriting the actual children of the iframe node.The markup is wrong but the accessibility tree must reflect what's rendered, in this case it's the iframe contents that's rendered, not the button.
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Jacobo Aragunde Pérez (Gerrit)
1 comment:
File third_party/blink/renderer/modules/accessibility/ax_object.cc:
Patch Set #3, Line 2501:
if (!GetLayoutObject())
(They will be adding a DCHECK that triggers if GetLayoutObject() retrieves a stale layout object).
Patchset 8 adds the extra comments suggested in this thread.
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Jacobo Aragunde Pérez.
Patch set 8:Code-Review +1
1 comment:
File third_party/blink/renderer/modules/accessibility/ax_object.cc:
Patch Set #3, Line 2501:
if (!GetLayoutObject())
Patchset 8 adds the extra comments suggested in this thread.
Right. Some nodes are not in the flat tree, that we reach via LayoutTreeBuilderTraversal. We should never create AXObjects for those nodes in the first place. The IsRelevant* functions in AXObjectCacheImpl should hopefully know how to do that.
Take a look at Joanie' recent fix here, and Rune's comments:
https://chromium-review.googlesource.com/c/chromium/src/+/3237246
I don't think we've done this correctly yet -- avoiding creating AXObjects for nodes that are not in the flat tree.
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Jacobo Aragunde Pérez (Gerrit)
1 comment:
Patchset:
I'll have to explicitly fix the failure in this test: CrossPlatformAccessibilityBrowserTest. […]
A workaround for this issue has been submitted to https://crrev.com/c/3251072. When that's landed, this test won't cause more trouble.
Meanwhile, there are other test failures I have to take care of.
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Jacobo Aragunde Pérez (Gerrit)
Jacobo Aragunde Pérez uploaded patch set #10 to this change.
Include hidden objects in accessible tree.
Include display:none, visibility:hidden and aria-hidden elements in
the accessible tree.
We plan to use them to streamline the calculation of accessible names,
so it does not require additional DOM or layout tree traversals. We
previously attempted to include only those hidden nodes which were
required for accname calculation, but there were complications to
keep the tree up-to-date. This is much easier to maintain and it
doesn't seem to have a noticeable performance impact.
A sizeable number of DumpAccessibilityTreeTest expectations were
updated to include the newly included hidden nodes.
Original author was aleve...@chromium.org in:
http://crrev.com/c/3197971
Bug: 1255036
Change-Id: I899781b654dd2b1dd97473b92e26d95fa54376bc
Cq-Include-Trybots: luci.chromium.try:win7-blink-rel,win7-rel
---
M content/test/data/accessibility/css/content-visibility-hidden-check-failure-expected-blink.txt
M content/test/data/accessibility/html/custom-element-expected-blink.txt
M third_party/blink/web_tests/accessibility/element-role-mapping-normal-expected.txt
M content/test/data/accessibility/css/content-visibility-to-hidden-expected-blink.txt
M content/test/data/accessibility/html/generated-content-after-hidden-input-expected-blink.txt
M content/test/data/accessibility/aria/hidden-described-by-expected-blink.txt
M third_party/blink/web_tests/accessibility/parent-is-included-in-tree.html
M content/test/data/accessibility/html/select-in-canvas-expected-blink.txt
M content/test/data/accessibility/regression/add-child-of-not-included-in-tree-chain-expected-blink.txt
M content/test/data/accessibility/html/modal-dialog-closed-expected-blink.txt
M content/test/data/accessibility/html/wbr-expected-blink.txt
M content/test/data/accessibility/html/accordion-expected-blink.txt
M content/test/data/accessibility/html/selectmenu-expected-blink.txt
M third_party/blink/renderer/modules/accessibility/ax_selection_test.cc
M content/test/data/accessibility/aria/aria-owns-crash-expected-blink.txt
M content/test/data/accessibility/aria/aria-undefined-expected-blink.txt
M content/test/data/accessibility/css/visibility-expected-blink.txt
M content/test/data/accessibility/css/content-visibility-auto-aria-hidden-expected-blink.txt
M content/test/data/accessibility/regression/title-in-shadow-expected-blink.txt
M content/test/data/accessibility/aria/aria-hidden-single-descendant-display-none-expected-blink.txt
M content/test/data/accessibility/html/input-list-expected-blink.txt
M content/test/data/accessibility/html/table-column-remove-expected-blink.txt
M content/test/data/accessibility/html/custom-element-hidden-expected-blink.txt
M content/test/data/accessibility/display-locking/non-activatable-expected-blink.txt
M content/test/data/accessibility/html/svg-expected-blink.txt
M content/test/data/accessibility/html/svg-with-clickable-rect-expected-blink.txt
M content/test/data/accessibility/html/open-modal-expected-blink.txt
M content/test/data/accessibility/html/ruby-expected-blink.txt
55 files changed, 303 insertions(+), 49 deletions(-)
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Jacobo Aragunde Pérez (Gerrit)
2 comments:
Patchset:
With the latest patchset, 12, there should be only some ChromeOS-specific tests failing.
File third_party/blink/web_tests/wpt_internal/display-lock/css-content-visibility/accessibility/content-visibility-accessibility-001.html:
This is exposing an interesting behavior change: when whitespace text nodes are created as children of an AXNodeObject (because the parent node does not have layout yet), and later the parent node becomes an AXLayoutObject, the whitespace text nodes are kept in the tree, marked as ignored.
This is different from creating an AXLayoutObject in first place, where these whitespace nodes never come to exist in first place.
It doesn't seem to have consequences for accname or platform accessibility tree, outside of altered test expectations.
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Jacobo Aragunde Pérez.
Patch set 12:Code-Review +1
1 comment:
File third_party/blink/web_tests/wpt_internal/display-lock/css-content-visibility/accessibility/content-visibility-accessibility-001.html:
This is exposing an interesting behavior change: when whitespace text nodes are created as children […]
Good find. That's a bug. Will you file an issue and address it for us?
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Jacobo Aragunde Pérez (Gerrit)
1 comment:
Patchset:
This is a WIP update, I'm trying to fix the browser test SelectToSpeakKeystrokeSelectionTest.textFieldWithComboBoxSimple, which is only run in ChromeOS: https://source.chromium.org/chromium/chromium/src/+/main:chrome/browser/resources/chromeos/accessibility/select_to_speak/select_to_speak_keystroke_selection_test.js;l=524;bpv=1;bpt=0;drc=f7dd12db4aef82c41589b0f77aea5390f96d1d6a
The test loads the following HTML+JS in the browser, then triggers the keypresses that enable "read selected" in ChromeVox, then checks if the TTS is speaking, that's when it fails:
<!doctype html>
<script type="text/javascript">
function doSelection() {
let selection = window.getSelection();
let range = document.createRange();
selection.removeAllRanges();
let body = document.getElementsByTagName("body")[0];
range.setStart(body, 0);
range.setEnd(body, 1);
selection.addRange(range);
}
</script>
<body onload="doSelection()">
<input list="list" value="one"></label><datalist id="list">
<option value="one"></datalist></body>The difference in the accessibility tree generated before and after the patch is that our patch makes the ignored nodes for the datalist and option DOM nodes be included in the tree. Apparently, that's enough to make the test fail, I don't know yet how this change can affect the selection code.
The wrong markup </label> doesn't seem to affect.
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Jacobo Aragunde Pérez (Gerrit)
1 comment:
Patchset:
This is a WIP update, I'm trying to fix the browser test SelectToSpeakKeystrokeSelectionTest. […]
The patchset 13 includes a fix for this: the test above will pass if the accessible nodes for datalist and option nodes are kept ignored and not included in the tree.
How this relates with this test, is a bit mysterious; I have several guesses:
1. The test is wrong, the selection is sets is actually empty and that's why TTS doesn't speak. I have this impression because, if you run that piece of HTML+JS in a browser, nothing will be visibly selected.
2. There is an underlying problem with selections when there are "ignored but included" nodes in the tree. We know how AXPosition behaves in relation to these kinds of nodes, it takes them into account like they were unignored. Maybe we are making asumptions about ignored nodes elsewhere.
I would rather land this CL with the workaround in patchset 13, and get back to the problem later. Mind you, there are still two other tests failing that I need to take a look, and I must redo some DumpTree test expectations due to this workaround.
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Jacobo Aragunde Pérez (Gerrit)
Attention is currently required from: Jonathan Ross.
Jacobo Aragunde Pérez would like Jonathan Ross to review this change.
Include hidden objects in accessible tree.
Include display:none, visibility:hidden and aria-hidden elements in
the accessible tree.
We plan to use them to streamline the calculation of accessible names,
so it does not require additional DOM or layout tree traversals. We
previously attempted to include only those hidden nodes which were
required for accname calculation, but there were complications to
keep the tree up-to-date. This is much easier to maintain and it
doesn't seem to have a noticeable performance impact.
A sizeable number of DumpAccessibilityTreeTest expectations were
updated to include the newly included hidden nodes.
Original author was aleve...@chromium.org in:
http://crrev.com/c/3197971
Bug: 1255036
Change-Id: I899781b654dd2b1dd97473b92e26d95fa54376bc
Cq-Include-Trybots: luci.chromium.try:win7-blink-rel,win7-rel
---
M content/test/data/accessibility/css/content-visibility-hidden-check-failure-expected-blink.txt
M content/test/data/accessibility/html/custom-element-expected-blink.txt
M third_party/blink/web_tests/accessibility/element-role-mapping-normal-expected.txt
M content/test/data/accessibility/css/content-visibility-to-hidden-expected-blink.txt
M content/test/data/accessibility/html/generated-content-after-hidden-input-expected-blink.txt
M content/test/data/accessibility/html/iframe-with-invalid-children-added-expected-blink.txt
M content/test/data/accessibility/html/svg-elements-not-mapped-expected-blink-cros.txt
M content/test/data/accessibility/aria/hidden-described-by-expected-blink.txt
M third_party/blink/web_tests/accessibility/parent-is-included-in-tree.html
M third_party/blink/web_tests/wpt_internal/display-lock/css-content-visibility/accessibility/content-visibility-accessibility-004.html
M content/test/data/accessibility/html/select-in-canvas-expected-blink.txt
M content/test/data/accessibility/regression/add-child-of-not-included-in-tree-chain-expected-blink.txt
M content/test/data/accessibility/html/modal-dialog-closed-expected-blink.txt
M content/test/data/accessibility/html/wbr-expected-blink.txt
M content/test/data/accessibility/event/add-dialog-expected-uia-win7.txt
M content/test/data/accessibility/html/accordion-expected-blink.txt
M content/test/data/accessibility/css/display-none-expected-blink.txt
M content/test/data/accessibility/css/dom-element-css-alternative-text-expected-blink.txt
M content/test/data/accessibility/html/svg-desc-in-group-expected-blink.txt
M content/test/data/accessibility/html/svg-symbol-with-role-expected-blink.txt
M content/test/data/accessibility/css/visibility-to-hidden-expected-blink.txt
M content/test/data/accessibility/display-locking/all-expected-blink.txt
M content/test/data/accessibility/html/details-expected-blink.txt
M third_party/blink/web_tests/wpt_internal/display-lock/css-content-visibility/accessibility/content-visibility-accessibility-005.html
M content/test/data/accessibility/event/add-dialog-no-info-expected-win.txt
M content/test/data/accessibility/html/svg-elements-not-mapped-expected-blink.txt
M content/browser/renderer_host/render_widget_host_view_mac.mm
M content/test/data/accessibility/aria/table-column-hidden-expected-blink.txt
M content/test/data/accessibility/html/meter-expected-blink.txt
M content/test/data/accessibility/html/svg-text-alternative-computation-expected-blink.txt
M content/test/data/accessibility/aria/hidden-labelled-by-expected-blink.txt
M content/test/data/accessibility/event/aria-hidden-single-descendant-visibility-hidden-expected-auralinux.txt
M third_party/blink/renderer/modules/accessibility/ax_object.cc
M content/test/data/accessibility/event/add-dialog-no-info-expected-uia-win7.txt
M content/test/data/accessibility/html/custom-element-with-aria-owns-inside-expected-blink.txt
M content/test/data/accessibility/aria/aria-undefined-literal-expected-blink.txt
M third_party/blink/web_tests/http/tests/inspector-protocol/accessibility/accessibility-query-axtree-expected.txt
M content/test/data/accessibility/css/alt-text-expected-blink.txt
M content/test/data/accessibility/event/add-dialog-described-by-expected-uia-win.txt
M content/test/data/accessibility/html/summary-expected-blink.txt
M content/test/data/accessibility/html/modal-dialog-stack-expected-blink.txt
M content/test/data/accessibility/aria/aria-hidden-expected-blink.txt
M content/test/data/accessibility/event/add-dialog-expected-win.txt
M content/test/data/accessibility/aria/aria-hidden-single-descendant-visibility-hidden-expected-blink.txt
M content/test/data/accessibility/aria/hidden-expected-blink.txt
M content/test/data/accessibility/html/svg-title-in-group-expected-blink.txt
M content/test/data/accessibility/aria/aria-empty-string-expected-blink.txt
M content/test/data/accessibility/regression/content-visibility-label-expected-blink.txt
M content/test/data/accessibility/html/input-suggestions-source-element-expected-blink.txt
M third_party/blink/web_tests/wpt_internal/display-lock/css-content-visibility/accessibility/content-visibility-accessibility-001.html
M content/test/data/accessibility/regression/missing-parent-expected-blink.txt
M content/test/data/accessibility/html/landmark-expected-blink.txt
M content/test/data/accessibility/event/add-dialog-expected-uia-win.txt
M third_party/blink/renderer/modules/accessibility/ax_selection_test.cc
M content/test/data/accessibility/aria/aria-owns-crash-expected-blink.txt
M content/test/data/accessibility/event/add-dialog-no-info-expected-uia-win.txt
M content/test/data/accessibility/aria/aria-undefined-expected-blink.txt
M content/test/data/accessibility/event/add-dialog-described-by-expected-uia-win7.txt
M third_party/blink/web_tests/wpt_internal/display-lock/css-content-visibility/accessibility/content-visibility-accessibility-002.html
M content/test/data/accessibility/css/visibility-expected-blink.txt
M content/test/data/accessibility/html/combobox-item-visibility-expected-blink.txt
M content/test/data/accessibility/css/content-visibility-auto-aria-hidden-expected-blink.txt
M third_party/blink/web_tests/wpt_internal/display-lock/css-content-visibility/accessibility/content-visibility-accessibility-008.html
M content/test/data/accessibility/regression/title-in-shadow-expected-blink.txt
M content/test/data/accessibility/aria/aria-hidden-single-descendant-display-none-expected-blink.txt
M content/test/data/accessibility/html/input-list-expected-blink.txt
M content/test/data/accessibility/html/table-column-remove-expected-blink.txt
M third_party/blink/web_tests/wpt_internal/display-lock/css-content-visibility/accessibility/content-visibility-accessibility-003.html
M content/test/data/accessibility/regression/slot-creation-crash-expected-blink.txt
M content/test/data/accessibility/html/custom-element-hidden-expected-blink.txt
M content/test/data/accessibility/display-locking/non-activatable-expected-blink.txt
M content/test/data/accessibility/html/modal-dialog-opened-expected-blink.txt
M content/test/data/accessibility/html/svg-expected-blink.txt
M content/test/data/accessibility/html/svg-with-clickable-rect-expected-blink.txt
M content/test/data/accessibility/event/add-dialog-described-by-expected-win.txt
M content/test/data/accessibility/html/open-modal-expected-blink.txt
M content/test/data/accessibility/html/ruby-expected-blink.txt
77 files changed, 417 insertions(+), 96 deletions(-)
Jacobo Aragunde Pérez (Gerrit)
Attention is currently required from: Jonathan Ross.
1 comment:
Patchset:
@jonross: Hi, we need a reviewer for render_widget_host_view_mac.mm. Thanks in advance!
Basically, the purpose if this CL is including hidden text nodes in the accessibility tree (as "ignored but included"). The goal is to be able to calculate accessible names and descriptions only from the information available in the accessibility tree.
This Mac change is necessary because of a problem detected by this test: RenderWidgetHostViewMacTest.GetPageTextForSpeech. A piece of hidden text was being included in the string returned by GetPageTextForSpeech(). Apparently, it was traversing all children, including ignored ones.
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Jacobo Aragunde Pérez (Gerrit)
Attention is currently required from: David Tseng, Jonathan Ross.
Jacobo Aragunde Pérez would like David Tseng to review this change.
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Jacobo Aragunde Pérez (Gerrit)
Attention is currently required from: David Tseng, Jonathan Ross.
1 comment:
Patchset:
@dtseng: Hi, sorry to disturb you! I wonder if you can explain this failure, the test fails due to excessive output, there's a message appearing over and over again.
The bot failure: https://ci.chromium.org/ui/p/chromium/builders/try/linux-chromeos-rel/1029201/overview
The test: ChromeVoxTutorialTest.ResourcesTest
The message: WARNING browser_tests[81628:1]: [logging_native_handler.cc(72)] Got parentChanged event on unknown node: 1353; this: 72
I have the same abundant output when running the test locally, both on the main branch and my CL branch, so I'm not certain if it's a problem with my CL or, if it were, how to reproduce it.
Thanks in advance for your time!
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
David Tseng (Gerrit)
Attention is currently required from: Jonathan Ross, Jacobo Aragunde Pérez.
Hi Jacobo,
No problem.
I patched in your change on top of origin/main as of ~1 hour ago and don't see the failure.
To try and reproduce locally, build with the same gn args as the bot. The important bit is
target_os = "chromeos"
and build the browser_tests target / run the ChromeVox test.
The parentChange event comes from ui/accessibility/ax_event_generator.cc and the errors seem to indicate that we're getting nodes that don't exist. I'd have to dig a bit further to see what's happening. It's plausible this change causes something to trigger that error.
Jacobo Aragunde Pérez (Gerrit)
Attention is currently required from: Jonathan Ross.
1 comment:
Patchset:
Thanks, David!
It makes sense, since we are dealing with the conditions to ignore nodes, that the problem happens more often. I've verified that there are twice as many error lines after my CL, that must be the reason for the "excessive output" failure. I'll dig further.
Before:
$ autoninja -C out/ChrOS/ browser_tests && \
out/ChrOS/browser_tests --gtest_filter=ChromeVoxTutorialTest.ResourcesTest 2>&1 \
| grep logging_native_handler | wc -l
1145After:
$ autoninja -C out/ChrOS/ browser_tests && \
out/ChrOS/browser_tests --gtest_filter=ChromeVoxTutorialTest.ResourcesTest 2>&1 \
| grep logging_native_handler | wc -l
2446(marking unresolved for my own tracking)
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Jonathan Ross (Gerrit)
Attention is currently required from: Jacobo Aragunde Pérez.
2 comments:
Patchset:
Sorry for the delay in replying, a bit behind past few days.
Conceptually SGTM. A question about this solution vs other platforms. I'm not familiar with the RequestAXTreeSnapshop path to know if Mac is the exception. Or if we are lacking test coverage on other platforms, and they too need an update.
File content/browser/renderer_host/render_widget_host_view_mac.mm:
Patch Set #16, Line 862:
GetWebContents()->RequestAXTreeSnapshotThis path seems to be shared cross platform:
WebContentsImpl::RequestAXTreeSnapshot -> RenderFrameHostImpl::RequestAXTreeSnapshot
Do we need similar logic to skip the newly indexed, but ignored, elements on other platforms?From the linux-rel run it appears that some blink_web_tests are failing.
I'm not familiar with those tests to know if they need updated expectations, like part of the rest of this change. Or if non-mac callsites need similar updates.
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Jacobo Aragunde Pérez (Gerrit)
1 comment:
Patchset:
Thanks, David! […]
An update (mostly to myself!):
- The warning message is like: [logging_native_handler.cc(72)] Got parentChanged event on unknown node: 401; this: 72
- The parentChanged event is produced at AXEventGenerator::OnNodeReparented(). The node ids are the ones that would be reported as "unknown nodes" later.
- Not all the parentChanged events result in warning messages later, according with the printed IDs.
- The number of times that AXEventGenerator::OnNodeReparented() is run is constant during the test, but it results in different amounts of warning messages. It seems that timing plays a role in this problem.
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Jacobo Aragunde Pérez (Gerrit)
1 comment:
Patchset:
The patchset 13 includes a fix for this: the test above will pass if the accessible nodes for datali […]
In patchset 16, I changed the code to ignore datalist/option a bit, to affect less of the existing tests. And after patchset 20, I think the expectations of all tests are updated, except for the other open comment threads.
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Jacobo Aragunde Pérez (Gerrit)
1 comment:
Patchset:
The number of times that AXEventGenerator::OnNodeReparented() is run is constant during the test, but it results in different amounts of warning messages. It seems that timing plays a role in this problem.
This is not true, sorry. I was using a reduced version of the test. The unmodified test always produces 2370 parentChanged warnings with the CL applied, and 1120 warnings without it.
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Jacobo Aragunde Pérez (Gerrit)
1 comment:
Patchset:
> The number of times that AXEventGenerator::OnNodeReparented() is run is constant during the test, […]
I think that ignored nodes, which are unknown to platform accessibility, are producing events anyway and that's the reason for these error messages.
This change to stop producing parent changed events on ignored nodes removes all the error messages in that test: https://chromium-review.googlesource.com/c/chromium/src/+/3281980
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Jacobo Aragunde Pérez (Gerrit)
Attention is currently required from: Jonathan Ross.
3 comments:
Patchset:
Sorry for the delay in replying, a bit behind past few days. […]
See other reply.
I think that ignored nodes, which are unknown to platform accessibility, are producing events anyway […]
Actually, it wasn't that ignored nodes were "producing events anyway", they generally did not and it only happened with specially convoluted trees. In the mentioned CL, this would be fixed and a test added to detect regressions.
File content/browser/renderer_host/render_widget_host_view_mac.mm:
Patch Set #16, Line 862:
GetWebContents()->RequestAXTreeSnapshot
This path seems to be shared cross platform: […]
I tried to follow other code paths that use this, but it's quite a lot of code. The most interesting one I found was the code that generates the thumbnails for tabs in Android Chrome, they seem to have accessibility information attached.
In patchset 17, I modified the HTML used in some Android tests to try to break them, and they passed. Which doesn't mean we are clear, tests could not be covering that case...
After discussing with Aaron, he suggested to add a DCHECK in the entry points to get the node name, to detect situations where we are trying to use the name of an ignored node. In patchsets 21-22, I tried to add such a check and undo the Mac fix to verify that it triggered the check. I got the condition wrong a couple times, so I ended up trying in a different CL to avoid noise here: https://chromium-review.googlesource.com/c/chromium/src/+/3304055
The DCHECK, unfortunately, is hit in debug/test code. I can't see a reasonable way to code when it's ok or not getting the name of an ignored node, and DCHECK might not be the way to do this: they should probably be "impossible" situations, also in test conditions.
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Jonathan Ross (Gerrit)
Attention is currently required from: Jacobo Aragunde Pérez.
Patch set 22:Code-Review +1
2 comments:
Patchset:
content/browser/renderer_host LGTM, once the debugging is removed.
Full comments in RenderWidgetHostViewMac, but thanks for doing a deeper dive into how this change affects other AX lookup, and tests. Worth considering a future update to make ignore element handling cleaner. But I'll leave that to the experts in the AX owners list.
File content/browser/renderer_host/render_widget_host_view_mac.mm:
Patch Set #16, Line 862:
GetWebContents()->RequestAXTreeSnapshot
I tried to follow other code paths that use this, but it's quite a lot of code. […]
Taking a brief look at some of the failures for the patch you linked, there seems to be a mix of test verification (DumpUnfilteredAccessibilityTreeAsString) which likely wants all the `ignored` string. As well as some update paths (ui::AXPlatformNodeAuraLinux::OnNameChanged) which also likely want to update the `ignored` strings.
Thank you for taking the time to look into that further. It does seem quite complex, and I lack the familiarity with the AX code to understand the impact outside of the test failures/expectations you've addressed.
I'd recommend discussing with the third_party/blink/renderer/modules/accessibility/OWNERS about potential ways to improve this. However that shouldn't block this particular fix.
I'll LGTM renderer_host, and feel free to revert the debugging to land.
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Jacobo Aragunde Pérez (Gerrit)
Attention is currently required from: Jacobo Aragunde Pérez.
1 comment:
File content/browser/renderer_host/render_widget_host_view_mac.mm:
Patch Set #16, Line 862:
GetWebContents()->RequestAXTreeSnapshotI tried to follow other code paths that use this, but it's quite a lot of code. […]
As a recap, the main problem is the usage of AXNode->children() for tree traversal, which includes ignored nodes. Nektarios marked it as deprecated and created a bunch of more explicit accessors.
I tried to track down the uses of AXNode->children() but they are very numerous... Fortunately, there are fewer uses of WebContentsImpl::RequestAXTreeSnapshot according to the code search site. They may, or may not, traverse nodes in a way that includes ignored nodes. These are the matches and my assessment of them:
content/browser/renderer_host/render_widget_host_view_mac.mm
* Exposes hidden content, breaking a test. Already fixed.content/browser/accessibility/snapshot_ax_tree_browsertest.cc
* I think it's fine to access ignored nodes from tests.chrome/browser/ui/ash/assistant/assistant_context_util.cc
* From here, it ends walking the tree using GetUnignoredChild* functions, so there's no problem.chrome/browser/paint_preview/services/paint_preview_tab_service.cc
* It runs ax::mojom::AXTreeUpdate::Serialize(&ax_tree_update), apparently it will serialize to a file to be recovered later (https://crbug.com/1179326). It makes sense that the tree includes everything, as long as ignored nodes are dealt with later. I couldn't try this feature first hand.chrome/browser/ui/webui/read_later/side_panel/reader_mode/reader_mode_page_handler.cc
* Problematic, it calls ->children(), traversing also ignored nodes and exposing them in reader mode. I'm sending a fix.components/translate/content/browser/per_frame_content_translate_driver.cc
* Here it's used for language detection. It shouldn't be a problem to include ignored content.content/browser/web_contents/web_contents_android.cc
* No problem, it walks the tree using GetUnignoredChild* functions.I'm following up these changes in a new CL: https://chromium-review.googlesource.com/c/chromium/src/+/3309188
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Jacobo Aragunde Pérez (Gerrit)
Attention is currently required from: Aaron Leventhal.
1 comment:
File content/browser/renderer_host/render_widget_host_view_mac.mm:
Patch Set #16, Line 862:
GetWebContents()->RequestAXTreeSnapshot
As a recap, the main problem is the usage of AXNode->children() for tree traversal, which includes i […]
@Aaron, I've updated this CL to set a DCHECK whenever someone tries to access the name of a hidden node, and added exceptions to use it in some tests: https://chromium-review.googlesource.com/c/chromium/src/+/3304055
The results are quite interesting:
- Dump tests in content_browsertests failed. This is expected, as they seem to print the entire tree before narrowing it down to match the test expectations. I added an exception for these tests.
- PDFExtensionTests in browser_tests failed, too. These are suspicious because there were /linux tests failing, which shouldn't be dealing with hidden nodes. The backtraces indicate access through atk_object::GetName(). I added an exception for them, too, but it should be investigated...
- A couple of blink_web_tests failed, too, the backtraces are like in the previous case, see below. I don't know how to add an exception for them, because these kinds of tests harness the content_shell, if I'm not wrong, adding an exception there would probably be too broad.
#6 0x564d0f1a6eab ui::AXPlatformNodeBase::GetName()
#7 0x564d0f1bbcbb ui::(anonymous namespace)::atk_object::GetName()
#8 0x564d0f1bbbe0 ui::AXPlatformNodeAuraLinux::OnNameChanged()
#9 0x564d0f699f16 content::BrowserAccessibilityManager::OnAccessibilityEvents()
#10 0x564d0fca5def content::RenderFrameHostImpl::HandleAXEvents()All in all, this DCHECK seems to expose cases of ignored nodes getting their names accessed that we should investigate. That's even before landing this CL to include hidden nodes.
The purpose of the DCHECK was to detect breakage caused by this CL, but either we investigate the hits we already found or it won't be useful. My proposal, right now, would be to land this change without the DCHECK. We have already tracked down and evaluated/fixed the uses of RequestAXTreeSnapshot to minimize impact.
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Jonathan Ross, Jacobo Aragunde Pérez.
Patch set 23:Code-Review +1
3 comments:
Patchset:
LGTM! Great work.
File content/browser/renderer_host/render_widget_host_view_mac.mm:
Patch Set #16, Line 862:
GetWebContents()->RequestAXTreeSnapshot
@Aaron, I've updated this CL to set a DCHECK whenever someone tries to access the name of a hidden n […]
Yes, let's land asap without the DCHECK. Thanks for investigating. I don't think it's worth follow-ups to pursue investigation of the above tests usage of GetName() on invisible objects.
File third_party/blink/renderer/modules/accessibility/ax_object.cc:
Patch Set #23, Line 2853:
// naming and including them seems to have side effects, so we don't.Can you comment on the side effects here?
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Nektarios Paisios (Gerrit)
Attention is currently required from: Jonathan Ross, Jacobo Aragunde Pérez.
Patch set 23:Code-Review +1
9 comments:
Patchset:
LGTM
In order to speed things up I am approving this patch, but would you mind looking at some comments I left.
File third_party/blink/renderer/modules/accessibility/ax_object.cc:
Patch Set #23, Line 2849:
IsHiddenViaStyle()) {Is the IsHiddenViaStyle()method still needed, or can we get rid of it as well with this change?
Patch Set #23, Line 2854:
if (!node->HasTagName(html_names::kDatalistTag) &&That's not how we usually check if a node is of a particular HTML element type. We often do:
IsA<HTMLDataListElement>(node)
IsA<HTMLOptionElement>(node)
I think that we should avoid explicitly checking for tag names everywhere. Can someone search through our code for other instances?
I believe that a widget could be created that has a different tag name but which behaves like a built-in element, that's why an element (IsA<...>) check is safer.Patch Set #23, Line 2855:
!node->HasTagName(html_names::kOptionTag))Nit: Add braces because the condition spans two lines.
Patch Set #23, Line 2856:
return true;Couldn't we make this method virtual and handle returning false from the special subclasses we have for datalist / <select> and <option>.
Look in ax_list_box.cc and ax_listbox_option.cc.Should we return false here and get rid of the "} else {" line?
I don't think that objects that have a node but lack a layout object and which are either a data list element or an option element should be further processed, should they? Isn't that what the above "if" conditions ensure that we have at this stage.Patch Set #23, Line 2858:
} else {Nit: else { // GetLayoutObject() != nullptr.
Or get rid of the else completely as pointed above.
File third_party/blink/renderer/modules/accessibility/ax_selection_test.cc:
Patch Set #23, Line 485:
// will adjust positions to point to the first character of the text object.It is not the browser process that performs the adjustment, it is the AXPosition class in Blink. When a particular platform asks for the selection in the browser process, it is adjusted so that it doesn't point to ignored nodes, but I assume we could omit saying this here, could we?
File third_party/blink/web_tests/wpt_internal/display-lock/css-content-visibility/accessibility/content-visibility-accessibility-002.html:
Patch Set #23, Line 48:
// Note that it's still the same 5 children, but the whitespace nodes are marked ignoredI don't understand this: The nodes were unignored when the subtree was display locked, but when it is unlocked they become ignored. Why? Shouldn't they always be ignored, both when the subtree is locked and when it is unlocked?
Also, this comment applies to similar web tests in other files.
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Jacobo Aragunde Pérez (Gerrit)
Attention is currently required from: Nektarios Paisios, Jonathan Ross.
10 comments:
File content/browser/renderer_host/render_widget_host_view_mac.mm:
Patch Set #16, Line 862:
GetWebContents()->RequestAXTreeSnapshot
Yes, let's land asap without the DCHECK. Thanks for investigating. […]
This CL landed: https://chromium-review.googlesource.com/c/chromium/src/+/3309188
This was useful for testing, but we'll abandon it: https://chromium-review.googlesource.com/c/chromium/src/+/3304055
File third_party/blink/renderer/modules/accessibility/ax_object.cc:
Patch Set #23, Line 2849:
IsHiddenViaStyle()) {Is the IsHiddenViaStyle()method still needed, or can we get rid of it as well with this change?
It's still used to decide if a node should contribute to accname.
Patch Set #23, Line 2853:
// naming and including them seems to have side effects, so we don't.Can you comment on the side effects here?
I hope it's more clear now.
Patch Set #23, Line 2854:
if (!node->HasTagName(html_names::kDatalistTag) &&
That's not how we usually check if a node is of a particular HTML element type. We often do: […]
Thanks, I don't really know why I used a check like that, fixing.
There are a bunch of uses of HasTagName in blink accessibility: https://source.chromium.org/search?q=%22HasTagName(html_names::%22%20filepath:accessibility&ss=chromium%2Fchromium%2Fsrc
Writing down as a TODO.
Patch Set #23, Line 2855:
!node->HasTagName(html_names::kOptionTag))Nit: Add braces because the condition spans two lines.
Now the condition is only one line 😊
Patch Set #23, Line 2856:
return true;
Couldn't we make this method virtual and handle returning false from the special subclasses we have […]
That would affect some tests and this CL is pretty big, let me consider this for a separate CL.
Should we return false here and get rid of the "} else {" line? […]
This affects a number of tests. I think I'd rather do it separately, we already have a lot of changes in this CL.
Patch Set #23, Line 2858:
} else {
Nit: else { // GetLayoutObject() != nullptr. […]
Done
File third_party/blink/renderer/modules/accessibility/ax_selection_test.cc:
Patch Set #23, Line 485:
// will adjust positions to point to the first character of the text object.
It is not the browser process that performs the adjustment, it is the AXPosition class in Blink. […]
Done
File third_party/blink/web_tests/wpt_internal/display-lock/css-content-visibility/accessibility/content-visibility-accessibility-002.html:
Patch Set #23, Line 48:
// Note that it's still the same 5 children, but the whitespace nodes are marked ignored
I don't understand this: The nodes were unignored when the subtree was display locked, but when it i […]
I've modified the test comments and strings to make it hopefully easier to understand:
1. When the subtree was display locked, all 5 children were ignored.
2. When the subtree is unlocked, the non-whitespace nodes become unignored.As a side note, the non-whitespace nodes would not exist if the subtree was unlocked in first place.
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Jacobo Aragunde Pérez (Gerrit)
Attention is currently required from: Nektarios Paisios, Jonathan Ross.
Patch set 24:Commit-Queue +2
Jacobo Aragunde Pérez (Gerrit)
Attention is currently required from: Nektarios Paisios, Jonathan Ross, Jacobo Aragunde Pérez.
Patch set 24:Commit-Queue +2
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
23 is the latest approved patch-set.
The change was submitted with unreviewed changes in the following files:
```
The name of the file: content/test/data/accessibility/html/modal-dialog-stack.html
Insertions: 2, Deletions: 2.
The diff is too large to show. Please review the diff.
```
```
The name of the file: third_party/blink/renderer/modules/accessibility/ax_selection_test.cc
Insertions: 2, Deletions: 2.
The diff is too large to show. Please review the diff.
```
```
The name of the file: third_party/blink/renderer/modules/accessibility/ax_object.cc
Insertions: 10, Deletions: 6.
The diff is too large to show. Please review the diff.
```
```
The name of the file: content/test/data/accessibility/html/modal-dialog-stack-expected-blink.txt
Insertions: 2, Deletions: 2.
The diff is too large to show. Please review the diff.
```
```
The name of the file: third_party/blink/web_tests/wpt_internal/display-lock/css-content-visibility/accessibility/content-visibility-accessibility-002.html
Insertions: 8, Deletions: 7.
The diff is too large to show. Please review the diff.
```
Include hidden objects in accessible tree.
Include display:none, visibility:hidden and aria-hidden elements in
the accessible tree.
We plan to use them to streamline the calculation of accessible names,
so it does not require additional DOM or layout tree traversals. We
previously attempted to include only those hidden nodes which were
required for accname calculation, but there were complications to
keep the tree up-to-date. This is much easier to maintain and it
doesn't seem to have a noticeable performance impact.
A sizeable number of DumpAccessibilityTreeTest expectations were
updated to include the newly included hidden nodes.
Original author was aleve...@chromium.org in:
http://crrev.com/c/3197971
Bug: 1255036
Change-Id: I899781b654dd2b1dd97473b92e26d95fa54376bc
Cq-Include-Trybots: luci.chromium.try:win7-blink-rel,win7-rel
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3217730
Reviewed-by: Jonathan Ross <jon...@chromium.org>
Reviewed-by: Aaron Leventhal <aleve...@chromium.org>
Reviewed-by: Nektarios Paisios <nek...@chromium.org>
Commit-Queue: Jacobo Aragunde Pérez <jara...@igalia.com>
Cr-Commit-Position: refs/heads/main@{#949376}
M content/test/data/accessibility/aria/table-column-hidden-expected-blink.txt
M content/test/data/accessibility/html/meter-expected-blink.txt
M content/test/data/accessibility/html/svg-text-alternative-computation-expected-blink.txt
M content/test/data/accessibility/aria/hidden-labelled-by-expected-blink.txt
M content/test/data/accessibility/event/aria-hidden-single-descendant-visibility-hidden-expected-auralinux.txt
M third_party/blink/renderer/modules/accessibility/ax_object.cc
M content/test/data/accessibility/event/add-dialog-no-info-expected-uia-win7.txt
M content/test/data/accessibility/html/custom-element-with-aria-owns-inside-expected-blink.txt
M content/test/data/accessibility/aria/aria-undefined-literal-expected-blink.txt
M third_party/blink/web_tests/http/tests/inspector-protocol/accessibility/accessibility-query-axtree-expected.txt
M content/test/data/accessibility/css/alt-text-expected-blink.txt
M content/test/data/accessibility/event/add-dialog-described-by-expected-uia-win.txt
M content/test/data/accessibility/html/summary-expected-blink.txt
M content/test/data/accessibility/html/modal-dialog-stack-expected-blink.txt
M content/test/data/accessibility/aria/aria-hidden-expected-blink.txt
M content/test/data/accessibility/event/add-dialog-expected-win.txt
M content/test/data/accessibility/aria/aria-hidden-single-descendant-visibility-hidden-expected-blink.txt
M content/test/data/accessibility/aria/hidden-expected-blink.txt
M third_party/blink/web_tests/http/tests/inspector-protocol/accessibility/accessibility-ignoredNodes-expected.txt
M content/test/data/accessibility/html/svg-title-in-group-expected-blink.txt
M third_party/blink/web_tests/http/tests/inspector-protocol/accessibility/accessibility-getFullAXTree-display-locked-expected.txt
M content/test/data/accessibility/aria/aria-empty-string-expected-blink.txt
M content/test/data/accessibility/regression/content-visibility-label-expected-blink.txt
M content/test/data/accessibility/html/input-suggestions-source-element-expected-blink.txt
M content/test/data/accessibility/html/modal-dialog-stack.html
M third_party/blink/web_tests/wpt_internal/display-lock/css-content-visibility/accessibility/content-visibility-accessibility-001.html
M content/test/data/accessibility/regression/missing-parent-expected-blink.txt
M content/test/data/accessibility/html/landmark-expected-blink.txt
M content/test/data/accessibility/event/add-dialog-expected-uia-win.txt
M third_party/blink/renderer/modules/accessibility/ax_selection_test.cc
M content/test/data/accessibility/aria/aria-owns-crash-expected-blink.txt
M content/test/data/accessibility/event/add-dialog-no-info-expected-uia-win.txt
M content/test/data/accessibility/aria/aria-undefined-expected-blink.txt
M content/test/data/accessibility/event/add-dialog-described-by-expected-uia-win7.txt
M third_party/blink/web_tests/wpt_internal/display-lock/css-content-visibility/accessibility/content-visibility-accessibility-002.html
M content/test/data/accessibility/css/visibility-expected-blink.txt
M content/test/data/accessibility/css/content-visibility-auto-aria-hidden-expected-blink.txt
M third_party/blink/web_tests/wpt_internal/display-lock/css-content-visibility/accessibility/content-visibility-accessibility-008.html
M content/test/data/accessibility/regression/title-in-shadow-expected-blink.txt
M content/test/data/accessibility/aria/aria-hidden-single-descendant-display-none-expected-blink.txt
M content/test/data/accessibility/html/input-list-expected-blink.txt
M content/test/data/accessibility/html/table-column-remove-expected-blink.txt
M third_party/blink/web_tests/wpt_internal/display-lock/css-content-visibility/accessibility/content-visibility-accessibility-003.html
M content/test/data/accessibility/regression/slot-creation-crash-expected-blink.txt
M content/test/data/accessibility/html/custom-element-hidden-expected-blink.txt
M content/test/data/accessibility/display-locking/non-activatable-expected-blink.txt
M content/test/data/accessibility/html/svg-expected-blink.txt
M content/test/data/accessibility/html/svg-with-clickable-rect-expected-blink.txt
M content/test/data/accessibility/event/add-dialog-described-by-expected-win.txt
M content/test/data/accessibility/html/open-modal-expected-blink.txt
M content/test/data/accessibility/html/ruby-expected-blink.txt
77 files changed, 440 insertions(+), 97 deletions(-)
Jacobo Aragunde Pérez (Gerrit)
Jacobo Aragunde Pérez has created a revert of this change.
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.
Jacobo Aragunde Pérez (Gerrit)
1 comment:
File third_party/blink/renderer/modules/accessibility/ax_object.cc:
Patch Set #23, Line 2854:
if (!node->HasTagName(html_names::kDatalistTag) &&Thanks, I don't really know why I used a check like that, fixing. […]
Follow-up at: https://crrev.com/c/3423828
Jacobo Aragunde Pérez (Gerrit)
2 comments:
File third_party/blink/renderer/modules/accessibility/ax_object.cc:
Patch Set #23, Line 2856:
return true;
That would affect some tests and this CL is pretty big, let me consider this for a separate CL.
Follow-up: https://crrev.com/c/3430820
This affects a number of tests. […]
Follow-up: https://crrev.com/c/3430820
To view, visit change 3217730. To unsubscribe, or for help writing mail filters, visit settings.