Bug 73180 - CSS3 currentColor on outline-color gets treated as inherit
Summary: CSS3 currentColor on outline-color gets treated as inherit
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: David Barr
URL: http://zegnat.net/tests/outline-curre...
Keywords:
Depends on:
Blocks: 75523
  Show dependency treegraph
 
Reported: 2011-11-27 18:31 PST by David Barr
Modified: 2012-02-12 21:00 PST (History)
8 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description David Barr 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.
Comment 1 David Barr 2011-11-27 18:31:54 PST
Created attachment 116691 [details]
Test case
Comment 2 David Barr 2011-11-29 16:21:04 PST
Created attachment 117069 [details]
Patch
Comment 3 David Barr 2011-11-29 16:24:41 PST
Created attachment 117070 [details]
Patch
Comment 4 David Barr 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.
Comment 5 David Barr 2011-11-29 16:40:41 PST
Downstream bug report: http://crbug.com/105499
Comment 6 David Barr 2011-11-29 19:37:56 PST
Created attachment 117100 [details]
Patch
Comment 7 WebKit Review Bot 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
Comment 8 David Barr 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.
Comment 9 David Barr 2011-12-01 20:18:02 PST
Created attachment 117551 [details]
Patch
Comment 10 Ryosuke Niwa 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.
Comment 11 David Barr 2011-12-05 20:12:17 PST
Created attachment 117983 [details]
Patch
Comment 12 Ryosuke Niwa 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.
Comment 13 Eric Seidel (no email) 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.
Comment 14 WebKit Review Bot 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
Comment 15 David Barr 2011-12-22 16:26:06 PST
Comment on attachment 117983 [details]
Patch

Maybe the commit queue is happy now?
Comment 16 Daniel Bates 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.
Comment 17 David Barr 2012-01-03 21:16:07 PST
Created attachment 121055 [details]
Patch
Comment 18 Daniel Bates 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.
Comment 19 Daniel Bates 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.
Comment 20 David Barr 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.
Comment 21 David Barr 2012-02-05 16:46:06 PST
Created attachment 125543 [details]
Patch
Comment 22 WebKit Review Bot 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.
Comment 23 WebKit Review Bot 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>
Comment 24 WebKit Review Bot 2012-02-12 21:00:56 PST
All reviewed patches have been landed.  Closing bug.