Bug 158634 – AX: Form label text should be exposed as static text if it contains only static text

• Component: Accessibility
• Status: RESOLVED FIXED

[Doug Russell]

LabelRole is currently exposed as a group, but would be more appropriate as static text if all the elements it contains are static text. This will save AT the complexity of evaluating the labels subtree and stitching the text manually.

[Radar WebKit Bug Importer]

<rdar://problem/26747020>

[Doug Russell]

Created attachment 281045 [details]
Patch

[chris fleizach]

Comment on attachment 281045 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Use AccessibilityLabel to represent HTMLLabelElement to AT.

AT -> assistive technologies

> Source/WebCore/accessibility/AccessibilityLabel.cpp:71
> +        if (!child->accessibilityIsIgnored() && !child->isDetached()) {

I think we're guaranteed to only have unignored children in m_children

The same might be true of detached children

> Source/WebCore/accessibility/AccessibilityLabel.cpp:76
> +                    staticText = true;

Seems like you don't need to keep track of staticText. As soon as you get a failure can you can just return false

Then you can return true at the end of it right

Also, you could probably cache this value and then update when children are updated

> Source/WebCore/accessibility/AccessibilityLabel.cpp:92
> +    return WebCore::containsOnlyStaticText(m_children);

This WebCore:: prefix is probably unnecessary

> Source/WebCore/accessibility/AccessibilityLabel.h:41
> +    AccessibilityRole roleValue() const override { return LabelRole; }

Add a final to these

roleValue and stringValue() can probably be private

> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2237
> +    if (role == LabelRole && is<AccessibilityLabel>(*m_object) && downcast<AccessibilityLabel>(*m_object).containsOnlyStaticText())

This check 
role == LabelRole

Is probably not necessary since is<AccessibilityLabel> guarantees role = Label

[Doug Russell]

Comment on attachment 281045 [details]
Patch

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

>> Source/WebCore/accessibility/AccessibilityLabel.cpp:76
>> +                    staticText = true;
> 
> Seems like you don't need to keep track of staticText. As soon as you get a failure can you can just return false
> 
> Then you can return true at the end of it right
> 
> Also, you could probably cache this value and then update when children are updated

Having bool staticText means I don't have to check if children is empty. I could switch to size == 0 at the top.

Caching seemed tricky given there seem to be a lot of cases where the funnels aren't used and m_children.append() is used directly. And this method needs to be const to be called from stringValue() so calculating lazily and caching is tricky.

>> Source/WebCore/accessibility/AccessibilityLabel.cpp:92
>> +    return WebCore::containsOnlyStaticText(m_children);
> 
> This WebCore:: prefix is probably unnecessary

You need it to disambiguate from AccessibilityLabel::containsOnlyStaticText() (the compiler gets huffy without it)

>> Source/WebCore/accessibility/AccessibilityLabel.h:41
>> +    AccessibilityRole roleValue() const override { return LabelRole; }
> 
> Add a final to these
> 
> roleValue and stringValue() can probably be private

Will do on final.

I'll see about role and string.

>> Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2237
>> +    if (role == LabelRole && is<AccessibilityLabel>(*m_object) && downcast<AccessibilityLabel>(*m_object).containsOnlyStaticText())
> 
> This check 
> role == LabelRole
> 
> Is probably not necessary since is<AccessibilityLabel> guarantees role = Label

It's not strictly necessary, but it seemed likely that role == Label was cheaper than calling is<> on every m_object.

[chris fleizach]

Comment on attachment 281045 [details]
Patch

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

>>> Source/WebCore/accessibility/AccessibilityLabel.cpp:76
>>> +                    staticText = true;
>> 
>> Seems like you don't need to keep track of staticText. As soon as you get a failure can you can just return false
>> 
>> Then you can return true at the end of it right
>> 
>> Also, you could probably cache this value and then update when children are updated
> 
> Having bool staticText means I don't have to check if children is empty. I could switch to size == 0 at the top.
> 
> Caching seemed tricky given there seem to be a lot of cases where the funnels aren't used and m_children.append() is used directly. And this method needs to be const to be called from stringValue() so calculating lazily and caching is tricky.

>> I could switch to size == 0 at the top.

That seems better to me

>>> Source/WebCore/accessibility/AccessibilityLabel.cpp:92
>>> +    return WebCore::containsOnlyStaticText(m_children);
>> 
>> This WebCore:: prefix is probably unnecessary
> 
> You need it to disambiguate from AccessibilityLabel::containsOnlyStaticText() (the compiler gets huffy without it)

Might be better named

objectsContainOnlyStaticText or elementsContainOnlyStaticText

Since children is no longer accurate, the method being static rattan than an instance

[Doug Russell]

Comment on attachment 281045 [details]
Patch

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

>>>> Source/WebCore/accessibility/AccessibilityLabel.cpp:92
>>>> +    return WebCore::containsOnlyStaticText(m_children);
>>> 
>>> This WebCore:: prefix is probably unnecessary
>> 
>> You need it to disambiguate from AccessibilityLabel::containsOnlyStaticText() (the compiler gets huffy without it)
> 
> Might be better named
> 
> objectsContainOnlyStaticText or elementsContainOnlyStaticText
> 
> Since children is no longer accurate, the method being static rattan than an instance

The argument type is const AccessibilityObject::AccessibilityChildrenVector& so I think children is fair to use. I'll rename is childrenContainOnlyStaticText.

[Doug Russell]

Created attachment 281050 [details]
Patch

[Doug Russell]

Created attachment 281067 [details]
Patch

[chris fleizach]

Comment on attachment 281067 [details]
Patch

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

> Source/WebCore/CMakeLists.txt:1049
> +    accessibility/AccessibilityLabel.cpp

Do we need to all to AccessibilityAllInOne.cpp for windows builds?

> Source/WebCore/accessibility/AccessibilityLabel.h:46
> +    AccessibilityRole roleValue() const final { return LabelRole; }

These should have the override keyword on them? Or does final take care of that

[Doug Russell]

Comment on attachment 281067 [details]
Patch

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

>> Source/WebCore/CMakeLists.txt:1049
>> +    accessibility/AccessibilityLabel.cpp
> 
> Do we need to all to AccessibilityAllInOne.cpp for windows builds?

I believe so. I could try submitting the patch without it.

>> Source/WebCore/accessibility/AccessibilityLabel.h:46
>> +    AccessibilityRole roleValue() const final { return LabelRole; }
> 
> These should have the override keyword on them? Or does final take care of that

My understanding is that final implies override.

[Doug Russell]

Created attachment 281154 [details]
Patch

[chris fleizach]

Comment on attachment 281154 [details]
Patch

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

We should add to these tests so we check that if
A) <label> element does not contain only text children we do not concat values
B) when elements inside <label> change we are validating that we get the correct data

