[Platform-provided behaviors] Accessibility integration [chromium/src : main]
Ana Sollano Kim (Gerrit)
Ana Sollano Kim voted and added 1 comment![]( "Open in Gerrit")
Votes added by Ana Sollano Kim
| Commit-Queue | +1 |
1 comment
Related details
- Ana Sollano Kim
- Dan Clark
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jacques Newman (Gerrit)
Jacques Newman added 2 comments![]( "Open in Gerrit")
// Check for `HTMLSubmitButtonBehavior`'s default role.While we only have this one for now, the [explainer](https://github.com/MicrosoftEdge/MSEdgeExplainers/blob/main/PlatformProvidedBehaviors/explainer.md#compatible-behaviors) lays out the ability for multiple behaviors with different roles and for conflict resolution:
> **Role conflict and accessibility implications:** `HTMLButtonBehavior` provides `role="button"` while `HTMLAnchorBehavior` provides `role="link"`. Under the last-in-wins rule, the element would have `role="link"` (or `role="button"` if the order is reversed). ...
Where do you expect this conflict resolution to live? Should there be something more like `ElementInternals::BehaviorBasedDefaultRole`?
With the current implementation, this information (which behavior was added last) doesn't seem readily available. (I could be wrong here.)
I would be *OK* to land as-is if we add a big TODO under ElementBehavior::DefaultRole saying this conflict resolution **MUST** be implemented before adding any new behaviors that override this method.
if (auto* submit_behavior =
GetElement() ? GetElement()->SubmitBehavior() : nullptr) {
const AtomicString& role = submit_behavior->DefaultAriaRole();
if (!role.empty()) {
ax::mojom::blink::Role internal_role = AriaRoleToInternalRole(role);
if (internal_role != ax::mojom::blink::Role::kUnknown) {
return internal_role;
}
}
}Why not have this return a ax::mojom::blink::Role instead of a string? This way you don't need to deal with strings in each behavior's override of DefaultAriaRole, or run the risk that you could ever get it wrong.
AXObject::AriaRoleName could facilitate reflecting out a string if we ever need it.
```suggestion
if (auto* submit_behavior =
GetElement() ? GetElement()->SubmitBehavior() : nullptr) {
return submit_behavior->DefaultAriaRole();
}
```
Related details
- Ana Sollano Kim
- Dan Clark
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. |
Ana Sollano Kim (Gerrit)
Ana Sollano Kim added 2 comments![]( "Open in Gerrit")
// Check for `HTMLSubmitButtonBehavior`'s default role.While we only have this one for now, the [explainer](https://github.com/MicrosoftEdge/MSEdgeExplainers/blob/main/PlatformProvidedBehaviors/explainer.md#compatible-behaviors) lays out the ability for multiple behaviors with different roles and for conflict resolution:
> **Role conflict and accessibility implications:** `HTMLButtonBehavior` provides `role="button"` while `HTMLAnchorBehavior` provides `role="link"`. Under the last-in-wins rule, the element would have `role="link"` (or `role="button"` if the order is reversed). ...Where do you expect this conflict resolution to live? Should there be something more like `ElementInternals::BehaviorBasedDefaultRole`?
With the current implementation, this information (which behavior was added last) doesn't seem readily available. (I could be wrong here.)
I would be *OK* to land as-is if we add a big TODO under ElementBehavior::DefaultRole saying this conflict resolution **MUST** be implemented before adding any new behaviors that override this method.
That's a good point. Implementation/design isn't set for now but I added a TODO in `third_party/blink/renderer/core/html/forms/element_behavior.h` to make sure we do that in the future (I think it will probably need to be a helper in `HTMLElement/Element` that gets the behavior that would take priority over the other ones, instead of just using `SubmitBehavior`, but tbd).
if (auto* submit_behavior =
GetElement() ? GetElement()->SubmitBehavior() : nullptr) {
const AtomicString& role = submit_behavior->DefaultAriaRole();
if (!role.empty()) {
ax::mojom::blink::Role internal_role = AriaRoleToInternalRole(role);
if (internal_role != ax::mojom::blink::Role::kUnknown) {
return internal_role;
}
}
}Why not have this return a ax::mojom::blink::Role instead of a string? This way you don't need to deal with strings in each behavior's override of DefaultAriaRole, or run the risk that you could ever get it wrong.
AXObject::AriaRoleName could facilitate reflecting out a string if we ever need it.
```suggestion
if (auto* submit_behavior =
GetElement() ? GetElement()->SubmitBehavior() : nullptr) {
return submit_behavior->DefaultAriaRole();
}
```
- Dan Clark
- Jacques Newman
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jacques Newman (Gerrit)
Jacques Newman voted and added 1 comment![]( "Open in Gerrit")
Votes added by Jacques Newman
| Code-Review | +1 |
1 comment
Thanks for making the change to use the role type, looks much nicer to me.
third_party/blink/renderer/modules/accessibility/ax_node_object.cc lgtm, but I'm a little confused that the code coverage is claiming the new logic isn't getting hit in test. Worth double checking and making sure these tests are really being run..
Related details
- Ana Sollano Kim
- Dan Clark
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. |
Ana Sollano Kim (Gerrit)
Ana Sollano Kim added 1 comment![]( "Open in Gerrit")
Thanks for making the change to use the role type, looks much nicer to me.
third_party/blink/renderer/modules/accessibility/ax_node_object.cc lgtm, but I'm a little confused that the code coverage is claiming the new logic isn't getting hit in test. Worth double checking and making sure these tests are really being run..
Not sure why the code coverage isn't picking it up. CEs don't have an assigned role and aren't focusable by default, but the tests are passing. That tells me they're working well.
Related details
- Dan Clark
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |