Bug 255433 – Accessibility is not informed of changes to several attributes

• Component: Accessibility
• Status: RESOLVED FIXED

[Tyler Wilcock]

Accessibility relies on attribute changes from the DOM to stay up to date, and we are missing quite a few. This is because accessibility is notified about attribute changes in Element::attributeChanged. However, this method is virtual, and many subclasses don't call up to Element::attributeChanged. I believe we've always had this problem for some attributes, but it was worsened with the restructuring in this change:

https://bugs.webkit.org/show_bug.cgi?id=255258 (Drop Element::parseAttribute())

After this change, we stopped receiving updates for the disabled attribute for option elements, the placeholder attribute for HTMLTextFormControlElement subclasses, and likely more.

This was caught because accessibility/dynamic-attribute-changes-should-update-isenabled.html started failing in isolated tree mode (--accessibility-isolated-tree test runner flag). This configuration is not in EWS (post-commit only for now), so it couldn't have been caught up-front.

[Radar WebKit Bug Importer]

<rdar://problem/108030716>

[Tyler Wilcock]

Created attachment 465908 [details]
Patch

[Tyler Wilcock]

cc Chris Dumez — would greatly appreciate your review.

[Chris Dumez]

Comment on attachment 465908 [details]
Patch

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

> Source/WebCore/dom/Element.cpp:-2108
> -    if (AXObjectCache* cache = document().existingAXObjectCache())

Can we just add a ScopeExit at the beginning of this function instead of introducing a new attributeChangedInternal() function?

e.g.
```
auto notifyAccessibilityOfAttributeChangeOnExit = makeScopeExit([&] {
    if (AXObjectCache* cache = document().existingAXObjectCache())
        cache->deferAttributeChangeIfNeeded(this, name, oldValue, newValue);
});
```

[Chris Dumez]

Comment on attachment 465908 [details]
Patch

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

>> Source/WebCore/dom/Element.cpp:-2108
>> -    if (AXObjectCache* cache = document().existingAXObjectCache())
> 
> Can we just add a ScopeExit at the beginning of this function instead of introducing a new attributeChangedInternal() function?
> 
> e.g.
> ```
> auto notifyAccessibilityOfAttributeChangeOnExit = makeScopeExit([&] {
>     if (AXObjectCache* cache = document().existingAXObjectCache())
>         cache->deferAttributeChangeIfNeeded(this, name, oldValue, newValue);
> });
> ```

Oh, I guess this still won't work if some subclasses don't end up calling the base class's attributeChanged() :/

[Chris Dumez]

Comment on attachment 465908 [details]
Patch

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

> Source/WebCore/dom/Element.h:813
> +    void attributeChangedInternal(const QualifiedName&, const AtomString& oldValue, const AtomString& newValue, AttributeModificationReason = ModifiedDirectly);

I think I'm OK with this patch but:
1. I have slight concerns about perf so it would be nice to trigger A/B testing for Speedometer to be on the safe side
2. attributeChangedInternal() is too obscure. I'd prefer if we found a better name, maybe `notifyAttributeChanged()`
3. The patch doesn't seem to apply

[Tyler Wilcock]

Created attachment 465909 [details]
Patch

[Tyler Wilcock]

