Bug 73180

Summary:

CSS3 currentColor on outline-color gets treated as inherit

Product:

WebKit

Reporter:

David Barr <davidbarr>

Component:

CSS

Assignee:

David Barr <davidbarr>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dbates, dglazkov, jchaffraix, macpherson, menard, rniwa, simon.fraser, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

URL:

http://zegnat.net/tests/outline-currentColor.html

Bug Depends on:

Bug Blocks:

75523

Attachments:

Description	Flags
Test case	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

David Barr

Reported 2011-11-27 18:31:09 PST

Chrome Version : 17.0.949.0 (Official Build 111462) canary URLs : http://zegnat.net/tests/outline-currentColor.html Other browsers tested: Safari 5: FAIL Firefox 8: OK Opera 12: FAIL What steps will reproduce the problem? 1. Specify outline-color: currentColor; on any HTML element who's parent element has an outline-color specified. What is the expected result? The element’s outline should be given the same color as the element’s text. What happens instead? The element’s outline inherits the color of the parent’s outline.

Attachments
Test case (3.26 KB, text/html) 2011-11-27 18:31 PST, David Barr	no flags	Details
Patch (4.79 KB, patch) 2011-11-29 16:21 PST, David Barr	no flags	Details Formatted Diff Diff
Patch (5.41 KB, patch) 2011-11-29 16:24 PST, David Barr	no flags	Details Formatted Diff Diff
Patch (4.80 KB, patch) 2011-11-29 19:37 PST, David Barr	no flags	Details Formatted Diff Diff
Patch (4.84 KB, patch) 2011-12-01 20:18 PST, David Barr	no flags	Details Formatted Diff Diff
Patch (5.00 KB, patch) 2011-12-05 20:12 PST, David Barr	no flags	Details Formatted Diff Diff
Patch (5.11 KB, patch) 2012-01-03 21:16 PST, David Barr	no flags	Details Formatted Diff Diff
Patch (5.52 KB, patch) 2012-02-05 16:46 PST, David Barr	no flags	Details Formatted Diff Diff
Show Obsolete (6) View All Add attachment proposed patch, testcase, etc.

David Barr

Comment 1 2011-11-27 18:31:54 PST

Created attachment 116691 [details] Test case

David Barr

Comment 2 2011-11-29 16:21:04 PST

Created attachment 117069 [details] Patch

David Barr

Comment 3 2011-11-29 16:24:41 PST

Created attachment 117070 [details] Patch

David Barr

Comment 4 2011-11-29 16:38:25 PST

The CSS2 and CSS3 UI modules state that outline-color is not inherited. Inspection of CSSStyleApplyProperty.cpp reveals that it is currently applied as inherit.

David Barr

Comment 5 2011-11-29 16:40:41 PST

Downstream bug report: http://crbug.com/105499

David Barr

Comment 6 2011-11-29 19:37:56 PST

Created attachment 117100 [details] Patch

WebKit Review Bot

Comment 7 2011-11-30 06:12:24 PST

Comment on attachment 117100 [details] Patch Attachment 117100 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/10688060 New failing tests: svg/transforms/text-with-pattern-with-svg-transform.svg

David Barr

Comment 8 2011-11-30 13:04:16 PST

Comment on attachment 117100 [details] Patch This test failure is unrelated to the change, it makes no use of outline and changes don't come much more narrow.

David Barr

Comment 9 2011-12-01 20:18:02 PST

Created attachment 117551 [details] Patch

Ryosuke Niwa

Comment 10 2011-12-05 19:45:56 PST

Comment on attachment 117551 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117551&action=review > Source/WebCore/ChangeLog:8 > + The CSS2 and CSS3 UI modules state that outline-color is not inherited. Make it so. You should have a hyperlink to a relevant spec. > LayoutTests/ChangeLog:7 > + You should describe what kind of a test you're adding. > LayoutTests/fast/css/outline-currentcolor.html:1 > +<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.0//EN"> Not <!DOCTYPE html> ? > LayoutTests/fast/css/outline-currentcolor.html:2 > +<html lang="en"> Do we really need that? > LayoutTests/fast/css/outline-currentcolor.html:29 > + if (window.getComputedStyle(div).outlineColor != "rgb(0, 128, 0)") { > + log("FAILED"); > + return; > + } > + div = document.getElementById("three"); > + if (window.getComputedStyle(div).color != "rgb(0, 128, 0)") { > + log("FAILED"); > + return; > + } > + log("PASSED"); Seems like we should convert this to a script test? > LayoutTests/fast/css/outline-currentcolor.html:33 > + <body onload="runTest()"> Do we really need to wait until onload? Can't we just place script after #console? > LayoutTests/fast/css/outline-currentcolor.html:40 > + <div id="one" style="color:green; outline: solid 1em currentColor" ></div> > + <div id="two" style="color:red; outline: solid 1em currentColor" ></div> > + <div style="color:green"> > + <div id="three" style="color:currentColor; outline: solid 1em currentColor" ></div> > + </div> > + <div id="console"></div> > + </body> Seems unnecessary to indent these elements.

David Barr

Comment 11 2011-12-05 20:12:17 PST

Created attachment 117983 [details] Patch

Ryosuke Niwa

Comment 12 2011-12-05 21:53:46 PST

