Bug 185521

Summary: AX: Listbox and Combobox roles embedded in labels should participate in name calculation
Product: WebKit Reporter: Joanmarie Diggs <jdiggs>
Component: AccessibilityAssignee: Joanmarie Diggs <jdiggs>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, apinheiro, cfleizach, commit-queue, dmazzoni, ews-watchlist, jcraig, n_wang, samuel_white, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ews204 for win-future
none
Patch
none
Archive of layout-test-results from ews116 for mac-sierra
none
Patch
none
Archive of layout-test-results from ews202 for win-future
none
Patch
none
Patch
none
patch for landing none

Description Joanmarie Diggs 2018-05-10 11:49:14 PDT
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
Comment 1 Radar WebKit Bug Importer 2018-05-10 11:49:27 PDT
<rdar://problem/40135781>
Comment 2 Joanmarie Diggs 2018-05-10 11:56:01 PDT
Created attachment 340115 [details]
Patch
Comment 3 chris fleizach 2018-05-10 13:04:40 PDT
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?
Comment 4 Joanmarie Diggs 2018-05-10 13:08:58 PDT
(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.
Comment 5 EWS Watchlist 2018-05-10 13:54:18 PDT
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
Comment 6 EWS Watchlist 2018-05-10 13:54:29 PDT
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
Comment 7 Joanmarie Diggs 2018-05-10 14:07:24 PDT
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?
Comment 8 Nan Wang 2018-05-10 14:49:22 PDT
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.
Comment 9 Nan Wang 2018-05-10 15:02:18 PDT
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
Comment 10 Joanmarie Diggs 2018-05-11 08:19:20 PDT
Created attachment 340192 [details]
Patch
Comment 11 Joanmarie Diggs 2018-05-11 08:27:28 PDT
(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.
Comment 12 EWS Watchlist 2018-05-11 10:00:08 PDT
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
Comment 13 EWS Watchlist 2018-05-11 10:00:09 PDT
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
Comment 14 Joanmarie Diggs 2018-05-11 10:14:55 PDT
Created attachment 340196 [details]
Patch
Comment 15 EWS Watchlist 2018-05-11 11:58:49 PDT
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
Comment 16 EWS Watchlist 2018-05-11 11:59:00 PDT
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
Comment 17 Joanmarie Diggs 2018-05-11 12:16:49 PDT
Created attachment 340212 [details]
Patch
Comment 18 Nan Wang 2018-05-11 12:48:29 PDT
(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.
Comment 19 Joanmarie Diggs 2018-05-11 13:15:52 PDT
(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.
Comment 20 Nan Wang 2018-05-11 13:41:50 PDT
(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
Comment 21 Joanmarie Diggs 2018-05-11 15:16:02 PDT
(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!
Comment 22 Joanmarie Diggs 2018-05-14 10:59:33 PDT
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?
Comment 23 Nan Wang 2018-05-14 11:06:12 PDT
(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.
Comment 24 Joanmarie Diggs 2018-05-14 15:42:25 PDT
Created attachment 340368 [details]
Patch
Comment 25 Joanmarie Diggs 2018-05-14 15:47:40 PDT
(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!
Comment 26 Nan Wang 2018-05-14 16:29:34 PDT
(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!
Comment 27 Nan Wang 2018-05-14 16:29:40 PDT
(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!
Comment 28 Nan Wang 2018-05-14 16:30:05 PDT
Comment on attachment 340368 [details]
Patch

r=me. I think I'll let Chris take a final look.
Comment 29 chris fleizach 2018-05-14 16:32:58 PDT
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
Comment 30 Joanmarie Diggs 2018-05-14 17:04:29 PDT
Created attachment 340380 [details]
patch for landing
Comment 31 WebKit Commit Bot 2018-05-14 17:32:12 PDT
Comment on attachment 340380 [details]
patch for landing

Clearing flags on attachment: 340380

Committed r231778: <https://trac.webkit.org/changeset/231778>
Comment 32 WebKit Commit Bot 2018-05-14 17:32:14 PDT
All reviewed patches have been landed.  Closing bug.