[<install> Element] Add button accessibility role to <install> [chromium/src : main]
Kristin Lee (Gerrit)
Message from Kristin Lee![]( "Open in Gerrit")
Set Ready For Review
Related details
- Kristin Lee
- Lia Hiscock
- Lu Huang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lu Huang (Gerrit)
Lu Huang added 1 comment![]( "Open in Gerrit")
What do Narrator and MacOS Voiceover say now? Let's compare those against what was documented as the before behavior in https://issues.chromium.org/issues/496947072.
Related details
- Kristin Lee
- Lia Hiscock
Lu Huang (Gerrit)
Lu Huang voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Kristin Lee
- Lia Hiscock
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kristin Lee (Gerrit)
Kristin Lee added 1 comment![]( "Open in Gerrit")
What do Narrator and MacOS Voiceover say now? Let's compare those against what was documented as the before behavior in https://issues.chromium.org/issues/496947072.
I have confirmed that Narrator announces the expected role, "button", now just like for <geolocation>. I have a gif of it that shows NarratorBuddy (though the text in NarratorBuddy is a little hard to read): https://drive.google.com/file/d/1PDgR52egVdC0BEEi34Mt0vfXD9X25dPY/view?usp=drive_link
Let me get back to you on MacOS VO.
Related details
- Lia Hiscock
Kristin Lee (Gerrit)
Kristin Lee added 1 comment![]( "Open in Gerrit")
This is ready for review:
Mike - Could you please help look at t/b/r/c/h/html_install_element_test.cc.
Kurt - Could you please help look at t/b/r/m/a/ax_node_object.cc. Feel free to redirect or let me know if you have any questions.
Thanks!
Related details
- Kurt Catti-Schmidt
- Lia Hiscock
- Mike West
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kristin Lee (Gerrit)
Kristin Lee added 1 comment![]( "Open in Gerrit")
This is ready for review:
Mike - Could you please help look at t/b/r/c/h/html_install_element_test.cc.
Kurt - Could you please help look at t/b/r/m/a/ax_node_object.cc. Feel free to redirect or let me know if you have any questions.Thanks!
Apologies - it turns out @mk...@chromium.org is an owner for both files so it'd be great if Mike could help review both.
Kurt, you're welcome to still review but I'll remove you from the attention set.
Thanks!
Related details
- Mike West
Kurt Catti-Schmidt (Gerrit)
Kurt Catti-Schmidt added 2 comments![]( "Open in Gerrit")
Moving myself to CC and adding Ben and Jacques
AXContext ax_context(GetDocument(), ui::kAXModeDefaultForTests);
HTMLInstallElement* element =
MakeGarbageCollected<HTMLInstallElement>(GetDocument());
WaitForElementRegistration(element);
GetDocument().View()->UpdateAllLifecyclePhasesForTest();
auto* ax_object_cache = GetDocument().ExistingAXObjectCache();
ASSERT_NE(nullptr, ax_object_cache);
EXPECT_EQ("button", ax_object_cache->ComputedRoleForNode(element));This is probably the wrong place to test this. You want an accessibility dump test like what was added for `HTMLGeolocationElement` (see https://chromium-review.googlesource.com/c/chromium/src/+/5463501).
Related details
- Kristin Lee
- Mike West
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jacques Newman (Gerrit)
Jacques Newman voted and added 2 comments![]( "Open in Gerrit")
Votes added by Jacques Newman
| Code-Review | +1 |
2 comments
this change lgtm with tests added, thanks!
AXContext ax_context(GetDocument(), ui::kAXModeDefaultForTests);
HTMLInstallElement* element =
MakeGarbageCollected<HTMLInstallElement>(GetDocument());
WaitForElementRegistration(element);
GetDocument().View()->UpdateAllLifecyclePhasesForTest();
auto* ax_object_cache = GetDocument().ExistingAXObjectCache();
ASSERT_NE(nullptr, ax_object_cache);
EXPECT_EQ("button", ax_object_cache->ComputedRoleForNode(element));This is probably the wrong place to test this. You want an accessibility dump test like what was added for `HTMLGeolocationElement` (see https://chromium-review.googlesource.com/c/chromium/src/+/5463501).
+1, and I would also add a (tentative) WPT, WPTs can check role so this should be pretty straightforward.
Related details
- Kristin Lee
- Mike West
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kristin Lee (Gerrit)
Kristin Lee added 1 comment![]( "Open in Gerrit")
AXContext ax_context(GetDocument(), ui::kAXModeDefaultForTests);
HTMLInstallElement* element =
MakeGarbageCollected<HTMLInstallElement>(GetDocument());
WaitForElementRegistration(element);
GetDocument().View()->UpdateAllLifecyclePhasesForTest();
auto* ax_object_cache = GetDocument().ExistingAXObjectCache();
ASSERT_NE(nullptr, ax_object_cache);
EXPECT_EQ("button", ax_object_cache->ComputedRoleForNode(element));Jacques NewmanThis is probably the wrong place to test this. You want an accessibility dump test like what was added for `HTMLGeolocationElement` (see https://chromium-review.googlesource.com/c/chromium/src/+/5463501).
+1, and I would also add a (tentative) WPT, WPTs can check role so this should be pretty straightforward.
Thanks for the feedback! I'll get those tests started and will reach back out when they're ready for a review.
Related details
- Mike West
Krzysztof (Gerrit)
Krzysztof added 1 comment![]( "Open in Gerrit")
- Kristin Lee
- Mike West
Benjamin Beaudry (Gerrit)
Benjamin Beaudry added 2 comments![]( "Open in Gerrit")
Thanks for working on this!
if (IsA<HTMLUserMediaElement>(node)) {
return ax::mojom::blink::Role::kButton;
}
if (IsA<HTMLGeolocationElement>(node)) {
return ax::mojom::blink::Role::kButton;
}
if (IsA<HTMLInstallElement>(node)) {
return ax::mojom::blink::Role::kButton;
}Drive-by comment: All three of HTMLUserMediaElement, HTMLGeolocationElement, and HTMLInstallElement are a subclass of HTMLCapabilityElementBase, and all three are exposed as buttons. If HTMLCapabilityElementBase always semantically maps to a button role, should we consider replacing those three separate conditions by:
```suggestion
if (IsA<HTMLCapabilityElementBase>(node)) {
return ax::mojom::blink::Role::kButton;
}
```
Related details
- Kristin Lee
- Mike West
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kristin Lee (Gerrit)
Kristin Lee added 4 comments![]( "Open in Gerrit")
Thanks for the feedback! I added new tests and changes.
@mk...@chromium.org - Since I'm no longer changing `third_party/blink/renderer/core/html/html_install_element_test.cc` I'll move you to CC, but still feel free to provide feedback. As an FYI, I added a WPT that checks the accessibility role for all the current capability elements.
data/accessibility/aria/apg-patterns/aria-accordion-expected-android-external.txtI had a presubmit error to regenerate the filelist even after I rebased. Let me know if this isn't quite expected to be part of this kind of CL.
** Presubmit ERRORS: 1 **
Filelist needs to be re-generated. Please run 'python3 E:\chr\src\build\ios\update_bundle_filelist.py E:\chr\src\content\test\content_test_bundle_data.filelist E:\chr\src\content\test\content_test_bundle_data.globlist E:\chr\src\content\test\.' and include the changes in this CL
AXContext ax_context(GetDocument(), ui::kAXModeDefaultForTests);
HTMLInstallElement* element =
MakeGarbageCollected<HTMLInstallElement>(GetDocument());
WaitForElementRegistration(element);
GetDocument().View()->UpdateAllLifecyclePhasesForTest();
auto* ax_object_cache = GetDocument().ExistingAXObjectCache();
ASSERT_NE(nullptr, ax_object_cache);
EXPECT_EQ("button", ax_object_cache->ComputedRoleForNode(element));Jacques NewmanThis is probably the wrong place to test this. You want an accessibility dump test like what was added for `HTMLGeolocationElement` (see https://chromium-review.googlesource.com/c/chromium/src/+/5463501).
Kristin Lee+1, and I would also add a (tentative) WPT, WPTs can check role so this should be pretty straightforward.
Thanks for the feedback! I'll get those tests started and will reach back out when they're ready for a review.
I've added the new tests, PTAL when you're free and let me know if they don't look as expected. I was following common patterns in
third_party/blink/web_tests/external/wpt/html/semantics/permission-element/*: https://chromium-review.googlesource.com/c/chromium/src/+/7368050
if (IsA<HTMLUserMediaElement>(node)) {
return ax::mojom::blink::Role::kButton;
}
if (IsA<HTMLGeolocationElement>(node)) {
return ax::mojom::blink::Role::kButton;
}
if (IsA<HTMLInstallElement>(node)) {
return ax::mojom::blink::Role::kButton;
}Drive-by comment: All three of HTMLUserMediaElement, HTMLGeolocationElement, and HTMLInstallElement are a subclass of HTMLCapabilityElementBase, and all three are exposed as buttons. If HTMLCapabilityElementBase always semantically maps to a button role, should we consider replacing those three separate conditions by:
```suggestion
if (IsA<HTMLCapabilityElementBase>(node)) {
return ax::mojom::blink::Role::kButton;
}
```
Great idea. Added and confirmed this works for all 3 elements, thanks!
Related details
- Benjamin Beaudry
- Jacques Newman
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jacques Newman (Gerrit)
Jacques Newman voted and added 2 comments![]( "Open in Gerrit")
Votes added by Jacques Newman
| Code-Review | +1 |
2 comments
data/accessibility/aria/apg-patterns/aria-accordion-expected-android-external.txtI had a presubmit error to regenerate the filelist even after I rebased. Let me know if this isn't quite expected to be part of this kind of CL.
** Presubmit ERRORS: 1 **
Filelist needs to be re-generated. Please run 'python3 E:\chr\src\build\ios\update_bundle_filelist.py E:\chr\src\content\test\content_test_bundle_data.filelist E:\chr\src\content\test\content_test_bundle_data.globlist E:\chr\src\content\test\.' and include the changes in this CL
Its expected and normal for this to need to be updated, but the amount of diff here looks a little strange, I would have expected only a single entry for the new test, not this big re-order. It looks like this just alphabetized, so this isn't particularly concerning.
Related details
- Benjamin Beaudry
- Kristin Lee
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
Jacques Newman (Gerrit)
Jacques Newman added 1 comment![]( "Open in Gerrit")
AXContext ax_context(GetDocument(), ui::kAXModeDefaultForTests);
HTMLInstallElement* element =
MakeGarbageCollected<HTMLInstallElement>(GetDocument());
WaitForElementRegistration(element);
GetDocument().View()->UpdateAllLifecyclePhasesForTest();
auto* ax_object_cache = GetDocument().ExistingAXObjectCache();
ASSERT_NE(nullptr, ax_object_cache);
EXPECT_EQ("button", ax_object_cache->ComputedRoleForNode(element));Jacques NewmanThis is probably the wrong place to test this. You want an accessibility dump test like what was added for `HTMLGeolocationElement` (see https://chromium-review.googlesource.com/c/chromium/src/+/5463501).
Kristin Lee+1, and I would also add a (tentative) WPT, WPTs can check role so this should be pretty straightforward.
Kristin LeeThanks for the feedback! I'll get those tests started and will reach back out when they're ready for a review.
I've added the new tests, PTAL when you're free and let me know if they don't look as expected. I was following common patterns in
third_party/blink/web_tests/external/wpt/html/semantics/permission-element/*: https://chromium-review.googlesource.com/c/chromium/src/+/7368050
- Benjamin Beaudry
- Kristin Lee
- Kurt Catti-Schmidt
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
Exportable changes to web-platform-tests were detected in this CL and a pull request in the upstream repo has been made: https://github.com/web-platform-tests/wpt/pull/59470.
When this CL lands, the bot will automatically merge the PR on GitHub if the required GitHub checks pass; otherwise, ecosystem-infra@ team will triage the failures and may contact you.
WPT Export docs:
https://chromium.googlesource.com/chromium/src/+/main/docs/testing/web_platform_tests.md#Automatic-export-process
Benjamin Beaudry (Gerrit)
Benjamin Beaudry voted and added 1 comment![]( "Open in Gerrit")
Votes added by Benjamin Beaudry
| Code-Review | +1 |
1 comment
- Kristin Lee
- Kurt Catti-Schmidt
Kristin Lee (Gerrit)
Kristin Lee (Gerrit)
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[<install> Element] Add button accessibility role to <install>
This CL adds the missing explicit accessibility role mapping that was
causing the role to default to kGenericContainer (announced as "group"
by screen readers), instead of the expected role, button.
Feature flags - InstallElement
- M content/browser/accessibility/dump_accessibility_tree_browsertest.cc
- M content/test/content_test_bundle_data.filelist
- A content/test/data/accessibility/html/install-expected-blink.txt
- A content/test/data/accessibility/html/install.html
- M third_party/blink/renderer/modules/accessibility/ax_node_object.cc
- A third_party/blink/web_tests/external/wpt/html/semantics/permission-element/roles.tentative.html
Code-Review: +1 by Benjamin Beaudry, +1 by Jacques Newman
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Blink W3C Test Autoroller (Gerrit)
Message from Blink W3C Test Autoroller![]( "Open in Gerrit")
The WPT PR for this CL has been merged upstream! https://github.com/web-platform-tests/wpt/pull/59470
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |