| Summary: | AX: The title of an input contained in a label should participate in the accessible name calculation | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Joanmarie Diggs <jdiggs> | ||||||||||||||||||
| Component: | Accessibility | Assignee: | Joanmarie Diggs <jdiggs> | ||||||||||||||||||
| Status: | RESOLVED INVALID | ||||||||||||||||||||
| Severity: | Normal | CC: | aboxhall, apinheiro, cfleizach, 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
Joanmarie Diggs
2018-09-06 15:37:39 PDT
Created attachment 349085 [details]
Patch
Comment on attachment 349085 [details] Patch Attachment 349085 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/9120037 New failing tests: accessibility/label-with-input-with-title.html Created attachment 349096 [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.10.0-0.325-5-3-x86_64-64bit
Created attachment 349098 [details]
Patch
Comment on attachment 349098 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349098&action=review > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1788 > + if (childText.isEmpty() && is<HTMLInputElement>(child->node())) Do we want to consider elements other than inputs? > LayoutTests/platform/mac/accessibility/label-with-input-with-title-expected.txt:9 > + AXTitleUIElement: non-null Can you try to see if the name calculation is included in the stringValue() on mac platform? Comment on attachment 349098 [details] Patch Attachment 349098 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/9121458 New failing tests: accessibility/label-with-input-with-title.html Created attachment 349106 [details]
Archive of layout-test-results from ews201 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews201 Port: win-future Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment on attachment 349098 [details] Patch Attachment 349098 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/9122159 New failing tests: accessibility/label-with-input-with-title.html Created attachment 349108 [details]
Archive of layout-test-results from ews200 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews200 Port: win-future Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit
(In reply to Nan Wang from comment #6) > Comment on attachment 349098 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=349098&action=review > > > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1788 > > + if (childText.isEmpty() && is<HTMLInputElement>(child->node())) > > Do we want to consider elements other than inputs? Maybe. What I'm doing at the moment is debugging and fixing AccName spec test failures so that AccName will be able to successfully exit CR and become a proposed recommendation. As I continue doing so, I may stumble across other instances where we're failing to include the title attribute of non-input elements. In which case the answer to your question will become "yes" and I'll submit a patch for that. If you have other elements you'd like me to consider, I'm happy to do so -- though we should probably ensure that jibes with what's in AccName and HTML-AAM and file issues against those specs if it doesn't. > > LayoutTests/platform/mac/accessibility/label-with-input-with-title-expected.txt:9 > > + AXTitleUIElement: non-null > > Can you try to see if the name calculation is included in the stringValue() > on mac platform? I'm not seeing it. Speaking of which: It was my assumption that in this instance, VoiceOver would build up the label to be presented from the descendants of the AXTitleUIElement, but VoiceOver is not presenting the "bar" when each widget in the test case gets focus. So.... Where should this be exposed so that VoiceOver says "foo bar baz" when presenting the label of the focused widget? Created attachment 349164 [details]
trying to make win EWS happy
Comment on attachment 349164 [details] trying to make win EWS happy Attachment 349164 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/9131414 New failing tests: accessibility/label-with-input-with-title.html Created attachment 349181 [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.10.0-0.325-5-3-x86_64-64bit
Created attachment 349192 [details]
trying to make win EWS happy
Comment on attachment 349192 [details] trying to make win EWS happy Hey Chris. If my answer to my earlier question is easy, then I'll do a new patch here to fix both of our platforms. Otherwise, please review. For convenience, this is the question I'm wondering about: > It was my assumption that in this instance, VoiceOver would build > up the label to be presented from the descendants of the AXTitleUIElement, > but VoiceOver is not presenting the "bar" when each widget in the test case > gets focus. So.... Where should this be exposed so that VoiceOver says > "foo bar baz" when presenting the label of the focused widget? Thanks! (In reply to Joanmarie Diggs (irc: joanie) from comment #16) > Comment on attachment 349192 [details] > trying to make win EWS happy > > Hey Chris. > > If my answer to my earlier question is easy, then I'll do a new patch here > to fix both of our platforms. Otherwise, please review. > > For convenience, this is the question I'm wondering about: > > > It was my assumption that in this instance, VoiceOver would build > > up the label to be presented from the descendants of the AXTitleUIElement, > > but VoiceOver is not presenting the "bar" when each widget in the test case > > gets focus. So.... Where should this be exposed so that VoiceOver says > > "foo bar baz" when presenting the label of the focused widget? Yea I think in this case the title UI element is exposed and VO calculates the name based on all the elements. So I guess this is correct, but I admit it's a bit of a weird discrepancy. > > Thanks! Comment on attachment 349098 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349098&action=review >>> LayoutTests/platform/mac/accessibility/label-with-input-with-title-expected.txt:9 >>> + AXTitleUIElement: non-null >> >> Can you try to see if the name calculation is included in the stringValue() on mac platform? > > I'm not seeing it. Speaking of which: It was my assumption that in this instance, VoiceOver would build up the label to be presented from the descendants of the AXTitleUIElement, but VoiceOver is not presenting the "bar" when each widget in the test case gets focus. So.... Where should this be exposed so that VoiceOver says "foo bar baz" when presenting the label of the focused widget? I think there's a containsOnlyStaticText() in AccessibilityLabel. Maybe we should add this case in that check? Comment on attachment 349098 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=349098&action=review >>>> LayoutTests/platform/mac/accessibility/label-with-input-with-title-expected.txt:9 >>>> + AXTitleUIElement: non-null >>> >>> Can you try to see if the name calculation is included in the stringValue() on mac platform? >> >> I'm not seeing it. Speaking of which: It was my assumption that in this instance, VoiceOver would build up the label to be presented from the descendants of the AXTitleUIElement, but VoiceOver is not presenting the "bar" when each widget in the test case gets focus. So.... Where should this be exposed so that VoiceOver says "foo bar baz" when presenting the label of the focused widget? > > I think there's a containsOnlyStaticText() in AccessibilityLabel. Maybe we should add this case in that check? Anyway I think you are right that things are exposed in AXTitleUIElement Hey Chris and Nan. Thanks for the info! I looked into it some more today. Responses inline below. (In reply to chris fleizach from comment #17) > Yea I think in this case the title UI element is exposed Correct. > and VO calculates the name based on all the elements. Ok, but VO is not fully succeeding. The descendants of the AXTitleUIElement (an AccessibilityLabel) are: * "foo" (text) * "bar" (the title attribute value of an input with type button or checkbox) * "baz" (text) VO's calculation is "foo baz". CANDIDATE SOLUTION 1: In this particular case, "bar" is exposed as the AXHelp value of the input. So VO *could* present it without our making any further changes in WebKit, if you change VO to fall back on AXHelp. (In reply to Nan Wang from comment #18) > I think there's a containsOnlyStaticText() in AccessibilityLabel. Maybe we > should add this case in that check? Do you mean modify containsOnlyStaticText()? If so, I don't think that's the right thing to do. In the test cases in question, the AccessibilityLabel does NOT contain only static text. It contains widgets. As a result containsOnlyStaticText() returns false -- I would argue that is the correct return value and that this case is already in that check. Or were you instead suggesting we add a call to containsOnlyStaticText() in AccessibilityRenderObject::exposesTitleUIElement()? i.e. if the AccessibilityLabel does NOT contain only static text, it doesn't expose a title UI element? If so, that works for me. We already treat labels which contain unrelated controls as not being a valid title UI element. CANDIDATE SOLUTION 2: Complicated labels should not be treated a valid title UI element; instead their calculated value should be exposed via AXTitle. As an experiment, I tried seeing what would happen if the calculated result included an AXTitle value but the AXTitleUIElement continued to be exposed. The answer, which I'm guessing you already knew, is that VO presents both. I'm also guessing that it is by design and thus modifying VO to ignore AXTitleUIElement when there is an AXTitle is not a solution. But if I'm wrong about that, please let me know. So.... Do you want the fix in VO or in WebKit. If the latter, are you cool with CANDIDATE SOLUTION 2? (In reply to Joanmarie Diggs (irc: joanie) from comment #20) > Hey Chris and Nan. > > Thanks for the info! I looked into it some more today. Responses inline > below. > > (In reply to chris fleizach from comment #17) > > > Yea I think in this case the title UI element is exposed > > Correct. > > > and VO calculates the name based on all the elements. > > Ok, but VO is not fully succeeding. The descendants of the AXTitleUIElement > (an AccessibilityLabel) are: > > * "foo" (text) > * "bar" (the title attribute value of an input with type button or checkbox) > * "baz" (text) > > VO's calculation is "foo baz". > > CANDIDATE SOLUTION 1: In this particular case, "bar" is exposed as the > AXHelp value of the input. So VO *could* present it without our making any > further changes in WebKit, if you change VO to fall back on AXHelp. > Should title be exposed as AXHelp here? it seems that if this is the name calculation algorithm, title is no longer considered "help" text but part of the label... > (In reply to Nan Wang from comment #18) > > > I think there's a containsOnlyStaticText() in AccessibilityLabel. Maybe we > > should add this case in that check? > > Do you mean modify containsOnlyStaticText()? If so, I don't think that's the > right thing to do. In the test cases in question, the AccessibilityLabel > does NOT contain only static text. It contains widgets. As a result > containsOnlyStaticText() returns false -- I would argue that is the correct > return value and that this case is already in that check. > > Or were you instead suggesting we add a call to containsOnlyStaticText() in > AccessibilityRenderObject::exposesTitleUIElement()? i.e. if the > AccessibilityLabel does NOT contain only static text, it doesn't expose a > title UI element? If so, that works for me. We already treat labels which > contain unrelated controls as not being a valid title UI element. > > CANDIDATE SOLUTION 2: Complicated labels should not be treated a valid title > UI element; instead their calculated value should be exposed via AXTitle. > > As an experiment, I tried seeing what would happen if the calculated result > included an AXTitle value but the AXTitleUIElement continued to be exposed. > The answer, which I'm guessing you already knew, is that VO presents both. > I'm also guessing that it is by design and thus modifying VO to ignore > AXTitleUIElement when there is an AXTitle is not a solution. But if I'm > wrong about that, please let me know. > > So.... Do you want the fix in VO or in WebKit. If the latter, are you cool > with CANDIDATE SOLUTION 2? (In reply to chris fleizach from comment #21) > Should title be exposed as AXHelp here? it seems that if this is the name > calculation algorithm, title is no longer considered "help" text but part of > the label... Good question. I just asked it: https://github.com/w3c/html-aam/issues/148 Executive summary is that if you read the HTML AAM literally, I believe it should indeed be exposed as AXHelp here. Whether or not that's what is correct or intended is another matter. On a different note, I gave CANDIDATE SOLUTION 2 a try. In macOS it fixes (in my mind) the test case in this bug, namely AXTitle is "foo bar baz" and AXTitleUIElement is null. But it does break 5 tests, all of which expect an AXTitleUIElement which no longer exists: * double-title.html * label-elements-exposed-as-title-ui-elements.html * radio-button-title-label.html * secure-textfield-title-ui.html * title-ui-element-correctness.html You happen to have an opinion on if this solution is correct and we just need to update the tests, or if this is the wrong approach? (In reply to Joanmarie Diggs (irc: joanie) from comment #23) > On a different note, I gave CANDIDATE SOLUTION 2 a try. In macOS it fixes > (in my mind) the test case in this bug, namely AXTitle is "foo bar baz" and > AXTitleUIElement is null. But it does break 5 tests, all of which expect an > AXTitleUIElement which no longer exists: > > * double-title.html > * label-elements-exposed-as-title-ui-elements.html > * radio-button-title-label.html > * secure-textfield-title-ui.html > * title-ui-element-correctness.html > > You happen to have an opinion on if this solution is correct and we just > need to update the tests, or if this is the wrong approach? Let me ask around here I'm going to close this as invalid for now. The ARIA Working Group's test expectations state that the "foo bar baz" is the right result. And based on AccName step 2E, it is. But AccName step 2D seems to preempt that calculation from occurring. Sorry for the noise! (In reply to chris fleizach from comment #24) > (In reply to Joanmarie Diggs (irc: joanie) from comment #23) > > On a different note, I gave CANDIDATE SOLUTION 2 a try. In macOS it fixes > > (in my mind) the test case in this bug, namely AXTitle is "foo bar baz" and > > AXTitleUIElement is null. But it does break 5 tests, all of which expect an > > AXTitleUIElement which no longer exists: > > > > * double-title.html > > * label-elements-exposed-as-title-ui-elements.html > > * radio-button-title-label.html > > * secure-textfield-title-ui.html > > * title-ui-element-correctness.html > > > > You happen to have an opinion on if this solution is correct and we just > > need to update the tests, or if this is the wrong approach? > > Let me ask around here It seems like it might be ok to collapse title elements into one label on the Mac The next time we have this issue might be worth checking out |