Bug 120188 – AX: Implement CSS -webkit-alt property (text alternative for generated content pseudo-elements ::before and ::after)

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

[James Craig]

AX: Implement CSS -webkit-alt property

Not in a spec yet, but discussion was positive and the issue is being tracked by the CSS WG. Since this is holding up several projects, I propose implementing the vendor-prefixed form: -webkit-alt.

http://lists.w3.org/Archives/Public/www-style/2012Nov/0319.html

[Radar WebKit Bug Importer]

<rdar://problem/14815096>

[James Craig]

Start of thread:
http://lists.w3.org/Archives/Public/www-style/2012Nov/thread.html#msg233

Tab's suggestion of "alt" property:
http://lists.w3.org/Archives/Public/www-style/2012Nov/0317.html

Clarifications:
http://lists.w3.org/Archives/Public/www-style/2012Nov/0318.html

[James Craig]

This will *only* apply to pseudo elements (generated content ::before and ::after), not to rendered DOM nodes.

[James Craig]

related to bug 122138

[James Craig]

webkit-dev discussion:
https://lists.webkit.org/pipermail/webkit-dev/2013-September/025551.html

thread:
https://lists.webkit.org/pipermail/webkit-dev/2013-October/thread.html#25581

[chris fleizach]

Created attachment 215281 [details]
patch

[WebKit Commit Bot]

Attachment 215281 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/accessibility/webkit-alt-for-css-content-expected.txt', u'LayoutTests/accessibility/webkit-alt-for-css-content.html', u'Source/WebCore/ChangeLog', u'Source/WebCore/accessibility/AccessibilityNodeObject.cpp', u'Source/WebCore/accessibility/AccessibilityRenderObject.cpp', u'Source/WebCore/css/CSSComputedStyleDeclaration.cpp', u'Source/WebCore/css/CSSParser.cpp', u'Source/WebCore/css/CSSParser.h', u'Source/WebCore/css/CSSPropertyNames.in', u'Source/WebCore/css/StyleResolver.cpp', u'Source/WebCore/rendering/RenderImage.h', u'Source/WebCore/rendering/RenderTextFragment.h', u'Source/WebCore/rendering/style/ContentData.cpp', u'Source/WebCore/rendering/style/ContentData.h', u'Source/WebCore/rendering/style/RenderStyle.cpp', u'Source/WebCore/rendering/style/RenderStyle.h', u'Source/WebCore/rendering/style/StyleRareNonInheritedData.h']" exit_code: 1
Source/WebCore/css/StyleResolver.cpp:2248:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1199:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1278:  An else if statement should be written as an if statement when the prior "if" concludes with a return, break, continue or goto statement.  [readability/control_flow] [4]
Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1285:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Source/WebCore/css/CSSParser.cpp:1954:  One space before end of line comments  [whitespace/comments] [5]
Source/WebCore/css/CSSParser.cpp:1955:  When wrapping a line, only indent 4 spaces.  [whitespace/indent] [3]
Total errors found: 6 in 18 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Created attachment 215282 [details]
patch

[Build Bot]

Comment on attachment 215282 [details]
patch

Attachment 215282 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/15508049

New failing tests:
accessibility/webkit-alt-for-css-content.html

[Build Bot]

Created attachment 215285 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 215282 [details]
patch

Attachment 215282 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/11168111

New failing tests:
accessibility/webkit-alt-for-css-content.html

[Build Bot]

Created attachment 215289 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[chris fleizach]

Created attachment 215313 [details]
patch

[Mario Sanchez Prada]

Comment on attachment 215313 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=215313&action=review

Besides the location of the test, I'm happy with the a11y changes. Somebody should review the other parts, though

> LayoutTests/accessibility/webkit-alt-for-css-content.html:82
> +        outputElement(accessibilityController.accessibleElementById("test1").childAtIndex(1));

