Bug 170475 - AX: On generic container elements, WebKit should distinguish between tooltip (e.g. @title) and label (e.g. @aria-label) attributes
Summary: AX: On generic container elements, WebKit should distinguish between tooltip ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: chris fleizach
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-04-04 15:05 PDT by James Craig
Modified: 2017-09-12 09:31 PDT (History)
11 users (show)

See Also:


Attachments
patch (8.36 KB, patch)
2017-07-18 12:15 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (8.35 KB, patch)
2017-07-18 17:35 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (7.40 KB, patch)
2017-07-26 00:00 PDT, chris fleizach
jdiggs: review+
Details | Formatted Diff | Diff
patch for updated comments (7.41 KB, patch)
2017-09-11 08:45 PDT, chris fleizach
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews100 for mac-elcapitan (1.37 MB, application/zip)
2017-09-11 09:52 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (1.50 MB, application/zip)
2017-09-11 09:57 PDT, Build Bot
no flags Details
patch for updated comments (7.47 KB, patch)
2017-09-11 09:59 PDT, chris fleizach
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews103 for mac-elcapitan (1.59 MB, application/zip)
2017-09-11 10:40 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.71 MB, application/zip)
2017-09-11 10:46 PDT, Build Bot
no flags Details
patch for updated comments (7.51 KB, patch)
2017-09-11 10:49 PDT, chris fleizach
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-elcapitan-wk2 (1.64 MB, application/zip)
2017-09-11 11:40 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews100 for mac-elcapitan (1.50 MB, application/zip)
2017-09-11 11:54 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-elcapitan (2.12 MB, application/zip)
2017-09-11 12:18 PDT, Build Bot
no flags Details
patch for updated comments (7.50 KB, patch)
2017-09-11 13:47 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch for landing (7.50 KB, patch)
2017-09-11 16:48 PDT, chris fleizach
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Craig 2017-04-04 15:05:46 PDT
AX: On generic container elements, WebKit should distinguish between tooltip (e.g. @title) and accessible label (e.g. @aria-label) attributes 

On containers (WebCore GroupRole, etc. not leaf nodes or explicit roles like abbr, fieldset>summary, etc.) the tooltip attributes (e.g. @title) should be exposed as AXHelp not AXTitle. WebKit exposes generic containers <div title="foo"> as indistinguishable in the APIs from containers that use a more explicit label attribute, including ARIA global attributes (<div aria-label="foo">) so AT can't distinguish these in the user context. 

Currently the Accessibility Name Computation (formerly known as ARIA Name Computation) defines explicit rules for using tooltip attributes the role is defined (<main>, <div role="group">, etc.) but does not distinguish when the role is undefined on an semantically meaningless element like <div> with no explicit ARIA role. Therefore I think it will be safe to make this change on GroupRole (<div>), but not make it on other semantic containers like ApplicationGroupRole (<div role="group">)
Comment 1 Radar WebKit Bug Importer 2017-04-04 15:21:57 PDT
<rdar://problem/31439222>
Comment 2 chris fleizach 2017-07-18 12:15:05 PDT
Created attachment 315812 [details]
patch
Comment 3 Build Bot 2017-07-18 12:16:17 PDT
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.
Comment 4 chris fleizach 2017-07-18 17:35:18 PDT
Created attachment 315863 [details]
patch
Comment 5 Joanmarie Diggs (irc: joanie) 2017-07-18 22:54:10 PDT
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.
Comment 6 chris fleizach 2017-07-19 00:11:00 PDT
James, can you comment which roles you think should participate in title attribute nullification?
Comment 7 James Craig 2017-07-25 12:07:26 PDT
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 8 James Craig 2017-07-25 12:12:36 PDT
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.
Comment 9 James Craig 2017-07-25 12:18:54 PDT
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.
Comment 10 chris fleizach 2017-07-26 00:00:52 PDT
Created attachment 316433 [details]
patch
Comment 11 chris fleizach 2017-07-26 09:21:24 PDT
(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 12 Joanmarie Diggs (irc: joanie) 2017-07-27 02:41:56 PDT
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
Comment 13 chris fleizach 2017-09-11 08:45:03 PDT
Created attachment 320429 [details]
patch for updated comments
Comment 14 Build Bot 2017-09-11 09:52:18 PDT
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
Comment 15 Build Bot 2017-09-11 09:52:19 PDT
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 16 Build Bot 2017-09-11 09:57:15 PDT
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
Comment 17 Build Bot 2017-09-11 09:57:16 PDT
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
Comment 18 chris fleizach 2017-09-11 09:59:58 PDT
Created attachment 320437 [details]
patch for updated comments
Comment 19 Build Bot 2017-09-11 10:40:32 PDT
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
Comment 20 Build Bot 2017-09-11 10:40:34 PDT
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 21 Build Bot 2017-09-11 10:46:37 PDT
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
Comment 22 Build Bot 2017-09-11 10:46:43 PDT
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
Comment 23 chris fleizach 2017-09-11 10:49:37 PDT
Created attachment 320444 [details]
patch for updated comments
Comment 24 Build Bot 2017-09-11 11:40:24 PDT
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
Comment 25 Build Bot 2017-09-11 11:40:25 PDT
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 26 Build Bot 2017-09-11 11:54:23 PDT
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
Comment 27 Build Bot 2017-09-11 11:54:24 PDT
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 28 Build Bot 2017-09-11 12:18:42 PDT
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
Comment 29 Build Bot 2017-09-11 12:18:45 PDT
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
Comment 30 chris fleizach 2017-09-11 13:47:44 PDT
Created attachment 320467 [details]
patch for updated comments
Comment 31 chris fleizach 2017-09-11 16:48:55 PDT
Created attachment 320503 [details]
patch for landing
Comment 32 WebKit Commit Bot 2017-09-12 09:31:45 PDT
Comment on attachment 320503 [details]
patch for landing

Clearing flags on attachment: 320503

Committed r221918: <http://trac.webkit.org/changeset/221918>
Comment 33 WebKit Commit Bot 2017-09-12 09:31:47 PDT
All reviewed patches have been landed.  Closing bug.