Bug 262787 – AX: Refactor the TitleUIElement property using LabeledBy and LabelFor relationships.

• Component: Accessibility
• Status: RESOLVED FIXED

[Andres Gonzalez]

.

[Radar WebKit Bug Importer]

<rdar://problem/116581948>

[Andres Gonzalez]

Created attachment 468090 [details]
Patch

[Andres Gonzalez]

Created attachment 468092 [details]
Patch

[Andres Gonzalez]

Created attachment 468097 [details]
Patch

Fixes the figure-element.html test on iOS.

[Tyler Wilcock]

View in context: https://bugs.webkit.org/attachment.cgi?id=468092&action=review

> Source/WebCore/accessibility/AXCoreObject.h:792
> +    LabeledBy,

I think the two-l variant matches aria-labelledby, and documentation on this page reads:

https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-labelledby

Note: While in U.S. English the attribute would be assumed to be spelled "labeledby", the "labelledby" spelling has been established and is the spelling used in accessibility APIs.

So while you are technically correct with this change, seems like AX APIs have standardized the other way. Do you think it makes sense to match them here in our naming too? Or keep this change as-is?

> Source/WebCore/accessibility/AXObjectCache.cpp:2622
> +void AXObjectCache::updateLabelFor(HTMLLabelElement& label)
> +{
> +    removeRelations(label, AXRelationType::LabelFor);
> +    addLabelRelations(label, AXRelationType::LabelFor);
> +
> +#if ENABLE(ACCESSIBILITY_ISOLATED_TREE)
> +    if (auto tree = AXIsolatedTree::treeForPageID(m_pageID))
> +        tree->relationsNeedUpdate(true);
> +#endif

This tree->relationsNeedUpdate(true) block is added 3 times with this patch, totaling 5 repetitions of this pattern including the existing ones. Should we extract this into a private AXObjectCache function that encapsulates everything including the #if ENABLE(ACCESSIBILITY_ISOLATED_TREE) check?

Also seems like AXObjectCache::removeRelations marks tree relations as dirty, while addLabelRelations doesn't, technically making this tree->relationsNeedUpdate(true) necessary. But maybe addLabelRelations should match removeRelations and mark the relations as dirty itself? What do you think?

> Source/WebCore/accessibility/AXObjectCache.cpp:4571
> +    case AXRelationType::ControllerFor:
> +        return aria_controlsAttr;

Is this correct considering we have overloaded AXRelationType::ControllerFor to also be applicable for popovertarget / popover relationships?

> Source/WebCore/accessibility/AXObjectCache.cpp:4833
> +        if (auto label = dynamicDowncast<HTMLLabelElement>(origin)) {

Is auto correct here? Or should it be auto*?

> Source/WebCore/accessibility/AXObjectCache.cpp:4834
> +            if (auto control = Accessibility::controlForLabelElement(*label))

Should this be RefPtr control instead of auto control? Or do you prefer auto here?

> Source/WebCore/accessibility/AXObjectCache.cpp:4842
> +        auto labels = Accessibility::labelsForElement(&origin);
> +        for (auto* label : labels)
> +            addRelation(getOrCreate(origin.renderer()), getOrCreate(label->renderer()), relationType);

Seems like we dereference `label` without a null check here. Would it be desirable to make labelsForElement return references rather than pointers?

> Source/WebCore/accessibility/AccessibilityLabel.cpp:111
> +    if (UNLIKELY(!labeledObjects.size()))

I think this UNLIKELY is predicated on authors correctly pointing a label at something, but I have to imagine in practice it gets done incorrectly a non-insignificant amount (i.e. for attribute pointing at a control that was removed or never existed in the first place).

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1857
> +        // MacOS does not expect native <li> elements to expose label information, it only expects leaf node elements to do that.

Is this extra indent intended?

> LayoutTests/accessibility/aria-labelledby-on-password-input.html:26
> +    output += expect("button1.title", "'AXTitle: '");

Is it correct for #button1 to lose its accessibility text here?

[Andres Gonzalez]

Created attachment 468549 [details]
Patch

[Andres Gonzalez]

Created attachment 468921 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Andres Gonzalez]

Created attachment 468922 [details]
Patch

[Andres Gonzalez]

Created attachment 468932 [details]
Patch

[Andres Gonzalez]

Created attachment 468944 [details]
Patch

[Andres Gonzalez]

Created attachment 468997 [details]
Patch

[Andres Gonzalez]

Created attachment 469008 [details]
Patch

[Andres Gonzalez]

Created attachment 469077 [details]
Patch

[Andres Gonzalez]

Created attachment 469080 [details]
Patch

[Andres Gonzalez]

Created attachment 469081 [details]
Patch

[Andres Gonzalez]

Created attachment 469082 [details]
Patch

[Andres Gonzalez]

Created attachment 469262 [details]
Patch

[Andres Gonzalez]

Created attachment 469276 [details]
Patch

[Tyler Wilcock]

Comment on attachment 469276 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=469276&action=review

> Source/WebCore/accessibility/AXCoreObject.cpp:549
> +    return labels.size() >= 1 ? labels.first().get() : nullptr;

Can this be: return labels.size() ? labels.first().get() : nullptr;

> Source/WebCore/accessibility/AXCoreObject.h:770
> +    LabeledBy,

Isn't aria-labelledby the canonical spelling (vs. aria-labeledby)? Would it make sense to match that?

> Source/WebCore/accessibility/AXObjectCache.cpp:4337
> +        case AXLabelChanged: {
> +            auto updatedFields = updatedObjects.get(notification.first->objectID());
> +            if (!updatedFields.nodeAndDependentProperties) {
> +                updatedObjects.set(notification.first->objectID(), UpdatedFields { updatedFields.children, updatedFields.node, true });
> +                tree->updateNodeAndDependentProperties(*notification.first);

By doing this, the same object could undergo AXIsolatedTree::updateNode and AXIsolatedTree::updateNodeAndDependentProperties if the updateNode happens first, which is not ideal. Is there any way to avoid that?

> Source/WebCore/accessibility/AXObjectCache.cpp:4648
> +    case AXRelationType::ControllerFor:
> +        return aria_controlsAttr;

Is this correct considering we have overloaded AXRelationType::ControllerFor to also be applicable for popovertarget / popover relationships?

> Source/WebCore/accessibility/AXObjectCache.cpp:4658
> +        return aria_labeledbyAttr;

aria-labelledby is the canonical spelling, right? Also in https://bugs.webkit.org/show_bug.cgi?id=266384 we decided that aria-labeledby (the non-canonical spelling) is used enough that we can't remove support for it. Does that cause this method to behave weirdly considering two attributes can create this relation, but this method only maps back out to one attribute, despite the fact that the relation may have been generated by the other attribute spelling?

> Source/WebCore/accessibility/AXObjectCache.cpp:4916
> +void AXObjectCache::addLabelRelation(Element& origin, AXRelationType relationType)

Why have a relationType parameter when all callers pass AXRelationType::LabelFor, and we ASSERT_NOT_REACHED for any other value?

> Source/WebCore/accessibility/AXObjectCache.cpp:4921
> +        if (auto label = dynamicDowncast<HTMLLabelElement>(origin)) {

Shouldn't this be auto*?

> Source/WebCore/accessibility/AXObjectCache.cpp:4922
> +            if (auto control = Accessibility::controlForLabelElement(*label))

Shouldn't this be RefPtr rather than auto?

> Source/WebCore/accessibility/AccessibilityLabel.cpp:107
> +    if (!m_children.size() || containsOnlyStaticText())

m_children.isEmpty() may read more easily here?

> Source/WebCore/accessibility/AccessibilityLabel.cpp:111
> +    if (UNLIKELY(!labeledObjects.size()))

labeledObjects.isEmpty() may read more easily here too.

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1703
> +        return labelAttribute.releaseString();

Is it really safe to do this? I thought AtomStrings were a shared "view" of the same string, so even if you have an "owned" copy of one, things like releaseString() affect all other usage of the atom string.

> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1899
> +        // MacOS does not expect native <li> elements to expose label information, it only expects leaf node elements to do that.

Looks like this comment accidentally got indented.

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:722
> +            updateNode(downcast<AccessibilityObject>(*labeledObject));

Maybe dynamicDowncast rather than downcast to avoid a vulnerability in case we have a bug? I doubt this is a hot path where it would matter.

> LayoutTests/accessibility/aria-labelledby-on-password-input-expected.txt:4
> +PASS: button1.title === 'AXTitle: '

Why does this button no longer have a title? Seems like we should still be exposing the text under element?

> LayoutTests/accessibility/aria-labelledby-on-password-input.html:38
> +        output += expect("button2.title", "'AXTitle: Before password After password'");

Seems like we now expose this text in both the AXTitle and AXDescription. Is that correct behavior?

> LayoutTests/accessibility/th-as-title-ui.html:-9
> -<html>
> -<script>
> -    if (window.testRunner)
> -        testRunner.dumpAsText();
> -</script>
> -<body id="body">
> -    
> -    <!-- the AXGroup that comes from the <td> should have the <th> as its title ui element 
> -         In the 2nd case, it should not have a title ui element -->

Why delete this test?

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #20)
> Comment on attachment 469276 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=469276&action=review
> 
> > Source/WebCore/accessibility/AXCoreObject.cpp:549
> > +    return labels.size() >= 1 ? labels.first().get() : nullptr;
> 
> Can this be: return labels.size() ? labels.first().get() : nullptr;

AG: Changed, although I thought that having the >= 1 would make more obvious the parallel with the COCOA restriction of == 1.
> 
> > Source/WebCore/accessibility/AXCoreObject.h:770
> > +    LabeledBy,
> 
> Isn't aria-labelledby the canonical spelling (vs. aria-labeledby)? Would it
> make sense to match that?

AG: This follows our naming conventions and it doesn't have to match ARIA's naming, e.g., FlowsTo vs. aria-flowto. LabeledBy can be the result of an aria-labelledby, but there are other sources of this relationship like <label for>.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:4337
> > +        case AXLabelChanged: {
> > +            auto updatedFields = updatedObjects.get(notification.first->objectID());
> > +            if (!updatedFields.nodeAndDependentProperties) {
> > +                updatedObjects.set(notification.first->objectID(), UpdatedFields { updatedFields.children, updatedFields.node, true });
> > +                tree->updateNodeAndDependentProperties(*notification.first);
> 
> By doing this, the same object could undergo AXIsolatedTree::updateNode and
> AXIsolatedTree::updateNodeAndDependentProperties if the updateNode happens
> first, which is not ideal. Is there any way to avoid that?

AG: Fixed.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:4648
> > +    case AXRelationType::ControllerFor:
> > +        return aria_controlsAttr;
> 
> Is this correct considering we have overloaded AXRelationType::ControllerFor
> to also be applicable for popovertarget / popover relationships?
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:4658
> > +        return aria_labeledbyAttr;
> 
> aria-labelledby is the canonical spelling, right? Also in
> https://bugs.webkit.org/show_bug.cgi?id=266384 we decided that
> aria-labeledby (the non-canonical spelling) is used enough that we can't
> remove support for it. Does that cause this method to behave weirdly
> considering two attributes can create this relation, but this method only
> maps back out to one attribute, despite the fact that the relation may have
> been generated by the other attribute spelling?

AG: This mapping didn't create any problem but I removed the method for now since it ended up being used in only one trivial case. Will revisit if needed.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:4916
> > +void AXObjectCache::addLabelRelation(Element& origin, AXRelationType relationType)
> 
> Why have a relationType parameter when all callers pass
> AXRelationType::LabelFor, and we ASSERT_NOT_REACHED for any other value?

AG: Fixed.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:4921
> > +        if (auto label = dynamicDowncast<HTMLLabelElement>(origin)) {
> 
> Shouldn't this be auto*?

AG: Fixed.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:4922
> > +            if (auto control = Accessibility::controlForLabelElement(*label))
> 
> Shouldn't this be RefPtr rather than auto?

AG: Fixed.
> 
> > Source/WebCore/accessibility/AccessibilityLabel.cpp:107
> > +    if (!m_children.size() || containsOnlyStaticText())
> 
> m_children.isEmpty() may read more easily here?
> 
> > Source/WebCore/accessibility/AccessibilityLabel.cpp:111
> > +    if (UNLIKELY(!labeledObjects.size()))
> 
> labeledObjects.isEmpty() may read more easily here too.


AG: Changed.
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1703
> > +        return labelAttribute.releaseString();
> 
> Is it really safe to do this? I thought AtomStrings were a shared "view" of
> the same string, so even if you have an "owned" copy of one, things like
> releaseString() affect all other usage of the atom string.

AG: Fixed.
> 
> > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1899
> > +        // MacOS does not expect native <li> elements to expose label information, it only expects leaf node elements to do that.
> 
> Looks like this comment accidentally got indented.

AG: Fixed.
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTree.cpp:722
> > +            updateNode(downcast<AccessibilityObject>(*labeledObject));
> 
> Maybe dynamicDowncast rather than downcast to avoid a vulnerability in case
> we have a bug? I doubt this is a hot path where it would matter.

AG: Fixed.
> 
> > LayoutTests/accessibility/aria-labelledby-on-password-input-expected.txt:4
> > +PASS: button1.title === 'AXTitle: '
> 
> Why does this button no longer have a title? Seems like we should still be
> exposing the text under element?


AG: Fixed.
> 
> > LayoutTests/accessibility/aria-labelledby-on-password-input.html:38
> > +        output += expect("button2.title", "'AXTitle: Before password After password'");
> 
> Seems like we now expose this text in both the AXTitle and AXDescription. Is
> that correct behavior?


AG: That's correct. It saves VO an IPC call since VO requests the description if there is no title
> 
> > LayoutTests/accessibility/th-as-title-ui.html:-9
> > -<html>
> > -<script>
> > -    if (window.testRunner)
> > -        testRunner.dumpAsText();
> > -</script>
> > -<body id="body">
> > -    
> > -    <!-- the AXGroup that comes from the <td> should have the <th> as its title ui element 
> > -         In the 2nd case, it should not have a title ui element -->
> 
> Why delete this test?

AG: This was testing that table headers ought to be labels for input fields in table cells, which is not required or desirable.

[Andres Gonzalez]

Created attachment 469378 [details]
Patch

[Tyler Wilcock]

Comment on attachment 469378 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=469378&action=review

> Source/WebCore/accessibility/AXObjectCache.cpp:4178
> +        bool dependentProperties { false };
>      };
>      HashMap<AXID, UpdatedFields> updatedObjects;
>      auto updateNode = [&] (RefPtr<AccessibilityObject> axObject) {

Does this `updateNode` helper function need to be updated to pass along the new `dependentProperties` state in the `UpdatedFields` struct it creates inside its body?

> Source/WebCore/accessibility/AXObjectCache.cpp:4340
> +            auto updatedFields = updatedObjects.get(notification.first->objectID());
> +            if (!updatedFields.node) {
> +                updatedObjects.set(notification.first->objectID(), UpdatedFields { updatedFields.children, true, updatedFields.dependentProperties });
> +                tree->updateNode(*notification.first);
> +            }

Can we use the `updateNode` helper lambda for this?

> Source/WebCore/accessibility/AXObjectCache.cpp:4779
> +        auto targetIDs = it->value.get(static_cast<uint8_t>(relationType));
> +        for (AXID targetID : targetIDs)
> +            removeRelationByID(targetID, axID, symmetric);

This may not be possible...but asking anyways. Is it possible for `it->value.get(static_cast<uint8_t>(relationType))` to return an empty set of targetIDs, causing us to never call removeRelationByID, and thus dirtyIsolatedTreeRelations unnecessarily? If so, maybe we should only conditionally dirtyIsolatedTreeRelations upon actually making a change?

> Source/WebCore/accessibility/AXObjectCache.cpp:4907
> +            addRelation(&origin, control.get(), AXRelationType::LabelFor);
> +            dirtyIsolatedTreeRelations();

It's technically possible for `addRelation` to fail to actually add a relation, right? In which case we would be calling `dirtyIsolatedTreeRelations` unnecessarily. Maybe addRelation can return a success / fail bool and we can key off of that?

> Source/WebCore/accessibility/AXObjectCache.cpp:4913
> +            addRelation(getOrCreate(&origin), getOrCreate(parent.get()), AXRelationType::LabelFor);
> +            dirtyIsolatedTreeRelations();

Same comment as the other addRelation + dirty pair above.

> Source/WebCore/accessibility/AXObjectCache.cpp:4935
> +    dirtyIsolatedTreeRelations();

Here's another place where we could consider only conditionally dirtying isolated tree relations? Though this one is not new to your patch.

> Source/WebCore/accessibility/AccessibilityLabel.cpp:105
>  bool AccessibilityLabel::containsUnrelatedControls() const

Is this function actually called anywhere? I can't seem to find a usage.

> LayoutTests/accessibility/aria-labelledby-on-password-input.html:26
> +    output += expect("button1.title", "'AXTitle: '");

Still seems like this button has no text under element exposed?

[Andres Gonzalez]

Created attachment 469381 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #23)
> Comment on attachment 469378 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=469378&action=review
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:4178
> > +        bool dependentProperties { false };
> >      };
> >      HashMap<AXID, UpdatedFields> updatedObjects;
> >      auto updateNode = [&] (RefPtr<AccessibilityObject> axObject) {
> 
> Does this `updateNode` helper function need to be updated to pass along the
> new `dependentProperties` state in the `UpdatedFields` struct it creates
> inside its body?

AG: At the moment, it is just like the children updates, it is used in only 1 place. I will rewrite this mechanism in a separate patch though to make it marginally more efficient by modifying the updatedFields in place.

> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:4340
> > +            auto updatedFields = updatedObjects.get(notification.first->objectID());
> > +            if (!updatedFields.node) {
> > +                updatedObjects.set(notification.first->objectID(), UpdatedFields { updatedFields.children, true, updatedFields.dependentProperties });
> > +                tree->updateNode(*notification.first);
> > +            }
> 
> Can we use the `updateNode` helper lambda for this?

AG: done.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:4779
> > +        auto targetIDs = it->value.get(static_cast<uint8_t>(relationType));
> > +        for (AXID targetID : targetIDs)
> > +            removeRelationByID(targetID, axID, symmetric);
> 
> This may not be possible...but asking anyways. Is it possible for
> `it->value.get(static_cast<uint8_t>(relationType))` to return an empty set
> of targetIDs, causing us to never call removeRelationByID, and thus
> dirtyIsolatedTreeRelations unnecessarily? If so, maybe we should only
> conditionally dirtyIsolatedTreeRelations upon actually making a change?

AG: in this case we are dirtying the relations because of:

    m_relations.remove(it);
    dirtyIsolatedTreeRelations();
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:4907
> > +            addRelation(&origin, control.get(), AXRelationType::LabelFor);
> > +            dirtyIsolatedTreeRelations();
> 
> It's technically possible for `addRelation` to fail to actually add a
> relation, right? In which case we would be calling
> `dirtyIsolatedTreeRelations` unnecessarily. Maybe addRelation can return a
> success / fail bool and we can key off of that?

AG: yes, thought about that, will do in a separate patch since as you noticed below, it is not a new problem
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:4913
> > +            addRelation(getOrCreate(&origin), getOrCreate(parent.get()), AXRelationType::LabelFor);
> > +            dirtyIsolatedTreeRelations();
> 
> Same comment as the other addRelation + dirty pair above.

AG: same as above.
> 
> > Source/WebCore/accessibility/AXObjectCache.cpp:4935
> > +    dirtyIsolatedTreeRelations();
> 
> Here's another place where we could consider only conditionally dirtying
> isolated tree relations? Though this one is not new to your patch.


AG: see above.
> 
> > Source/WebCore/accessibility/AccessibilityLabel.cpp:105
> >  bool AccessibilityLabel::containsUnrelatedControls() const
> 
> Is this function actually called anywhere? I can't seem to find a usage.

AG: removed the dead code.
> 
> > LayoutTests/accessibility/aria-labelledby-on-password-input.html:26
> > +    output += expect("button1.title", "'AXTitle: '");
> 
> Still seems like this button has no text under element exposed?

AG: and that is correct because the title is an empty password field. In line 32 you can see that the title is the content of the password field when something was entered.

[EWS]

Committed 273000@main (4ef6169e54ce): <https://commits.webkit.org/273000@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 469381 [details].

[Ross Kirsling]

This broke Win debug:
> C:\BW\WinCairo-64-bit-Debug-Build\build\Source\WebCore\accessibility/AXObjectCache.cpp(4656,148): error: unknown type name 'uint'; did you mean 'int'?

Indeed, uint is not a thing, so I'm not sure why this didn't break even more builds.

[Ross Kirsling]

(In reply to Ross Kirsling from comment #27)
> This broke Win debug:
> > C:\BW\WinCairo-64-bit-Debug-Build\build\Source\WebCore\accessibility/AXObjectCache.cpp(4656,148): error: unknown type name 'uint'; did you mean 'int'?
> 
> Indeed, uint is not a thing, so I'm not sure why this didn't break even more
> builds.

Fixing in https://github.com/WebKit/WebKit/pull/22748.

[Andres Gonzalez]

(In reply to Ross Kirsling from comment #27)
> This broke Win debug:
> > C:\BW\WinCairo-64-bit-Debug-Build\build\Source\WebCore\accessibility/AXObjectCache.cpp(4656,148): error: unknown type name 'uint'; did you mean 'int'?
> 
> Indeed, uint is not a thing, so I'm not sure why this didn't break even more
> builds.

#ifndef __DARWIN_UINT
typedef unsigned int            uint;           /* Sys V compatibility */
#define __DARWIN_UINT
#endif

types.h line 93.