Bug 185521

<rdar://problem/40135781>

Created attachment 340115 [details] Patch

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?

(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 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

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

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 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 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

Created attachment 340192 [details] Patch

(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 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

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

Created attachment 340196 [details] Patch

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

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

Created attachment 340212 [details] Patch

(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.

(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.

(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

(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!

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?

(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.

Created attachment 340368 [details] Patch

(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!

(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!

(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 on attachment 340368 [details] Patch r=me. I think I'll let Chris take a final look.

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

Created attachment 340380 [details] patch for landing

Comment on attachment 340380 [details] patch for landing Clearing flags on attachment: 340380 Committed r231778: <https://trac.webkit.org/changeset/231778>

All reviewed patches have been landed. Closing bug.