Bits like this one are going to fail in GTK/EFL because we do not expose StaticText objects individually (we merge them into their parents), so for instance in this case for those platforms we should use childAtIndex(0). In cases where  childAtIndex(0) is used (below), we should just avoid that and check the div directly.

So, I think it would be better to move this to platform/mac for now, and later we could either make a GTK/EFL specific version or just use testRunner.platformName to make it variable (and I can do that)

[chris fleizach]

Created attachment 215512 [details]
patch

patch to move test to Mac only for now

[James Craig]

Comment on attachment 215512 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=215512&action=review

> Source/WebCore/css/CSSParser.cpp:1954
> +    case CSSPropertyWebkitAlt: // [ <string> | attr(X) ]

I think this should include <counter>, too.

> Source/WebCore/css/CSSParser.cpp:3794
> +bool CSSParser::parseAlt(CSSPropertyID propID, bool important)

It doesn't look like this is handling concatenated values, which should be added to the test case, too.

See: CSSParser::parseContent

<div foo="foo" baz="baz" bif="bif" bop="bop" style="content: attr(foo) "bar" attr(baz); -webkit-alt: attr(bif) "bap" attr(bop); ">contents</div>

Display renders "foo bar baz".
AX API gets "bif bap bop".

> LayoutTests/platform/mac/accessibility/webkit-alt-for-css-content.html:20
> +    -webkit-alt: "ALTERNATIVE CONTENT TEST2";

I know the string content fallback is a different bug, but this test should include a check to make sure it does not regress -webkit-alt once it's fixed.

content: url(resources/cake.png), "foo";
-webkit-alt: "bar, not foo";

content: url(resources/cake.png), "";
-webkit-alt: "not empty";

content: url(resources/cake.png), "foo";
-webkit-alt: ""; // should be empty, not foo

[James Craig]

