| 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: | P2 | ||||||||||||||||||||
| Version: | 528+ (Nightly build) | ||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||
| URL: | http://zegnat.net/tests/outline-currentColor.html | ||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||
| Bug Blocks: | 75523 | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
David Barr
2011-11-27 18:31:09 PST
Created attachment 116691 [details]
Test case
Created attachment 117069 [details]
Patch
Created attachment 117070 [details]
Patch
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. Downstream bug report: http://crbug.com/105499 Created attachment 117100 [details]
Patch
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 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.
Created attachment 117551 [details]
Patch
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. Created attachment 117983 [details]
Patch
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 on attachment 117983 [details]
Patch
David is not a committer, so you might as well set r-. :) But this looks landable as-is.
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 on attachment 117983 [details]
Patch
Maybe the commit queue is happy now?
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. Created attachment 121055 [details]
Patch
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. (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. + // 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. Created attachment 125543 [details]
Patch
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 on attachment 125543 [details] Patch Clearing flags on attachment: 125543 Committed r107528: <http://trac.webkit.org/changeset/107528> All reviewed patches have been landed. Closing bug. |