Bug 185521 – AX: Listbox and Combobox roles embedded in labels should participate in name calculation

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Joanmarie Diggs]

Given:

    <input type="checkbox" id="test1" />
    <label for="test1" id="label">Flash the screen
      <ul role="listbox" style="list-style-type: none;">
	<li role="option" aria-selected="false">1</li>
	<li role="option" aria-selected="true">2</li>
	<li role="option">3</li>
      </ul>
      times.
    </label>

The accessible name of the checkbox should be "Flash the screen 2 times." (See step 2.E, bullet 3 of Accname[1]). It's currently "Flash the screen times."

The same issue is been seen with the ARIA combobox role as well as with native select element with size > 1.

[1] https://rawgit.com/w3c/accname/master/#mapping_additional_nd_te

[Radar WebKit Bug Importer]

<rdar://problem/40135781>

[Joanmarie Diggs]

Created attachment 340115 [details]
Patch

[chris fleizach]

Comment on attachment 340115 [details]
Patch

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

> LayoutTests/accessibility/text-alternative-calculation-from-listbox-expected.txt:6
> +Test 1 - W3C Name: 

how come this doesn't apply to Mac?

[Joanmarie Diggs]

(In reply to chris fleizach from comment #3)
> Comment on attachment 340115 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=340115&action=review
> 
> > LayoutTests/accessibility/text-alternative-calculation-from-listbox-expected.txt:6
> > +Test 1 - W3C Name: 
> 
> how come this doesn't apply to Mac?

You seem to expose it only through the AXTitleUIElement. On my platform, it's a flat string.

[EWS Watchlist]

Comment on attachment 340115 [details]
Patch

Attachment 340115 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7641907

New failing tests:
http/tests/preload/onload_event.html

[EWS Watchlist]

Created attachment 340125 [details]
Archive of layout-test-results from ews204 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews204  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Joanmarie Diggs]

I don't *think* my patch could be responsible for the Windows failures.

Chris, did you want me to do something regarding the AXTitleUIElement situation?

[Nan Wang]

Comment on attachment 340115 [details]
Patch

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

>>> LayoutTests/accessibility/text-alternative-calculation-from-listbox-expected.txt:6
>>> +Test 1 - W3C Name: 
>> 
>> how come this doesn't apply to Mac?
> 
> You seem to expose it only through the AXTitleUIElement. On my platform, it's a flat string.

Yes I think on macOS it's reflected in AXTitle
void AccessibilityNodeObject::titleElementText(Vector<AccessibilityText>& textOrder) const

> LayoutTests/accessibility/text-alternative-calculation-from-listbox.html:65
> +            debug("Test " + i + " - W3C Name: " + platformValueForW3CName(axElement));

If you make the change for macOS, platformValueForW3CName won't work since it's taking the description instead of the title.

[Nan Wang]

Comment on attachment 340115 [details]
Patch

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

>>>> LayoutTests/accessibility/text-alternative-calculation-from-listbox-expected.txt:6
>>>> +Test 1 - W3C Name: 
>>> 
>>> how come this doesn't apply to Mac?
>> 
>> You seem to expose it only through the AXTitleUIElement. On my platform, it's a flat string.
> 
> Yes I think on macOS it's reflected in AXTitle
> void AccessibilityNodeObject::titleElementText(Vector<AccessibilityText>& textOrder) const

After some look, I think your changes should already work for macOS?
Because AccessibilityNodeObject::textForLabelElement() is already using accessibleNameForNode(). And both AccessibilityNodeObject::titleElementText() and AccessibilityNodeObject::title() is getting the string from textForLabelElement(). 
Maybe you just need to update the test to account for macOS

[Joanmarie Diggs]

Created attachment 340192 [details]
Patch

[Joanmarie Diggs]

(In reply to Nan Wang from comment #9)

> Maybe you just need to update the test to account for macOS

I updated the test to dump pretty much everything out about the checkboxes (whose labels contain listbox descendants which should participate in the name calculation for the checkboxes). The shared (non-platform-specific) results were generated on my iMac. I'm not seeing the information showing up in AXTitle.

Furthermore, with the exception of test 3 -- which is the one test which worked as expected prior to my proposed fix -- I'm not seeing the "2" showing up anywhere on your platform for the label-y content.

Please advise.

As an aside, I suspect that this more detailed test will fail in Windows. I don't have a Windows environment, but I'll wait for EWS to spit up with the expected results.

[EWS Watchlist]

Comment on attachment 340192 [details]
Patch

Attachment 340192 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/7651697

New failing tests:
accessibility/text-alternative-calculation-from-listbox.html

[EWS Watchlist]

Created attachment 340193 [details]
Archive of layout-test-results from ews116 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Joanmarie Diggs]