(In reply to comment #16)
> content: url(resources/cake.png), "foo";
> -webkit-alt: ""; // should be empty, not foo

Just to clarify, the line comment is a mistake. 
"//" does not work in CSS, only "/**/"

[James Craig]

(In reply to comment #16)
> <div foo="foo" baz="baz" bif="bif" bop="bop" style="content: attr(foo) "bar" attr(baz); -webkit-alt: attr(bif) "bap" attr(bop); ">contents</div>
> 
> Display renders "foo bar baz".
> AX API gets "bif bap bop".

Actually, that'd probably render "foobarbaz" and "bifbapbop".

I think you have to add the spaces in the string in order to get the original values.
content: attr(foo) " bar " attr(baz); -webkit-alt: attr(bif) " bap " attr(bop);

[James Craig]

Also, the tests should verify that this only applies to pseudo elements:

<img src="#" alt="foo" style="-webkit-alt: 'ignore this';">

Image alternative text should be "foo" not "ignore this"…

[chris fleizach]

(In reply to comment #16)
> (From update of attachment 215512 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=215512&action=review
> 
> > Source/WebCore/css/CSSParser.cpp:1954
> > +    case CSSPropertyWebkitAlt: // [ <string> | attr(X) ]
> 
> I think this should include <counter>, too.

It's not clear how this would apply to counters. Certainly we shouldn't do it in this patch

> 
> > Source/WebCore/css/CSSParser.cpp:3794
> > +bool CSSParser::parseAlt(CSSPropertyID propID, bool important)
> 
> It doesn't look like this is handling concatenated values, which should be added to the test case, too.
> 

Please file a new bug. 

> See: CSSParser::parseContent
> 
> <div foo="foo" baz="baz" bif="bif" bop="bop" style="content: attr(foo) "bar" attr(baz); -webkit-alt: attr(bif) "bap" attr(bop); ">contents</div>
> 
> Display renders "foo bar baz".
> AX API gets "bif bap bop".
> 
> > LayoutTests/platform/mac/accessibility/webkit-alt-for-css-content.html:20
> > +    -webkit-alt: "ALTERNATIVE CONTENT TEST2";
> 
> I know the string content fallback is a different bug, but this test should include a check to make sure it does not regress -webkit-alt once it's fixed.

I don't think it's necessary to have a failing test case. When the other issue is resolved, new test cases can be added

> 
> content: url(resources/cake.png), "foo";
> -webkit-alt: "bar, not foo";
> 
> content: url(resources/cake.png), "";
> -webkit-alt: "not empty";
> 
> content: url(resources/cake.png), "foo";
> -webkit-alt: ""; // should be empty, not foo

[James Craig]

(In reply to comment #20)
> (In reply to comment #16)
> > (From update of attachment 215512 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=215512&action=review
> > 
> > > Source/WebCore/css/CSSParser.cpp:1954
> > > +    case CSSPropertyWebkitAlt: // [ <string> | attr(X) ]
> > 
> > I think this should include <counter>, too.
> 
> It's not clear how this would apply to counters. Certainly we shouldn't do it in this patch.

Filed bug 123752.

> > > Source/WebCore/css/CSSParser.cpp:3794
> > > +bool CSSParser::parseAlt(CSSPropertyID propID, bool important)
> > 
> > It doesn't look like this is handling concatenated values, which should be added to the test case, too.
> > 
> > See: CSSParser::parseContent
> 
> Please file a new bug. 

Filed bug 123751.

FWIW you should still update the patch to test that this only applies to pseudo elements (see comment #19) because it would be detrimental to allow -webkit-alt on general elements for the same reason that 'content' is not allowed on general elements.

[James Craig]

(In reply to comment #16)

> I know the string content fallback is a different bug, but this test should include a check to make sure it does not regress -webkit-alt once it's fixed.
> 
> content: url(resources/cake.png), "foo";
> -webkit-alt: "bar, not foo";
> 
> content: url(resources/cake.png), "";
> -webkit-alt: "not empty";
> 
> content: url(resources/cake.png), "foo";
> -webkit-alt: ""; // should be empty, not foo

Moved that comment to bug 122138.

[chris fleizach]

Created attachment 216585 [details]
patch

[Tim Horton]

Comment on attachment 216585 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=216585&action=review

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1215
> +        // The alt attribute may be set on a text fragment through CSS, which should be honored.

Is there something different between these two blocks, or is it just duplicated verbatim? I might be going blind.

[chris fleizach]

Comment on attachment 216585 [details]
patch

View in context: https://bugs.webkit.org/attachment.cgi?id=216585&action=review

>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1215
>> +        // The alt attribute may be set on a text fragment through CSS, which should be honored.
> 
> Is there something different between these two blocks, or is it just duplicated verbatim? I might be going blind.

ugh, patch got applied twice.

[chris fleizach]

Created attachment 216611 [details]
patch (with removed code duplication)

[Dean Jackson]

Comment on attachment 216611 [details]
patch (with removed code duplication)

View in context: https://bugs.webkit.org/attachment.cgi?id=216611&action=review

Overall I wonder if altText should be alternateText, but I guess the attribute name is well know in HTML.

> Source/WebCore/ChangeLog:11
> +        it sets that string on the TextFragment or RenderImage, which can be queiered by accessibility code.

typo: queried

> Source/WebCore/css/CSSParser.cpp:3804
> +        // attr(X)

I don't think we need this line.

> Source/WebCore/css/StyleResolver.cpp:2272
> +                // register the fact that the attribute value affects the style

Nit: Uppercase R and end with .

> LayoutTests/platform/mac/accessibility/webkit-alt-for-css-content.html:17
> +/* webkit-alt on image content that string. */

Typo: is a string

> LayoutTests/platform/mac/accessibility/webkit-alt-for-css-content.html:86
> +        debug("Test2 - webkit-alt on image content that string");

Typo here too.

[chris fleizach]

http://trac.webkit.org/changeset/159591