Summary: | AX: On generic container elements, WebKit should distinguish between tooltip (e.g. @title) and label (e.g. @aria-label) attributes | ||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Craig <jcraig> | ||||||||||||||||||||||||||||||||
Component: | Accessibility | Assignee: | chris fleizach <cfleizach> | ||||||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||||||
Severity: | Normal | CC: | aboxhall, apinheiro, buildbot, cfleizach, commit-queue, dmazzoni, jcraig, jdiggs, rniwa, samuel_white, webkit-bug-importer | ||||||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||||||||||||
Attachments: |
|
Description
James Craig
2017-04-04 15:05:46 PDT
Created attachment 315812 [details]
patch
Attachment 315812 [details] did not pass style-queue:
ERROR: Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1566: Weird number of spaces at line-start. Are you using a 4-space indent? [whitespace/indent] [3]
ERROR: Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1582: When wrapping a line, only indent 4 spaces. [whitespace/indent] [3]
Total errors found: 2 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 315863 [details]
patch
Comment on attachment 315863 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=315863&action=review > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1576 > + case ParagraphRole: I'm not 100% sure we want to say ParagraphRole is "semantically unimportant." Though strictly for the purpose of where to expose the title attribute on a ParagraphRole, I agree that it's no different from DivRole. So I wonder if a tweak to the function name is called for. Regardless of the function name, this doesn't appear to include spans (inline or block). And what should happen with style format groups? In the latter case, even if no code change is needed here, I still think it would be worth adding some test cases for them. Were it me, I'd probably also get James to weigh in on the style format group question because in a different context (roledescription sans ARIA role) he indicated that the 'code' element was semantically meaningful. But I'm not sure he would say the same about the 'pre' element. And I'd have to look at the spec and/or check with James myself to see if, for the purpose of the title attribute exposure, 'pre' and 'code' are like 'div' or not. James, can you comment which roles you think should participate in title attribute nullification? Comment on attachment 315863 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=315863&action=review >> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1576 >> + case ParagraphRole: > > I'm not 100% sure we want to say ParagraphRole is "semantically unimportant." Though strictly for the purpose of where to expose the title attribute on a ParagraphRole, I agree that it's no different from DivRole. So I wonder if a tweak to the function name is called for. > > Regardless of the function name, this doesn't appear to include spans (inline or block). And what should happen with style format groups? In the latter case, even if no code change is needed here, I still think it would be worth adding some test cases for them. > > Were it me, I'd probably also get James to weigh in on the style format group question because in a different context (roledescription sans ARIA role) he indicated that the 'code' element was semantically meaningful. But I'm not sure he would say the same about the 'pre' element. And I'd have to look at the spec and/or check with James myself to see if, for the purpose of the title attribute exposure, 'pre' and 'code' are like 'div' or not. I agree with Joanie that the change here could be: 1. Change the name to something more explicit like ignoreTitleOnRole(). 2. Remove ParagraphRole. 3. Add InlineRole and UnknownRole. We can address the other potentially ignorable roles individually in other patches. Comment on attachment 315863 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=315863&action=review >>> Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1576 >>> + case ParagraphRole: >> >> I'm not 100% sure we want to say ParagraphRole is "semantically unimportant." Though strictly for the purpose of where to expose the title attribute on a ParagraphRole, I agree that it's no different from DivRole. So I wonder if a tweak to the function name is called for. >> >> Regardless of the function name, this doesn't appear to include spans (inline or block). And what should happen with style format groups? In the latter case, even if no code change is needed here, I still think it would be worth adding some test cases for them. >> >> Were it me, I'd probably also get James to weigh in on the style format group question because in a different context (roledescription sans ARIA role) he indicated that the 'code' element was semantically meaningful. But I'm not sure he would say the same about the 'pre' element. And I'd have to look at the spec and/or check with James myself to see if, for the purpose of the title attribute exposure, 'pre' and 'code' are like 'div' or not. > > I agree with Joanie that the change here could be: > > 1. Change the name to something more explicit like ignoreTitleOnRole(). > 2. Remove ParagraphRole. > 3. Add InlineRole and UnknownRole. > > We can address the other potentially ignorable roles individually in other patches. Actually, including the other two might regress <abbr> and <acronym>. What about shipping this patch with only GroupRole support and including the others later... E.g. InlineRole could be added after separating <abbr> and <acronym> into a new AbbreviationRole. Joanie wrote:
> in a different context (roledescription sans ARIA role) [James] indicated that the 'code' element was semantically meaningful.
That comment was partially theoretical. For example, we don't currently, but we could pass the "code" context along to the speech engine to help with pronunciation, which would allow more fine-grained speech control than you can do with author-provided CSS Speech. Ditto for the different form element types: tel, etc.
Created attachment 316433 [details]
patch
(In reply to Joanmarie Diggs (irc: joanie) from comment #5) > Comment on attachment 315863 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=315863&action=review > > > Source/WebCore/accessibility/AccessibilityNodeObject.cpp:1576 > > + case ParagraphRole: > > I'm not 100% sure we want to say ParagraphRole is "semantically > unimportant." Though strictly for the purpose of where to expose the title > attribute on a ParagraphRole, I agree that it's no different from DivRole. > So I wonder if a tweak to the function name is called for. > > Regardless of the function name, this doesn't appear to include spans > (inline or block). And what should happen with style format groups? In the > latter case, even if no code change is needed here, I still think it would > be worth adding some test cases for them. > > Were it me, I'd probably also get James to weigh in on the style format > group question because in a different context (roledescription sans ARIA > role) he indicated that the 'code' element was semantically meaningful. But > I'm not sure he would say the same about the 'pre' element. And I'd have to > look at the spec and/or check with James myself to see if, for the purpose > of the title attribute exposure, 'pre' and 'code' are like 'div' or not. Updated patch Comment on attachment 316433 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=316433&action=review r+ BUT it would be great if you could tweak the test to use the shared-platform-friendlier helper methods mentioned inline. Thanks in advance! > LayoutTests/accessibility/title-tag-on-unimportant-elements-is-help-text.html:23 > + shouldBe("accessibilityController.accessibleElementById('div1').helpText", "'AXHelp: test1'"); Your platform's "AXHelp" is my platform's "description" is the W3C AccName spec's "description". A while back, I added some methods to resources/accessibility-helper.js to address this very issue. For the above test case, platformValueForW3CDescription() should do it: https://trac.webkit.org/browser/trunk/LayoutTests/resources/accessibility-helper.js#L53. If you want to keep the shouldBe() and have a single, shared expectations file, then the default value for the includeSource argument is what you want, though it will cause the "AXHelp: " prefix to be dropped. If you want to preserve the "AXHelp: " prefix for your platform, setting includeSource to true will accomplish that, though shouldBe() would need to be replaced with debug() so platform-specific expectations can be created by gardeners. > LayoutTests/accessibility/title-tag-on-unimportant-elements-is-help-text.html:24 > + shouldBe("accessibilityController.accessibleElementById('div1').description", "'AXDescription: '"); Ditto, but platformValueForW3CName(). https://trac.webkit.org/browser/trunk/LayoutTests/resources/accessibility-helper.js#L38 Created attachment 320429 [details]
patch for updated comments
Comment on attachment 320429 [details] patch for updated comments Attachment 320429 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4512769 New failing tests: accessibility/title-tag-on-unimportant-elements-is-help-text.html Created attachment 320434 [details]
Archive of layout-test-results from ews100 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 320429 [details] patch for updated comments Attachment 320429 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4512776 New failing tests: accessibility/title-tag-on-unimportant-elements-is-help-text.html Created attachment 320435 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 320437 [details]
patch for updated comments
Comment on attachment 320437 [details] patch for updated comments Attachment 320437 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4513136 New failing tests: accessibility/title-tag-on-unimportant-elements-is-help-text.html Created attachment 320440 [details]
Archive of layout-test-results from ews103 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 320437 [details] patch for updated comments Attachment 320437 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4513147 New failing tests: accessibility/title-tag-on-unimportant-elements-is-help-text.html Created attachment 320443 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Created attachment 320444 [details]
patch for updated comments
Comment on attachment 320444 [details] patch for updated comments Attachment 320444 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.webkit.org/results/4513682 New failing tests: accessibility/title-tag-on-unimportant-elements-is-help-text.html Created attachment 320448 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-elcapitan-wk2 Platform: Mac OS X 10.11.6
Comment on attachment 320444 [details] patch for updated comments Attachment 320444 [details] did not pass mac-ews (mac): Output: http://webkit-queues.webkit.org/results/4513826 New failing tests: accessibility/title-tag-on-unimportant-elements-is-help-text.html Created attachment 320450 [details]
Archive of layout-test-results from ews100 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 320444 [details] patch for updated comments Attachment 320444 [details] did not pass mac-debug-ews (mac): Output: http://webkit-queues.webkit.org/results/4513921 New failing tests: accessibility/title-tag-on-unimportant-elements-is-help-text.html Created attachment 320453 [details]
Archive of layout-test-results from ews114 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews114 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Created attachment 320467 [details]
patch for updated comments
Created attachment 320503 [details]
patch for landing
Comment on attachment 320503 [details] patch for landing Clearing flags on attachment: 320503 Committed r221918: <http://trac.webkit.org/changeset/221918> All reviewed patches have been landed. Closing bug. |