Created attachment 340196 [details]
Patch

[EWS Watchlist]

Comment on attachment 340196 [details]
Patch

Attachment 340196 [details] did not pass win-ews (win):
Output: http://webkit-queues.webkit.org/results/7653045

New failing tests:
accessibility/text-alternative-calculation-from-listbox.html

[EWS Watchlist]

Created attachment 340211 [details]
Archive of layout-test-results from ews202 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[Joanmarie Diggs]

Created attachment 340212 [details]
Patch

[Nan Wang]

(In reply to Joanmarie Diggs (irc: joanie) from comment #11)
> (In reply to Nan Wang from comment #9)
> 
> > Maybe you just need to update the test to account for macOS
> 
> I updated the test to dump pretty much everything out about the checkboxes
> (whose labels contain listbox descendants which should participate in the
> name calculation for the checkboxes). The shared (non-platform-specific)
> results were generated on my iMac. I'm not seeing the information showing up
> in AXTitle.
> 

I think the issue here is that exposesTitleUIElement() is returning true so that it's not using the computed text as AXTitle. The AccessibilityLabel object is returned as the titleUIElement, VoiceOver on macOS would then use its description or its children's description to form the announcement.

Couple thoughts I have:
- Maybe we can return false for exposesTitleUIElement() in such case where !containsOnlyStaticText() for the AccessibilityLabel object.
- Or we can return the computed name in AccessibilityLabel::stringValue() while still exposing this object as the titleUIElement.

> Furthermore, with the exception of test 3 -- which is the one test which
> worked as expected prior to my proposed fix -- I'm not seeing the "2"
> showing up anywhere on your platform for the label-y content.
> 
> Please advise.

Isn't there a 
AXTitleUIElement Child 2 - AXValue: 2

> 
> As an aside, I suspect that this more detailed test will fail in Windows. I
> don't have a Windows environment, but I'll wait for EWS to spit up with the
> expected results.

[Joanmarie Diggs]

(In reply to Nan Wang from comment #18)

> Couple thoughts I have:
> - Maybe we can return false for exposesTitleUIElement() in such case where
> !containsOnlyStaticText() for the AccessibilityLabel object.
> - Or we can return the computed name in AccessibilityLabel::stringValue()
> while still exposing this object as the titleUIElement.

Thanks for the suggestions!

Seeing as how it's your platform, do you have a preference, or would you like me to try each independent of this bug and see what happens?

> Isn't there a 
> AXTitleUIElement Child 2 - AXValue: 2

On your platform, only for test 3. And if memory serves me, that's the case even without my proposed fix.

[Nan Wang]

(In reply to Joanmarie Diggs (irc: joanie) from comment #19)
> (In reply to Nan Wang from comment #18)
> 
> > Couple thoughts I have:
> > - Maybe we can return false for exposesTitleUIElement() in such case where
> > !containsOnlyStaticText() for the AccessibilityLabel object.
> > - Or we can return the computed name in AccessibilityLabel::stringValue()
> > while still exposing this object as the titleUIElement.
> 
> Thanks for the suggestions!
> 
> Seeing as how it's your platform, do you have a preference, or would you
> like me to try each independent of this bug and see what happens?

I think if you can make option 1 working it would be great. Because there might be extra work on the AT side for option 2. 

> 
> > Isn't there a 
> > AXTitleUIElement Child 2 - AXValue: 2
> 
> On your platform, only for test 3. And if memory serves me, that's the case
> even without my proposed fix.

I see. I think for AXList, macOS is using AXSelectedChildren to determine the selected child instead of exposing this info to AXValue

[Joanmarie Diggs]

(In reply to Nan Wang from comment #20)
 
> I think if you can make option 1 working it would be great. Because there
> might be extra work on the AT side for option 2. 

Okie dokie. I did a quick trial and broke around 10 tests on your platform. Some of the errors are just due to the new behavior (no longer an AXTitleUIElement, but the AXTitle contains the flat string). Some need more investigation. It also broke the fix for the problem stated in the opening report on my platform. So, I'll definitely pursue this approach, but on Monday. :)

Thanks again for the suggestions!

[Joanmarie Diggs]

I'm now debugging the newly-introduced macOS failures that result from the proposed change. Questions:

1. Should an HTMLLabelElement which contains text and links still be treated as a valid AXTitleUIElement?

2. Should an HTMLLabelElement which lacks a "for" attribute and labels a descendant control be treated as the AXTitleUIElement for that descendant control?

In both of the above cases, the HTMLLabelElement fails the containsOnlyStaticText() test and thus stops being the AXTitleUIElement expected in a number of layout tests. I don't know if I should just update the tests to reflect the new behavior or if I should create a method that only changes the behavior if the HTMLLabelElement contains a control which it is not labeling.

Thoughts?

[Nan Wang]

(In reply to Joanmarie Diggs (irc: joanie) from comment #22)
> I'm now debugging the newly-introduced macOS failures that result from the
> proposed change. Questions:
> 
> 1. Should an HTMLLabelElement which contains text and links still be treated
> as a valid AXTitleUIElement?
> 
> 2. Should an HTMLLabelElement which lacks a "for" attribute and labels a
> descendant control be treated as the AXTitleUIElement for that descendant
> control?
> 
> In both of the above cases, the HTMLLabelElement fails the
> containsOnlyStaticText() test and thus stops being the AXTitleUIElement
> expected in a number of layout tests. I don't know if I should just update
> the tests to reflect the new behavior or if I should create a method that
> only changes the behavior if the HTMLLabelElement contains a control which
> it is not labeling.
> 
> Thoughts?

I think creating a method that only changes the behavior for label elements with controls might be better. Since we don't want to cause any regressions on the AT side as well.

[Joanmarie Diggs]

Created attachment 340368 [details]
Patch

[Joanmarie Diggs]

(In reply to Nan Wang from comment #23)

> I think creating a method that only changes the behavior for label elements
> with controls might be better. Since we don't want to cause any regressions
> on the AT side as well.

Okie dokie. That's what I've done. Only one test has changed behavior and the test cases in question are labels which have descendant controls which are used to calculate the name of controls which are outside of the label (i.e. a variant of this bug).

On a related note, I had to add a hack to the new layout test. For some reason in macOS (at least on my iMac), the checkboxes in the test had the correct AXTitle when I viewed the test using Accessibility Inspector. But if I ran the test as a Layout Test, the checkboxes were mostly (and often entirely) sans AXTitle. The hack (as you'll see in the test) is to touch the accessibility tree. Is this a known issue, and if so, is there a better solution?

Thanks again!

[Nan Wang]

(In reply to Joanmarie Diggs (irc: joanie) from comment #25)
> (In reply to Nan Wang from comment #23)
> 
> > I think creating a method that only changes the behavior for label elements
> > with controls might be better. Since we don't want to cause any regressions
> > on the AT side as well.
> 
> Okie dokie. That's what I've done. Only one test has changed behavior and
> the test cases in question are labels which have descendant controls which
> are used to calculate the name of controls which are outside of the label
> (i.e. a variant of this bug).
> 

I think that's ok.

> On a related note, I had to add a hack to the new layout test. For some
> reason in macOS (at least on my iMac), the checkboxes in the test had the
> correct AXTitle when I viewed the test using Accessibility Inspector. But if
> I ran the test as a Layout Test, the checkboxes were mostly (and often
> entirely) sans AXTitle. The hack (as you'll see in the test) is to touch the
> accessibility tree. Is this a known issue, and if so, is there a better
> solution?

Hmmm this is strange. But it worked fine on other platforms? Is it possible that the layout is not yet finished when we calling those titles? We recently made changes to update Accessibility information post layout only. 

> 
> Thanks again!

[Nan Wang]

(In reply to Joanmarie Diggs (irc: joanie) from comment #25)
> (In reply to Nan Wang from comment #23)
> 
> > I think creating a method that only changes the behavior for label elements
> > with controls might be better. Since we don't want to cause any regressions
> > on the AT side as well.
> 
> Okie dokie. That's what I've done. Only one test has changed behavior and
> the test cases in question are labels which have descendant controls which
> are used to calculate the name of controls which are outside of the label
> (i.e. a variant of this bug).
> 

I think that's ok.

> On a related note, I had to add a hack to the new layout test. For some
> reason in macOS (at least on my iMac), the checkboxes in the test had the
> correct AXTitle when I viewed the test using Accessibility Inspector. But if
> I ran the test as a Layout Test, the checkboxes were mostly (and often
> entirely) sans AXTitle. The hack (as you'll see in the test) is to touch the
> accessibility tree. Is this a known issue, and if so, is there a better
> solution?

Hmmm this is strange. But it worked fine on other platforms? Is it possible that the layout is not yet finished when we calling those titles? We recently made changes to update Accessibility information post layout only. 

> 
> Thanks again!

[Nan Wang]

Comment on attachment 340368 [details]
Patch

r=me. I think I'll let Chris take a final look.

[chris fleizach]

Comment on attachment 340368 [details]
Patch

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

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1419
> +    // find out if this element is inside of a label element.

Find out

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1420
> +    // if so, it may be ignored because it's the label for a checkbox or radio button

If so

[Joanmarie Diggs]

Created attachment 340380 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 340380 [details]
patch for landing

Clearing flags on attachment: 340380

Committed r231778: <https://trac.webkit.org/changeset/231778>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.