Comment on attachment 117983 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=117983&action=review Wait, your test should describe what you're testing. cq-. > LayoutTests/fast/css/outline-currentcolor.html:38 > + <div id="one" style="color:green; outline: solid 1em currentColor" ></div> > + <div id="two" style="color:red; outline: solid 1em currentColor" ></div> > + <div style="color:green"> > + <div id="three" style="color:currentColor; outline: solid 1em currentColor" ></div> > + </div> Please add some description as to what you're adding.

Eric Seidel (no email)

Comment 13 2011-12-21 15:09:03 PST

Comment on attachment 117983 [details] Patch David is not a committer, so you might as well set r-. :) But this looks landable as-is.

WebKit Review Bot

Comment 14 2011-12-21 19:37:40 PST

Comment on attachment 117983 [details] Patch Rejecting attachment 117983 [details] from commit-queue. Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=queues.webkit.org', '-..." exit_code: 2 Last 500 characters of output: hromium/testing/gmock --revision 374 --non-interactive --force --accept theirs-conflict --ignore-externals returned non-zero exit status 1 in /mnt/git/webkit-commit-queue/Source/WebKit/chromium Error: 'depot_tools/gclient sync --force --reset --delete_unversioned_trees' failed 3 tries and returned 256 at Tools/Scripts/update-webkit-chromium line 106. Re-trying 'depot_tools/gclient sync --force --reset --delete_unversioned_trees' No such file or directory at Tools/Scripts/update-webkit line 112. Full output: http://queues.webkit.org/results/10981277

David Barr

Comment 15 2011-12-22 16:26:06 PST

Comment on attachment 117983 [details] Patch Maybe the commit queue is happy now?

Daniel Bates

Comment 16 2012-01-03 20:31:40 PST

Comment on attachment 117983 [details] Patch David Barr mentioned on IRC today that this test case is a "mirror of [<http://trac.webkit.org/browser/trunk/LayoutTests/fast/css/background-currentcolor.html>]." We shouldn't duplicate code. At the very least we should add a description to this test case (as mentioned by Ryosuke in comment #12). One step further, we should extract the common code between these test. Event further, we should abstract runTest() such that it takes as an argument the CSS property to test. Even better, we should make use of the functionality in <http://trac.webkit.org/browser/trunk/LayoutTests/fast/js/resources/js-test-pre.js> instead of rolling our own analogous testing support, such as the log() function. We may also benefit from more descriptive PASS/FAIL message by using the testing functionality in js-test-pre.js. We can do these tasks in separate bugs.

David Barr

Comment 17 2012-01-03 21:16:07 PST

Created attachment 121055 [details] Patch

Daniel Bates

Comment 18 2012-01-03 21:50:52 PST

Comment on attachment 121055 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=121055&action=review The test case looks better. Thanks for abstracting the CSS property logic in runTest() (*). That being said, putting the test description in the HTML Title element is insufficient. Please put a test description in the HTML markup to make it straight forward to both understand what the test when testing by hand as well as to help someone interpret the meaning of the message "PASSED" in the expected results file, outline-currentcolor-expected.txt. One example of putting a description in the HTML markup is <http://trac.webkit.org/browser/trunk/LayoutTests/fast/css/text-align-webkit-match-parent-parse.html>. Also, we should extract the inline JavaScript script in this file into an external JavaScript file, say LayoutTests/fast/css/resources/computeCSSColorProperty.js, and then reference this external script in both this file and <http://trac.webkit.org/browser/trunk/LayoutTests/fast/css/background-currentcolor.html>. > LayoutTests/fast/css/outline-currentcolor.html:11 > + // FIXME: Re-write this and background-currentcolor.html to use js-test-pre.js This is fine as-is. That being said, I suggest we file a bug for this and reference the bug number in this FIXME so as to make this FIXME actionable.

Daniel Bates

Comment 19 2012-01-03 21:52:24 PST

(In reply to comment #18) > (From update of attachment 121055 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=121055&action=review > > The test case looks better. Thanks for abstracting the CSS property logic in runTest() (*). Disregard the (*). I incorporated the (*) remark into the second paragraph of my response in comment 18.

David Barr

Comment 20 2012-01-03 21:56:18 PST

+ // FIXME: Re-write this and background-currentcolor.html to use js-test-pre.js > > This is fine as-is. That being said, I suggest we file a bug for this and reference the bug number in this FIXME so as to make this FIXME actionable. https://bugs.webkit.org/show_bug.cgi?id=75523 Will add to the FIXME in the re-roll.

David Barr

Comment 21 2012-02-05 16:46:06 PST

Created attachment 125543 [details] Patch

WebKit Review Bot

Comment 22 2012-02-12 19:56:03 PST

Comment on attachment 125543 [details] Patch Rejecting attachment 125543 [details] from commit-queue. davidbarr@chromium.org does not have committer permissions according to http://trac.webkit.org/browser/trunk/Tools/Scripts/webkitpy/common/config/committers.py. - If you do not have committer rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags. - If you have committer rights please correct the error in Tools/Scripts/webkitpy/common/config/committers.py by adding yourself to the file (no review needed). The commit-queue restarts itself every 2 hours. After restart the commit-queue will correctly respect your committer rights.

WebKit Review Bot

Comment 23 2012-02-12 21:00:49 PST

Comment on attachment 125543 [details] Patch Clearing flags on attachment: 125543 Committed r107528: <http://trac.webkit.org/changeset/107528>

WebKit Review Bot

Comment 24 2012-02-12 21:00:56 PST

All reviewed patches have been landed. Closing bug.

Note You need to log in before you can comment on or make changes to this bug.