Update aria_properties.json5, create generated utility functions [chromium/src : main]
Joanmarie Diggs (Gerrit)
Joanmarie Diggs added 1 comment![]( "Open in Gerrit")
David: This CL started out as the one you reviewed to generate functions from aria_properties.json5. And then I figured, why not use it for even more things? And, well, it grew. 😇
There are more generated functions for attributes, plus name-from logic. Lots of test coverage. Using the generated functions in ax_object.cc/ax_node_object.cc led to finding several places where what we do is not in alignment with the ARIA spec. In those cases the current chromium behavior was preserved and TODOs added so we can revisit them.
PTAL and let me know if this is a good idea, or if things should be kept limited to what was done in the CL you reviewed. Thanks in advance!
Related details
- David Tseng
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
David Tseng (Gerrit)
David Tseng added 2 comments![]( "Open in Gerrit")
"application",Was looking for an rdf version of ARIA 1.3. Does that exist? Only found something for ARIA 1.
https://www.w3.org/TR/wai-aria-1.1/img/rdf_model
I haven't manually verified the additions here, but can double check it against something. Will take some time to check it since it's a large change but wanted to ask first.
const auto& idref_attrs = GetAriaIdrefAttrs();With these changes, are we introducing new attributes, new constraints, etc?
Wanted to see if this is a purely aesthetics, cleanup change or if we are introducing functional changes that we may have to test for.
Related details
- Joanmarie Diggs
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. |
Joanmarie Diggs (Gerrit)
Joanmarie Diggs added 5 comments![]( "Open in Gerrit")
"application",Was looking for an rdf version of ARIA 1.3. Does that exist? Only found something for ARIA 1.
https://www.w3.org/TR/wai-aria-1.1/img/rdf_modelI haven't manually verified the additions here, but can double check it against something. Will take some time to check it since it's a large change but wanted to ask first.
I'll ask around (my colleague co-chairs the ARIA working group), but I'm not aware of any.
const auto& idref_attrs = GetAriaIdrefAttrs();With these changes, are we introducing new attributes, new constraints, etc?
Wanted to see if this is a purely aesthetics, cleanup change or if we are introducing functional changes that we may have to test for.
We are potentially introducing changes. A number of them were caught by existing tests. In each of those cases I added special-casing (along with a TODO comment) to the production code so that the existing behavior is preserved. I'll point those changes out in this CL. But there could be instances where we lack test coverage. I'll take a look at where coverage is missing for ax_object.cc and ax_node_object.cc.
if (RoleSupportsAriaRequired(RoleValue())) {
if (IsAriaAttributeTrue(html_names::kAriaRequiredAttr)) {
return true;
}
}
// TODO(accessibility): The ARIA spec says aria-required is supported on
// radiogroup, not individual radio buttons. However, the
// `aria-required-changed.html` test uses aria-required directly on a radio
// button and expects it to be supported.
if (RoleValue() == ax::mojom::blink::Role::kRadioButton) {
if (IsAriaAttributeTrue(html_names::kAriaRequiredAttr)) {
return true;
}
}David: Here's an example where we had to special-case things because the ARIA spec and our implementation are out of alignment.
// Special case: combobox roles used to support aria-orientation but it was
// removed from the ARIA spec. We continue to honor explicit aria-orientation
// attributes on comboboxes for backwards compatibility.
// TODO(accessibility): Remove this once tests/content are updated.
if (RoleValue() == ax::mojom::blink::Role::kComboBoxGrouping ||
RoleValue() == ax::mojom::blink::Role::kComboBoxMenuButton ||
RoleValue() == ax::mojom::blink::Role::kComboBoxSelect ||
RoleValue() == ax::mojom::blink::Role::kTextFieldWithComboBox) {
const AtomicString& aria_orientation =
AriaTokenAttribute(html_names::kAriaOrientationAttr);
if (EqualIgnoringASCIICase(aria_orientation, "horizontal")) {
return kAccessibilityOrientationHorizontal;
} else if (EqualIgnoringASCIICase(aria_orientation, "vertical")) {
return kAccessibilityOrientationVertical;
}
// Fall through for combobox without explicit orientation
}David: Here's another example of special-casing to preserve existing behavior that is not in alignment with the ARIA spec.
// TODO(accessibility): The ARIA spec says name from: prohibited on the term
// role, but that breaks DumpAccessibilityTreeTest.AccessibilityAriaTerm.
if (RoleValue() == ax::mojom::blink::Role::kTerm) {
return true;
}
// TODO(accessibility): The ARIA spec says name from: author on the math
// role, but that breaks DumpAccessibilityTreeTest.AccessibilityAriaMath.
if (RoleValue() == ax::mojom::blink::Role::kMath) {
return true;
}
David: Two more cases of preserving existing behavior that is not in alignment with the ARIA spec.
Related details
- David Tseng
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. |
Joanmarie Diggs (Gerrit)
Joanmarie Diggs added 1 comment![]( "Open in Gerrit")
"application",Joanmarie DiggsWas looking for an rdf version of ARIA 1.3. Does that exist? Only found something for ARIA 1.
https://www.w3.org/TR/wai-aria-1.1/img/rdf_modelI haven't manually verified the additions here, but can double check it against something. Will take some time to check it since it's a large change but wanted to ask first.
I'll ask around (my colleague co-chairs the ARIA working group), but I'm not aware of any.
My colleague pointed out https://github.com/w3c/aria/blob/main/common/script/roleInfo.js which they are planning to start using for spec PRs, so that might come in handy.
David Tseng (Gerrit)
David Tseng added 1 comment![]( "Open in Gerrit")
const auto& idref_attrs = GetAriaIdrefAttrs();Joanmarie DiggsWith these changes, are we introducing new attributes, new constraints, etc?
Wanted to see if this is a purely aesthetics, cleanup change or if we are introducing functional changes that we may have to test for.
We are potentially introducing changes. A number of them were caught by existing tests. In each of those cases I added special-casing (along with a TODO comment) to the production code so that the existing behavior is preserved. I'll point those changes out in this CL. But there could be instances where we lack test coverage. I'll take a look at where coverage is missing for ax_object.cc and ax_node_object.cc.
I wonder if we can try to roll this out more gradually though I believe getting the spec compliant aria properties landed is the right choice.
I think we're bound to regress behaviors in ax*object but also downstream in the browser accessibility platform accessibility APIs.
Rollouts of this type have been done via flags. One starting point is to guard the changes behind a flag and I can help set up an experiment to ramp up things to a percentage of Chrome on progressively more channels. .e.g. load the re-vamped aria properties.json5 with the flag enabled. Haven't thought through how this may impact the changes in ax_object.
This would allow us to land changes on trunk (for fuzz testing) but not ship it to dev, beta, stable till later.
Let me know what you think or if there's a path forward with less work and that can keep things from regressing.
Related details
- Joanmarie Diggs
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. |
Joanmarie Diggs (Gerrit)
Joanmarie Diggs added 1 comment![]( "Open in Gerrit")
const auto& idref_attrs = GetAriaIdrefAttrs();Joanmarie DiggsWith these changes, are we introducing new attributes, new constraints, etc?
Wanted to see if this is a purely aesthetics, cleanup change or if we are introducing functional changes that we may have to test for.
David TsengWe are potentially introducing changes. A number of them were caught by existing tests. In each of those cases I added special-casing (along with a TODO comment) to the production code so that the existing behavior is preserved. I'll point those changes out in this CL. But there could be instances where we lack test coverage. I'll take a look at where coverage is missing for ax_object.cc and ax_node_object.cc.
I wonder if we can try to roll this out more gradually though I believe getting the spec compliant aria properties landed is the right choice.
I think we're bound to regress behaviors in ax*object but also downstream in the browser accessibility platform accessibility APIs.
Rollouts of this type have been done via flags. One starting point is to guard the changes behind a flag and I can help set up an experiment to ramp up things to a percentage of Chrome on progressively more channels. .e.g. load the re-vamped aria properties.json5 with the flag enabled. Haven't thought through how this may impact the changes in ax_object.
This would allow us to land changes on trunk (for fuzz testing) but not ship it to dev, beta, stable till later.
Let me know what you think or if there's a path forward with less work and that can keep things from regressing.
Absolutely. And whatever you think is best (e.g. flag) is fine with me. In the meantime here's my plan:
1. Remove all changes to the production code (i.e. ax_node_object.cc and ax_object.cc) in this CL. (Done.)
I don't *think* that merely changing the JSON file should break anything. Note that devtools-frontend also uses this file, but a code search + local grep turned up no consumers of the removed field (supportedAttributes). And I ran the script to regenerate the devtools-frontend ARIAProperties.js and built and tested using `--custom-devtools-frontend`. I see no differences, errors, etc. (Though getting a review from a devtools-frontend owner seems like a smart thing to do.)
2. Break all the changes to the production code into smaller chunks. (Will be finished later today.) That should make them both easier to review and easier to determine if they belong behind a flag.
Please let me know what you think. And thanks again!
Related details
- David Tseng
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. |
Joanmarie Diggs (Gerrit)
Joanmarie Diggs added 1 comment![]( "Open in Gerrit")
David: I did some more work on the generated files. Now there are three templates. I also created a gist so that you can see what gets generated. (Couldn't find any artifacts in Luci.) https://gist.github.com/joanmarie/68915f5b2ccde5b5366d61bee264a60e
Related details
- David Tseng
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. |
David Tseng (Gerrit)
David Tseng voted and added 5 comments![]( "Open in Gerrit")
David Tseng added votes with a message
lgtm % comments.
| Code-Review | +1 |
5 comments
// General lookup function to get valid enum values for any ARIA attributenit: let's use periods to end all comments.
CORE_EXPORT const AtomicString& InternalRoleToAriaRole(ax::mojom::blink::Role internal_role);Noting we're returning an AtomicString here but using std::string above.
Wondering if that was intended?
CORE_EXPORT bool RoleSupports{{ attr.base_name }}(ax::mojom::blink::Role internal_role);Looks like we get some unexpected casing after template subs e.g.
RoleSupportsAriaColindextext
CORE_EXPORT bool RoleSupports{{ attr.base_name }}(ax::mojom::blink::Role internal_role);I kind of like the direct RoleSupports<attribute> methods here, but do we want to consider reducing the number of methods and pass the attribute as an argument? i.e. RoleSupportsAttribute(role, attribute)?
name: "aria-labeledby",Is there an official decision noted somewhere that both spellings (aria-labeledby and aria-labelledby) are supported?
Related details
- Joanmarie Diggs
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. |
Joanmarie Diggs (Gerrit)
Joanmarie Diggs added 9 comments![]( "Open in Gerrit")
Thanks for the review! Changes made and gist updated with the updated generated files: https://gist.github.com/joanmarie/68915f5b2ccde5b5366d61bee264a60e
// General lookup function to get valid enum values for any ARIA attributenit: let's use periods to end all comments.
Done
CORE_EXPORT const AtomicString& InternalRoleToAriaRole(ax::mojom::blink::Role internal_role);Noting we're returning an AtomicString here but using std::string above.
Wondering if that was intended?
It was not. I've been iterating on this change quite a bit. Thanks for catching that! Using AtomicString everywhere now.
CORE_EXPORT bool RoleSupports{{ attr.base_name }}(ax::mojom::blink::Role internal_role);I kind of like the direct RoleSupports<attribute> methods here, but do we want to consider reducing the number of methods and pass the attribute as an argument? i.e. RoleSupportsAttribute(role, attribute)?
Done
CORE_EXPORT bool RoleSupports{{ attr.base_name }}(ax::mojom::blink::Role internal_role);Looks like we get some unexpected casing after template subs e.g.
RoleSupportsAriaColindextext
I could fix that by adding basenames for attributes to the JSON file. But the current casing, while admittedly less than ideal, is consistent with the ARIA attribute names in [html_names.cc](https://source.chromium.org/chromium/chromium/src/+/main:out/linux-Debug/gen/third_party/blink/renderer/core/html_names.cc;l=212;drc=07836fac7be4e6d77355bd9c673b4be0de3a41aa;bpv=0;bpt=1).
Having said that, making the change you suggested about a single RoleSupportsAttribute() eliminates the functions with the unexpected casing.
Is there an official decision noted somewhere that both spellings (aria-labeledby and aria-labelledby) are supported?
I couldn't find one. But from some code searching, it looks like supporting both was inherited from WebKit. In addition, there's a note in the ARIA spec which says the following:
The expected spelling of this property in U.S. English is "labeledby." However, the accessibility API features to which this property is mapped have established the "labelledby" spelling. This property is spelled that way to match the convention and minimize the difficulty for developers.
1. I wonder how many web developers know how things are spelled in accessibility APIs.
2. When the spec uses the word generically, e.g. "In the following code sample, the containing img and is appropriately labeled by the caption paragraph." They are using the U.S. English spelling.
I think it's wise to continue supporting both forms.
if (RoleSupportsAriaRequired(RoleValue())) {
if (IsAriaAttributeTrue(html_names::kAriaRequiredAttr)) {
return true;
}
}
// TODO(accessibility): The ARIA spec says aria-required is supported on
// radiogroup, not individual radio buttons. However, the
// `aria-required-changed.html` test uses aria-required directly on a radio
// button and expects it to be supported.
if (RoleValue() == ax::mojom::blink::Role::kRadioButton) {
if (IsAriaAttributeTrue(html_names::kAriaRequiredAttr)) {
return true;
}
}Joanmarie DiggsDavid: Here's an example where we had to special-case things because the ARIA spec and our implementation are out of alignment.
Acknowledged
// Special case: combobox roles used to support aria-orientation but it was
// removed from the ARIA spec. We continue to honor explicit aria-orientation
// attributes on comboboxes for backwards compatibility.
// TODO(accessibility): Remove this once tests/content are updated.
if (RoleValue() == ax::mojom::blink::Role::kComboBoxGrouping ||
RoleValue() == ax::mojom::blink::Role::kComboBoxMenuButton ||
RoleValue() == ax::mojom::blink::Role::kComboBoxSelect ||
RoleValue() == ax::mojom::blink::Role::kTextFieldWithComboBox) {
const AtomicString& aria_orientation =
AriaTokenAttribute(html_names::kAriaOrientationAttr);
if (EqualIgnoringASCIICase(aria_orientation, "horizontal")) {
return kAccessibilityOrientationHorizontal;
} else if (EqualIgnoringASCIICase(aria_orientation, "vertical")) {
return kAccessibilityOrientationVertical;
}
// Fall through for combobox without explicit orientation
}Joanmarie DiggsDavid: Here's another example of special-casing to preserve existing behavior that is not in alignment with the ARIA spec.
Acknowledged
// TODO(accessibility): The ARIA spec says name from: prohibited on the term
// role, but that breaks DumpAccessibilityTreeTest.AccessibilityAriaTerm.
if (RoleValue() == ax::mojom::blink::Role::kTerm) {
return true;
}
// TODO(accessibility): The ARIA spec says name from: author on the math
// role, but that breaks DumpAccessibilityTreeTest.AccessibilityAriaMath.
if (RoleValue() == ax::mojom::blink::Role::kMath) {
return true;
}
Joanmarie DiggsDavid: Two more cases of preserving existing behavior that is not in alignment with the ARIA spec.
Acknowledged
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joanmarie Diggs (Gerrit)
Joanmarie Diggs added 1 comment![]( "Open in Gerrit")
Yang: Could you please look at the changes to aria_properties.json5? I built devtools-frontend with those changes and did not notice any difference in the Accessibility panel, nor could I find anything consuming the supportedAttributes field beyond copying it to the generated files. But I don't want to break anything. Thanks in advance!
Related details
- Yang Guo
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Yang Guo (Gerrit)
Yang Guo voted and added 1 comment![]( "Open in Gerrit")
Votes added by Yang Guo
| Commit-Queue | +1 |
1 comment
Philip is the right person on my team to take a look.
Related details
- Joanmarie Diggs
- Philip Pfaffe
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Philip Pfaffe (Gerrit)
Philip Pfaffe voted and added 1 comment![]( "Open in Gerrit")
Votes added by Philip Pfaffe
| Code-Review | +1 |
1 comment
Changes look good from a devtools perspective.
Related details
- Joanmarie Diggs
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joanmarie Diggs (Gerrit)
Joanmarie Diggs added 1 comment![]( "Open in Gerrit")
- Mason Freed
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Mason Freed (Gerrit)
Mason Freed voted and added 1 comment![]( "Open in Gerrit")
Votes added by Mason Freed
| Code-Review | +1 |
1 comment
third_party/blink/renderer/core/BUILD.gn and third_party/blink/renderer/core/html/aria_properties.json5
LGTM
Related details
- Joanmarie Diggs
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Joanmarie Diggs (Gerrit)
Joanmarie Diggs voted Commit-Queue+2![]( "Open in Gerrit")
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
Update aria_properties.json5, create generated utility functions
aria_properties.json5 got out of date: roles were missing, details about
which attributes were supported on a given role were also not current.
This file should be kept up-to-date.
In addition, ax_object.cc has hard-coded role and attribute related
logic that must also be manually kept in alignment with the ARIA spec.
Rather than having to maintain both the JSON file and the accessibility
code, make the JSON file the single source of truth for roles and
attributes and then generate utility functions based on that file which
can be used in ax_object.cc et al.
This initial step does not impact production code.
* Add missing roles and new internalRoles field to aria_properties.json5
so that we can generate role-related functions.
* Remove the supportedAttributes field from aria_properties.json5. In
its place add three optional fields for each attribute: isGlobal,
supportedOnRoles, and preventedOnRoles.
* Create make_ax_utilities.py script which is used by the new templates
and has validation e.g. to ensure that non-global attributes have
supportedOnRoles, and that global attributes lack supportedOnRoles.
* Add comprehensive unit test coverage for all roles and attributes,
both for the functions themselves and to detect incomplete changes
made to aria_properties.json5.
- M third_party/blink/renderer/build/scripts/aria_properties.py
- A third_party/blink/renderer/build/scripts/make_ax_utilities.py
- A third_party/blink/renderer/build/scripts/templates/ax_utilities_generated.h.tmpl
- A third_party/blink/renderer/build/scripts/templates/ax_utilities_generated_attributes.cc.tmpl
- A third_party/blink/renderer/build/scripts/templates/ax_utilities_generated_naming.cc.tmpl
- A third_party/blink/renderer/build/scripts/templates/ax_utilities_generated_roles.cc.tmpl
- M third_party/blink/renderer/core/BUILD.gn
- A third_party/blink/renderer/core/accessibility/ax_utilities_test.cc
- M third_party/blink/renderer/core/accessibility/build.gni
- M third_party/blink/renderer/core/html/aria_properties.json5
Code-Review: +1 by Philip Pfaffe, +1 by David Tseng, +1 by Mason Freed
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |