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
<rdar://problem/14815096>
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
This will *only* apply to pseudo elements (generated content ::before and ::after), not to rendered DOM nodes.
related to bug 122138
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
Created attachment 215281 [details] patch
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.
Created attachment 215282 [details] patch
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
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 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
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
Created attachment 215313 [details] patch
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)
Created attachment 215512 [details] patch patch to move test to Mac only for now
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
(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 "/**/"
(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);
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"…
(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
(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.
(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.
Created attachment 216585 [details] patch
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 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.
Created attachment 216611 [details] patch (with removed code duplication)
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.
http://trac.webkit.org/changeset/159591