(In reply to Chris Dumez from comment #6)
> Comment on attachment 465908 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=465908&action=review
> 
> > Source/WebCore/dom/Element.h:813
> > +    void attributeChangedInternal(const QualifiedName&, const AtomString& oldValue, const AtomString& newValue, AttributeModificationReason = ModifiedDirectly);
> 
> I think I'm OK with this patch but:
> 1. I have slight concerns about perf so it would be nice to trigger A/B
> testing for Speedometer to be on the safe side
> 2. attributeChangedInternal() is too obscure. I'd prefer if we found a
> better name, maybe `notifyAttributeChanged()`
> 3. The patch doesn't seem to apply
Rebased the patch and renamed `attributeChangedInternal` to `notifyAttributeChanged`. Was considering `attributeChangedWithNotification` to make it clear it's attributeChanged, _plus_ more behavior...but feels wordy?

I just queued some Speedometer runs with this latest patch. Will report back here with results.

[Chris Dumez]

Comment on attachment 465909 [details]
Patch

Looks fine assuming A/B testing comes back neutral.

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #7)
> Created attachment 465909 [details]
> Patch
I presume the speedometer results are going to be neutral since the AXObjectCache won't exist in the speedometer configuration.

--- a/Source/WebCore/dom/Element.cpp
+++ b/Source/WebCore/dom/Element.cpp

+void Element::notifyAttributeChanged(const QualifiedName& name, const AtomString& oldValue, const AtomString& newValue, AttributeModificationReason reason)
+{
+    attributeChanged(name, oldValue, newValue, reason);
+
+    if (auto* cache = document().existingAXObjectCache()) {
+        if (oldValue != newValue)

Should we do these ifs in reverse order? The comparison of two AtomString should be fast, but so may be getting the AXObjectcache ?

--- a/Source/WebCore/dom/Element.h
+++ b/Source/WebCore/dom/Element.h

     // This function is called whenever an attribute is added, changed or removed.
+    // For use inside this class, prefer calling notifyAttributeChanged.
     virtual void attributeChanged(const QualifiedName&, const AtomString& oldValue, const AtomString& newValue, AttributeModificationReason = ModifiedDirectly);

Can we make this comment more explicit in that notifyAttributeChanged must be called instead in order to update accessibility.

[Chris Dumez]

(In reply to Andres Gonzalez from comment #10)
> (In reply to Tyler Wilcock from comment #7)
> > Created attachment 465909 [details]
> > Patch
> I presume the speedometer results are going to be neutral since the
> AXObjectCache won't exist in the speedometer configuration.

Results are indeed neutral. This is a very hot code path so we should not make assumptions and test.

> --- a/Source/WebCore/dom/Element.cpp
> +++ b/Source/WebCore/dom/Element.cpp
> 
> +void Element::notifyAttributeChanged(const QualifiedName& name, const
> AtomString& oldValue, const AtomString& newValue,
> AttributeModificationReason reason)
> +{
> +    attributeChanged(name, oldValue, newValue, reason);
> +
> +    if (auto* cache = document().existingAXObjectCache()) {
> +        if (oldValue != newValue)
> 
> Should we do these ifs in reverse order? The comparison of two AtomString
> should be fast, but so may be getting the AXObjectcache ?

I don't mind either way. Getting the AXObjectCache is about as cheap as the AtomString comparison in the case where the AXObjectCache doesn't exist.
Might be a little more expensive when the cache actually exists.

> 
> --- a/Source/WebCore/dom/Element.h
> +++ b/Source/WebCore/dom/Element.h
> 
>      // This function is called whenever an attribute is added, changed or
> removed.
> +    // For use inside this class, prefer calling notifyAttributeChanged.
>      virtual void attributeChanged(const QualifiedName&, const AtomString&
> oldValue, const AtomString& newValue, AttributeModificationReason =
> ModifiedDirectly);
> 
> Can we make this comment more explicit in that notifyAttributeChanged must
> be called instead in order to update accessibility.

Maybe something like this?
// Do not call this function directly. notifyAttributeChanged() should be used instead in order to update accessibility.

[Andres Gonzalez]

(In reply to Chris Dumez from comment #11)
> (In reply to Andres Gonzalez from comment #10)
> > (In reply to Tyler Wilcock from comment #7)
> > > Created attachment 465909 [details]
> > > Patch
> > I presume the speedometer results are going to be neutral since the
> > AXObjectCache won't exist in the speedometer configuration.
> 
> Results are indeed neutral. This is a very hot code path so we should not
> make assumptions and test.
> 
> > --- a/Source/WebCore/dom/Element.cpp
> > +++ b/Source/WebCore/dom/Element.cpp
> > 
> > +void Element::notifyAttributeChanged(const QualifiedName& name, const
> > AtomString& oldValue, const AtomString& newValue,
> > AttributeModificationReason reason)
> > +{
> > +    attributeChanged(name, oldValue, newValue, reason);
> > +
> > +    if (auto* cache = document().existingAXObjectCache()) {
> > +        if (oldValue != newValue)
> > 
> > Should we do these ifs in reverse order? The comparison of two AtomString
> > should be fast, but so may be getting the AXObjectcache ?
> 
> I don't mind either way. Getting the AXObjectCache is about as cheap as the
> AtomString comparison in the case where the AXObjectCache doesn't exist.
> Might be a little more expensive when the cache actually exists.
> 
> > 
> > --- a/Source/WebCore/dom/Element.h
> > +++ b/Source/WebCore/dom/Element.h
> > 
> >      // This function is called whenever an attribute is added, changed or
> > removed.
> > +    // For use inside this class, prefer calling notifyAttributeChanged.
> >      virtual void attributeChanged(const QualifiedName&, const AtomString&
> > oldValue, const AtomString& newValue, AttributeModificationReason =
> > ModifiedDirectly);
> > 
> > Can we make this comment more explicit in that notifyAttributeChanged must
> > be called instead in order to update accessibility.
> 
> Maybe something like this?
> // Do not call this function directly. notifyAttributeChanged() should be
> used instead in order to update accessibility.

Perfect! thanks!

[Tyler Wilcock]

Created attachment 465916 [details]
Patch

[EWS]

Committed 262988@main (b6dc2fc14af4): <https://commits.webkit.org/262988@main>

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