Bug 170475 – AX: On generic container elements, WebKit should distinguish between tooltip (e.g. @title) and label (e.g. @aria-label) attributes

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[James Craig]

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

[Radar WebKit Bug Importer]

<rdar://problem/31439222>

[chris fleizach]

Created attachment 315812 [details]
patch

[Build Bot]

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.

[chris fleizach]

Created attachment 315863 [details]
patch

[Joanmarie Diggs]

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.

[chris fleizach]

James, can you comment which roles you think should participate in title attribute nullification?

[James Craig]

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.

[James Craig]

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.

[James Craig]

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.

[chris fleizach]

Created attachment 316433 [details]
patch

[chris fleizach]

(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

[Joanmarie Diggs]

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

[chris fleizach]

Created attachment 320429 [details]
patch for updated comments

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[chris fleizach]

Created attachment 320437 [details]
patch for updated comments

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[chris fleizach]

Created attachment 320444 [details]
patch for updated comments

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[chris fleizach]

Created attachment 320467 [details]
patch for updated comments

[chris fleizach]

Created attachment 320503 [details]
patch for landing

[WebKit Commit Bot]

Comment on attachment 320503 [details]
patch for landing

Clearing flags on attachment: 320503

Committed r221918: <http://trac.webkit.org/changeset/221918>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.