Bug 120188 - AX: Implement CSS -webkit-alt property (text alternative for generated content pseudo-elements ::before and ::after)
Summary: AX: Implement CSS -webkit-alt property (text alternative for generated conten...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: chris fleizach
URL:
Keywords: InRadar
Depends on:
Blocks: 123751 123752
  Show dependency treegraph
 
Reported: 2013-08-22 17:59 PDT by James Craig
Modified: 2013-11-20 16:26 PST (History)
23 users (show)

See Also:


Attachments
patch (26.04 KB, patch)
2013-10-27 22:54 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (26.03 KB, patch)
2013-10-27 23:03 PDT, chris fleizach
buildbot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 (446.99 KB, application/zip)
2013-10-28 01:14 PDT, Build Bot
no flags Details
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2 (490.48 KB, application/zip)
2013-10-28 01:55 PDT, Build Bot
no flags Details
patch (26.33 KB, patch)
2013-10-28 09:20 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (26.77 KB, patch)
2013-10-30 09:07 PDT, chris fleizach
no flags Details | Formatted Diff | Diff
patch (27.16 KB, patch)
2013-11-11 10:06 PST, chris fleizach
no flags Details | Formatted Diff | Diff
patch (with removed code duplication) (26.71 KB, patch)
2013-11-11 14:06 PST, chris fleizach
dino: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Craig 2013-08-22 17:59:42 PDT
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
Comment 1 Radar WebKit Bug Importer 2013-08-22 18:00:01 PDT
<rdar://problem/14815096>
Comment 3 James Craig 2013-09-30 21:31:53 PDT
This will *only* apply to pseudo elements (generated content ::before and ::after), not to rendered DOM nodes.
Comment 4 James Craig 2013-09-30 22:01:39 PDT
related to bug 122138
Comment 6 chris fleizach 2013-10-27 22:54:48 PDT
Created attachment 215281 [details]
patch
Comment 7 WebKit Commit Bot 2013-10-27 22:57:06 PDT
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.
Comment 8 chris fleizach 2013-10-27 23:03:59 PDT
Created attachment 215282 [details]
patch
Comment 9 Build Bot 2013-10-28 01:14:02 PDT
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
Comment 10 Build Bot 2013-10-28 01:14:05 PDT
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
Comment 11 Build Bot 2013-10-28 01:55:07 PDT
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
Comment 12 Build Bot 2013-10-28 01:55:11 PDT
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
Comment 13 chris fleizach 2013-10-28 09:20:12 PDT
Created attachment 215313 [details]
patch
Comment 14 Mario Sanchez Prada 2013-10-29 03:29:03 PDT
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)
Comment 15 chris fleizach 2013-10-30 09:07:39 PDT
Created attachment 215512 [details]
patch

patch to move test to Mac only for now
Comment 16 James Craig 2013-11-02 11:35:48 PDT
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
Comment 17 James Craig 2013-11-02 11:38:11 PDT
(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 "/**/"
Comment 18 James Craig 2013-11-02 11:41:54 PDT
(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);
Comment 19 James Craig 2013-11-02 13:11:00 PDT
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"…
Comment 20 chris fleizach 2013-11-02 23:56:27 PDT
(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
Comment 21 James Craig 2013-11-04 12:47:05 PST
(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.
Comment 22 James Craig 2013-11-04 12:50:53 PST
(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.
Comment 23 chris fleizach 2013-11-11 10:06:05 PST
Created attachment 216585 [details]
patch
Comment 24 Tim Horton 2013-11-11 14:00:08 PST
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.
Comment 25 chris fleizach 2013-11-11 14:02:04 PST
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.
Comment 26 chris fleizach 2013-11-11 14:06:17 PST
Created attachment 216611 [details]
patch (with removed code duplication)
Comment 27 Dean Jackson 2013-11-20 14:57:44 PST
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.
Comment 28 chris fleizach 2013-11-20 16:26:05 PST
http://trac.webkit.org/changeset/159591