Address TODO(accessibility) things in ax_object.cc [chromium/src : main]
Mason Freed (Gerrit)
Attention is currently required from: Aaron Leventhal, Mark Schillaci.
Mason Freed would like Aaron Leventhal and Mark Schillaci to review this change.
Address TODO(accessibility) things in ax_object.cc
There are several TODO(accessibility) comments in ax_object.cc related
to the KeyboardFocusableScrollers feature. This CL implements those
TODOs.
Bug: 1444450
Change-Id: I21f99f3390a4a570698b18904af1b677d8177115
---
M content/browser/accessibility/dump_accessibility_tree_browsertest.cc
M content/public/android/javatests/src/org/chromium/content/browser/accessibility/WebContentsAccessibilityTest.java
M content/test/content_test_bundle_data.filelist
M content/test/data/accessibility/event/scroll-horizontal-scroll-percent-change-expected-win.txt
M content/test/data/accessibility/event/scroll-vertical-scroll-percent-change-expected-win.txt
M content/test/data/accessibility/html/overflow-actions-expected-android-external.txt
M content/test/data/accessibility/html/overflow-actions-expected-android.txt
A content/test/data/accessibility/html/overflow-scroller-expected-blink.txt
A content/test/data/accessibility/html/overflow-scroller.html
M content/test/data/accessibility/html/scrollable-expected-android-external.txt
M content/test/data/accessibility/html/scrollable-overflow-expected-android-external.txt
M content/test/data/accessibility/html/scrollable-overflow-expected-android.txt
M content/test/data/accessibility/html/scrollable-overflow-expected-auralinux.txt
M content/test/data/accessibility/html/scrollable-overflow-expected-blink.txt
M third_party/blink/renderer/core/dom/element.cc
M third_party/blink/renderer/core/dom/element.h
M third_party/blink/renderer/modules/accessibility/ax_object.cc
M third_party/blink/web_tests/http/tests/inspector-protocol/accessibility/accessibility-modal-expected.txt
18 files changed, 75 insertions(+), 85 deletions(-)
To view, visit change 4813829. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Aaron Leventhal, Mark Schillaci.
1 comment:
Patchset:
Moving a11y reviews of the behavioral changes over to this CL.
To view, visit change 4813829. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Mark Schillaci, Mason Freed.
3 comments:
Patchset:
Couple of questions, but I don't need to hold it up for the answers.
File content/test/data/accessibility/html/scrollable-overflow-expected-blink.txt:
Patch Set #12, Line 28:
++++++++genericContainer className='x-hidden-large scrollable' scrollXMin=0 scrollable=trueSo it looks like we're losing the accessible names for scrollable areas. Is that intentional? I think it's actually ok, as these names tend to be very long and probably not very useful. However, it would be good if we exposed the fact that the region is scrollable in some way. For example, we could expose an object attribute.
I don't expect you to figure all that out. I'll file an issue on HTML-AAM.
File third_party/blink/renderer/modules/accessibility/ax_object.cc:
Patch Set #12, Line 4136:
bool AXObject::IsKeyboardFocusable() const {Should we have a TODO to eventually replace this with Element::IsKeyboardFocusable(). Or did you determine that's basically impossible? I haven't kept up with the earlier test failures and frustrations with doing it, so it would be good to get a summary on the state of things.
To view, visit change 4813829. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Mark Schillaci, Mason Freed.
Patch set 12:Code-Review +1
Aaron Leventhal (Gerrit)
Attention is currently required from: Mark Schillaci, Mason Freed.
1 comment:
File content/test/data/accessibility/html/scrollable-overflow-expected-blink.txt:
Patch Set #12, Line 28:
++++++++genericContainer className='x-hidden-large scrollable' scrollXMin=0 scrollable=true
So it looks like we're losing the accessible names for scrollable areas. […]
Here's the HTML-AAM GitHub issue (it was already filed actually):
https://github.com/w3c/html-aam/issues/358
To view, visit change 4813829. To unsubscribe, or for help writing mail filters, visit settings.
Mark Schillaci (Gerrit)
Attention is currently required from: Mason Freed.
Patch set 12:Code-Review +1
1 comment:
Patchset:
LGTM!
To view, visit change 4813829. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Aaron Leventhal, Mark Schillaci.
3 comments:
Patchset:
Thanks to both of you!
File content/test/data/accessibility/html/scrollable-overflow-expected-blink.txt:
Patch Set #12, Line 28:
++++++++genericContainer className='x-hidden-large scrollable' scrollXMin=0 scrollable=true
Here's the HTML-AAM GitHub issue (it was already filed actually): […]
Thanks for filing the issue and maybe taking care of that. Does the `scrollable=true` here not indicate that the region is scrollable? I'm not sure why the name goes away when it becomes focusable.
File third_party/blink/renderer/modules/accessibility/ax_object.cc:
Patch Set #12, Line 4136:
bool AXObject::IsKeyboardFocusable() const {
Should we have a TODO to eventually replace this with Element::IsKeyboardFocusable(). […]
Yeah I had that TODO in my head, but better to add a comment here. Done.
I think it should be possible, but I learned from my fights with the code on these CLs that it should be attempted separately from this CL. 😊
To view, visit change 4813829. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Aaron Leventhal, Mark Schillaci.
1 comment:
Patchset:
And due to a rebase mistake, I cleared both of your +1's. Sorry about that - could I get one more?
To view, visit change 4813829. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
File content/test/data/accessibility/html/scrollable-overflow-expected-blink.txt:
Patch Set #12, Line 28:
++++++++genericContainer className='x-hidden-large scrollable' scrollXMin=0 scrollable=true
Thanks for filing the issue and maybe taking care of that. […]
The name goes away because AXObject::SupportsNameFromContents() no longer returns what it did.
I think we should probably just make this a refactoring CL. I'll deal with the name issue separately.
The scrollable=true is in the expected-blink.txt, but afaik there is nothing in the various platform APIs (e.g. expected-win.txt).
To view, visit change 4813829. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
File third_party/blink/renderer/modules/accessibility/ax_object.cc:
Patch Set #12, Line 7300:
AOMRelationProperty::kActiveDescendant)) {Mason, the test change is caused by this. We no longer are returning true for the scrollable elements.
I think we should restore that behavior before landing.
To view, visit change 4813829. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Mark Schillaci, Mason Freed.
Mason Freed uploaded patch set #16 to this change.
Address TODO(accessibility) things in ax_object.cc
There are several TODO(accessibility) comments in ax_object.cc related
to the KeyboardFocusableScrollers feature. This CL implements those
TODOs.
Note that this undoes some of the test changes made in [1].
[1] https://chromium-review.googlesource.com/c/chromium/src/+/4194963
Bug: 1444450
Change-Id: I21f99f3390a4a570698b18904af1b677d8177115
---
M content/browser/accessibility/dump_accessibility_tree_browsertest.cc
M content/public/android/javatests/src/org/chromium/content/browser/accessibility/WebContentsAccessibilityTest.java
M content/test/content_test_bundle_data.filelist
M content/test/data/accessibility/event/scroll-horizontal-scroll-percent-change-expected-win.txt
M content/test/data/accessibility/event/scroll-vertical-scroll-percent-change-expected-win.txt
M content/test/data/accessibility/html/overflow-actions-expected-android-external.txt
M content/test/data/accessibility/html/overflow-actions-expected-android.txt
A content/test/data/accessibility/html/overflow-scroller-expected-blink.txt
A content/test/data/accessibility/html/overflow-scroller.html
M content/test/data/accessibility/html/scrollable-expected-android-external.txt
M content/test/data/accessibility/html/scrollable-overflow-expected-android-external.txt
M content/test/data/accessibility/html/scrollable-overflow-expected-android.txt
M content/test/data/accessibility/html/scrollable-overflow-expected-auralinux.txt
M content/test/data/accessibility/html/scrollable-overflow-expected-blink.txt
M third_party/blink/renderer/core/dom/element.cc
M third_party/blink/renderer/modules/accessibility/ax_object.cc
M third_party/blink/web_tests/http/tests/inspector-protocol/accessibility/accessibility-modal-expected.txt
M third_party/blink/web_tests/shadow-dom/focus-navigation/focus-navigation-scroller-delegatesFocus.html
M third_party/blink/web_tests/shadow-dom/focus-navigation/focus-navigation-scroller-tabindex.html
19 files changed, 82 insertions(+), 62 deletions(-)
To view, visit change 4813829. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Aaron Leventhal, Mark Schillaci.
3 comments:
Patchset:
Alright, I reverted the one requested part, and rebaselined all of the tests. I believe they should all pass at this point, but we'll see.
File content/test/data/accessibility/html/scrollable-overflow-expected-blink.txt:
Patch Set #12, Line 28:
++++++++genericContainer className='x-hidden-large scrollable' scrollXMin=0 scrollable=true
The name goes away because AXObject::SupportsNameFromContents() no longer returns what it did. […]
Done
File third_party/blink/renderer/modules/accessibility/ax_object.cc:
Patch Set #12, Line 7300:
AOMRelationProperty::kActiveDescendant)) {
Mason, the test change is caused by this. […]
Done
To view, visit change 4813829. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Patchset:
Change to SupportsNameFromFocus() looks good, but I'll wait for the final green before +1.
To view, visit change 4813829. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Aaron Leventhal, Mark Schillaci.
2 comments:
Patchset:
Change to SupportsNameFromFocus() looks good, but I'll wait for the final green before +1.
Thanks. Last patchset should clear all but one test.
File content/public/android/javatests/src/org/chromium/content/browser/accessibility/WebContentsAccessibilityTest.java:
Patch Set #18, Line 1886:
ViewIdResourceNameMatcher, "li1mschillaci@, any thoughts on this test failure? I tried the trick you suggested for the other test, switching to `sViewIdResourceNameMatcher`, but it didn't appear to fix the problem. The test fails on line 1886 - can't find the element. Ideas appreciated.
To view, visit change 4813829. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Aaron Leventhal, Mark Schillaci.
1 comment:
File content/public/android/javatests/src/org/chromium/content/browser/accessibility/WebContentsAccessibilityTest.java:
Patch Set #18, Line 1886:
ViewIdResourceNameMatcher, "li1
mschillaci@, any thoughts on this test failure? I tried the trick you suggested for the other test, […]
Yep, this is the only remaining failing test. It was triggered by this change to SupportsNameFromContents():
https://chromium-review.googlesource.com/c/chromium/src/+/4813829/14..15
which now returns true for the `<div class="Footer">` in `hide_visible_elements_with_css.html` because it's a scroller. Given that the test seems to just be about "invisible" elements, I changed the `overflow:auto` to `overflow:hidden` which should hopefully fix it. Let me know if that works for you.
To view, visit change 4813829. To unsubscribe, or for help writing mail filters, visit settings.
Mark Schillaci (Gerrit)
Attention is currently required from: Aaron Leventhal, Mason Freed.
1 comment:
File content/public/android/javatests/src/org/chromium/content/browser/accessibility/WebContentsAccessibilityTest.java:
Patch Set #18, Line 1886:
ViewIdResourceNameMatcher, "li1
Yep, this is the only remaining failing test. […]
I'm not sure if `overflow:hidden` would be testing the same thing. This is testing that nodes in the scroll view do not get scroll actions, and only the container gets the scroll actions. If we make it hidden, I'm not sure we still populate the nodes in the Java-side tree.
To view, visit change 4813829. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
Attention is currently required from: Aaron Leventhal, Mark Schillaci.
1 comment:
File content/public/android/javatests/src/org/chromium/content/browser/accessibility/WebContentsAccessibilityTest.java:
Patch Set #18, Line 1886:
ViewIdResourceNameMatcher, "li1
I'm not sure if `overflow:hidden` would be testing the same thing. […]
Oh ok, I was just basing this idea on the name of the test.
Do you have any other ideas I can try? (I have none, and I'm blocked on this one test.)
To view, visit change 4813829. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
File content/public/android/javatests/src/org/chromium/content/browser/accessibility/WebContentsAccessibilityTest.java:
Patch Set #18, Line 1886:
ViewIdResourceNameMatcher, "li1
Oh ok, I was just basing this idea on the name of the test. […]
Ok, instead of `overflow:hidden` I added `tabindex` to one of the descendants to avoid the new focusable scroller behavior.
To view, visit change 4813829. To unsubscribe, or for help writing mail filters, visit settings.
Aaron Leventhal (Gerrit)
Attention is currently required from: Mark Schillaci, Mason Freed.
Patch set 23:Code-Review +1
Mark Schillaci (Gerrit)
Attention is currently required from: Mason Freed.
Patch set 23:Code-Review +1
1 comment:
Patchset:
LGTM!
To view, visit change 4813829. To unsubscribe, or for help writing mail filters, visit settings.
Mason Freed (Gerrit)
To view, visit change 4813829. To unsubscribe, or for help writing mail filters, visit settings.
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted this change.
Address TODO(accessibility) things in ax_object.cc
There are several TODO(accessibility) comments in ax_object.cc related
to the KeyboardFocusableScrollers feature. This CL implements those
TODOs.
Note that this undoes some of the test changes made in [1].
[1] https://chromium-review.googlesource.com/c/chromium/src/+/4194963
Bug: 1444450
Change-Id: I21f99f3390a4a570698b18904af1b677d8177115
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/4813829
Reviewed-by: Mark Schillaci <mschi...@google.com>
Reviewed-by: Aaron Leventhal <aleve...@chromium.org>
Commit-Queue: Mason Freed <mas...@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1190286}
---
M content/browser/accessibility/dump_accessibility_tree_browsertest.cc
M content/public/android/javatests/src/org/chromium/content/browser/accessibility/WebContentsAccessibilityTest.java
M content/test/content_test_bundle_data.filelist
M content/test/data/accessibility/event/scroll-horizontal-scroll-percent-change-expected-win.txt
M content/test/data/accessibility/event/scroll-vertical-scroll-percent-change-expected-win.txt
M content/test/data/accessibility/html/overflow-actions-expected-android-external.txt
M content/test/data/accessibility/html/overflow-actions-expected-android.txt
A content/test/data/accessibility/html/overflow-scroller-expected-blink.txt
A content/test/data/accessibility/html/overflow-scroller.html
M content/test/data/accessibility/html/scrollable-expected-android-external.txt
M content/test/data/accessibility/html/scrollable-overflow-expected-android-external.txt
M content/test/data/accessibility/html/scrollable-overflow-expected-android.txt
M content/test/data/android/hide_visible_elements_with_css.html
M third_party/blink/renderer/core/dom/element.cc
M third_party/blink/renderer/modules/accessibility/ax_object.cc
M third_party/blink/web_tests/http/tests/inspector-protocol/accessibility/accessibility-modal-expected.txt
M third_party/blink/web_tests/shadow-dom/focus-navigation/focus-navigation-scroller-delegatesFocus.html
M third_party/blink/web_tests/shadow-dom/focus-navigation/focus-navigation-scroller-tabindex.html
18 files changed, 70 insertions(+), 66 deletions(-)
Zijie He (Gerrit)
1 comment:
Patchset:
I think this change broke Fuchsia: https://ci.chromium.org/ui/p/chromium/builders/ci/fuchsia-arm64-cast-receiver-rel/8021/overview.
To view, visit change 4813829. To unsubscribe, or for help writing mail filters, visit settings.
Tim (Gerrit)
1 comment:
Patchset:
Yup, I was literally just putting in the revert. Thanks for the heads up.
To view, visit change 4813829. To unsubscribe, or for help writing mail filters, visit settings.
Tim (Gerrit)
Tim has created a revert of this change.
To view, visit change 4813829. To unsubscribe, or for help writing mail filters, visit settings.
Zijie He (Gerrit)
Yup, I was literally just putting in the revert. Thanks for the heads up.
Thank you.
To view, visit change 4813829. To unsubscribe, or for help writing mail filters, visit settings.