> LayoutTests/accessibility/mac/label-element-with-link-string-value.html:21
> +    description("This tests that if a label element contains text children it's AXValue will be their concatenated AXValues.");

This description is the same as the previous test (LayoutTests/accessibility/mac/label-element-all-text-string-value.html) - seems like it should be different

[chris fleizach]

Comment on attachment 281154 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityLabel.cpp:100
> +    m_containsOnlyStaticTextDirty = false;

Shouldn't this one be set to true (m_containsOnlyStaticTextDirty) because we've just cleared the children and we want to recalculate?

[chris fleizach]

Comment on attachment 281154 [details]
Patch

Were any of my comments addressed?

[Doug Russell]

(In reply to comment #14)
> Comment on attachment 281154 [details]
> Patch
> 
> Were any of my comments addressed?

Forgot to hit submit before sending it back. Fixing now.

[Doug Russell]

Comment on attachment 281154 [details]
Patch

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

>> Source/WebCore/accessibility/AccessibilityLabel.cpp:100
>> +    m_containsOnlyStaticTextDirty = false;
> 
> Shouldn't this one be set to true (m_containsOnlyStaticTextDirty) because we've just cleared the children and we want to recalculate?

No, it should be false because there are no children so m_containsOnlyStaticText is definitely false and we don't need to recalculate.

[Doug Russell]

> We should add to these tests so we check that if
> A) <label> element does not contain only text children we do not concat
> values

label-element-with-link-string-value.html addresses this

> B) when elements inside <label> change we are validating that we get the
> correct data

I'll add a test for that

> 
> > LayoutTests/accessibility/mac/label-element-with-link-string-value.html:21
> > +    description("This tests that if a label element contains text children it's AXValue will be their concatenated AXValues.");
> 
> This description is the same as the previous test
> (LayoutTests/accessibility/mac/label-element-all-text-string-value.html) -
> seems like it should be different

Fixed in my next patch.

[Doug Russell]

Created attachment 281279 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 281279 [details]
Patch

Clearing flags on attachment: 281279

Committed r202063: <http://trac.webkit.org/changeset/202063>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.