Bug 58608 - Groove/inset/outset borders show solid if the color is black
Summary: Groove/inset/outset borders show solid if the color is black
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All OS X 10.5
: P2 Normal
Assignee: Nobody
URL: http://smfr.org/misc/tables.html
Keywords:
Depends on:
Blocks: 139190
  Show dependency treegraph
 
Reported: 2011-04-14 17:09 PDT by Simon Fraser (smfr)
Modified: 2014-12-09 15:32 PST (History)
12 users (show)

See Also:


Attachments
Testcase (1.83 KB, text/html)
2011-04-14 17:09 PDT, Simon Fraser (smfr)
no flags Details
IE9 rendering for the test in the URL (20.97 KB, image/png)
2011-04-25 10:05 PDT, Simon Fraser (smfr)
no flags Details
Firefox 4 rendering (19.42 KB, image/png)
2011-04-25 10:39 PDT, Simon Fraser (smfr)
no flags Details
Patch. (7.67 KB, patch)
2011-09-19 00:05 PDT, Antaryami Pandia
simon.fraser: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (1.34 MB, patch)
2011-09-19 23:42 PDT, Antaryami Pandia
simon.fraser: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Patch (25.43 KB, patch)
2011-09-20 22:07 PDT, Antaryami Pandia
no flags Details | Formatted Diff | Diff
Patch (28.03 KB, patch)
2011-09-21 22:19 PDT, Antaryami Pandia
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-03 (2.53 MB, application/zip)
2011-09-21 23:34 PDT, WebKit Review Bot
no flags Details
firefox-behavior (22.34 KB, image/jpeg)
2011-10-04 23:54 PDT, Antaryami Pandia
no flags Details
opera-behavior (14.90 KB, image/png)
2011-10-04 23:55 PDT, Antaryami Pandia
no flags Details
IE7-behavior (13.92 KB, image/png)
2011-10-04 23:55 PDT, Antaryami Pandia
no flags Details
Patch for getting Simon's feedback. (2.26 KB, patch)
2011-10-05 00:20 PDT, Antaryami Pandia
no flags Details | Formatted Diff | Diff
Updated patch (26.62 KB, patch)
2011-10-11 23:14 PDT, Antaryami Pandia
eric: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
patch with changelog (29.22 KB, patch)
2011-12-22 02:50 PST, Antaryami Pandia
simon.fraser: review-
webkit.review.bot: commit-queue-
Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-01 (2.44 MB, application/zip)
2011-12-22 03:22 PST, WebKit Review Bot
no flags Details
Vanilla test results (OSX 10.9.5): fast/css fast/table fast/borders (1.10 KB, text/plain)
2014-11-25 14:00 PST, Adenilson Cavalcanti Silva
no flags Details
Patched test results (OSX 10.9.5): fast/css fast/table fast/borders (1.10 KB, text/plain)
2014-11-25 14:01 PST, Adenilson Cavalcanti Silva
no flags Details
Updated patch (6.53 KB, patch)
2014-11-25 14:13 PST, Adenilson Cavalcanti Silva
no flags Details | Formatted Diff | Diff
Adding the pixel test output. (8.95 KB, patch)
2014-11-25 14:39 PST, Adenilson Cavalcanti Silva
no flags Details | Formatted Diff | Diff
Vanilla tests: fast/css fast/table fast/borders (1.10 KB, text/plain)
2014-11-25 17:18 PST, Adenilson Cavalcanti Silva
no flags Details
Patched: fast/css fast/table fast/borders (855 bytes, text/plain)
2014-11-25 17:24 PST, Adenilson Cavalcanti Silva
no flags Details
Rewrote function, added rebaseline in TestExpectations (9.62 KB, patch)
2014-11-30 17:27 PST, Adenilson Cavalcanti Silva
simon.fraser: review+
simon.fraser: commit-queue-
Details | Formatted Diff | Diff
css1 tests output (691 bytes, text/plain)
2014-12-01 12:20 PST, Adenilson Cavalcanti Silva
no flags Details
Addressing reviewer's comments. (9.53 KB, patch)
2014-12-01 13:31 PST, Adenilson Cavalcanti Silva
no flags Details | Formatted Diff | Diff
Addressing reviewer's comments, updated test baseline. (9.28 KB, patch)
2014-12-01 13:39 PST, Adenilson Cavalcanti Silva
no flags Details | Formatted Diff | Diff
Patched: Tables layout tests (731 bytes, text/plain)
2014-12-01 16:00 PST, Adenilson Cavalcanti Silva
no flags Details
Updated + fixes (8.44 KB, patch)
2014-12-02 15:29 PST, Adenilson Cavalcanti Silva
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2011-04-14 17:09:01 PDT
WebKit doesn't lighten the colors when drawing groove/inset/outset borders, so if the color is black (even transparent black), the border appears solid. This differs from Firefox.
Comment 1 Simon Fraser (smfr) 2011-04-14 17:09:18 PDT
Created attachment 89689 [details]
Testcase
Comment 2 Simon Fraser (smfr) 2011-04-25 10:05:02 PDT
Created attachment 90923 [details]
IE9 rendering for the test in the URL
Comment 3 Simon Fraser (smfr) 2011-04-25 10:39:30 PDT
Created attachment 90928 [details]
Firefox 4 rendering
Comment 4 Antaryami Pandia 2011-09-19 00:05:25 PDT
Created attachment 107810 [details]
Patch.
Comment 5 WebKit Review Bot 2011-09-19 00:29:33 PDT
Comment on attachment 107810 [details]
Patch.

Attachment 107810 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9733978

New failing tests:
css1/basic/comments.html
css1/basic/grouping.html
css1/box_properties/border_bottom_width.html
css1/box_properties/border_bottom_width_inline.html
css1/box_properties/border_left_width.html
css1/basic/inheritance.html
css1/box_properties/border_color_inline.html
css1/box_properties/border_inline.html
css1/basic/class_as_selector.html
css1/box_properties/border.html
css1/box_properties/border_right.html
css1/box_properties/border_color.html
css1/box_properties/border_left_width_inline.html
css1/basic/id_as_selector.html
css1/box_properties/border_left.html
css1/box_properties/border_bottom_inline.html
css1/basic/containment.html
css1/box_properties/border_bottom.html
css1/box_properties/border_left_inline.html
css1/basic/contextual_selectors.html
Comment 6 Simon Fraser (smfr) 2011-09-19 10:55:24 PDT
Comment on attachment 107810 [details]
Patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=107810&action=review

The patch looks OK but you need to include pixel results.

> LayoutTests/fast/borders/border-Outset.html:9
> +	<style type="text/css">
> +		div {
> +			width: 200px;
> +			height: 200px;
> +			border: 20px outset #000;
> +			background-color: #0f0;
> +		}

Weird indentation here. Does it contain tabs? Please use 'black', 'green' rather than hex values.

> LayoutTests/fast/borders/border-Outset.html:14
> +		&nbsp;

No need for that.
Comment 7 Antaryami Pandia 2011-09-19 23:42:53 PDT
Created attachment 107970 [details]
Patch

Updated patch as per review comments.
Comment 8 WebKit Review Bot 2011-09-20 02:00:31 PDT
Comment on attachment 107970 [details]
Patch

Attachment 107970 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9767174

New failing tests:
css1/basic/comments.html
css1/basic/grouping.html
css1/box_properties/border_bottom_width.html
css1/box_properties/border_bottom_width_inline.html
css1/box_properties/border_left_width.html
css1/basic/inheritance.html
css1/box_properties/border_color_inline.html
css1/box_properties/border_inline.html
css1/basic/class_as_selector.html
css1/box_properties/border.html
css1/box_properties/border_right.html
css1/box_properties/border_color.html
css1/box_properties/border_left_width_inline.html
css1/basic/id_as_selector.html
css1/box_properties/border_left.html
css1/box_properties/border_bottom_inline.html
css1/basic/containment.html
css1/box_properties/border_bottom.html
css1/box_properties/border_left_inline.html
css1/basic/contextual_selectors.html
Comment 9 Antaryami Pandia 2011-09-20 05:01:57 PDT
Comment on attachment 107970 [details]
Patch

It's still failing for cr-linux.Will check and upload a modified patch
Comment 10 Antaryami Pandia 2011-09-20 05:07:54 PDT
(In reply to comment #6)
> (From update of attachment 107810 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=107810&action=review
> 
> The patch looks OK but you need to include pixel results.

To generate pixel result should I use "run-webkit-tests --pixel" or the "new-run-webkit-tests --pixel".

> Weird indentation here. Does it contain tabs? Please use 'black', 'green' rather than hex values.
> 
Ok.will remove the hex values and the tabs.

> > LayoutTests/fast/borders/border-Outset.html:14
> > +		&nbsp;
> 
> No need for that.

Ok.
Comment 11 Simon Fraser (smfr) 2011-09-20 10:02:55 PDT
Comment on attachment 107970 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=107970&action=review

> Source/WebCore/ChangeLog:11
> +        Tests: fast/borders/border-Groove.html
> +               fast/borders/border-Inset.html
> +               fast/borders/border-Outset.html
> +               fast/borders/border-Ridge.html

Where are the pixel results for these tests?

Also, please keep the filenames all lowercase.
Comment 12 Antaryami Pandia 2011-09-20 22:07:11 PDT
Created attachment 108107 [details]
Patch

Modified patch.
Comment 13 WebKit Review Bot 2011-09-21 00:21:04 PDT
Comment on attachment 108107 [details]
Patch

Attachment 108107 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9773571

New failing tests:
css1/basic/comments.html
css1/basic/grouping.html
css1/box_properties/border_bottom_width.html
css1/box_properties/border_bottom_width_inline.html
css1/box_properties/border_left_width.html
css1/basic/inheritance.html
css1/box_properties/border_color_inline.html
css1/box_properties/border_inline.html
css1/basic/class_as_selector.html
css1/box_properties/border.html
css1/box_properties/border_right.html
css1/box_properties/border_color.html
css1/box_properties/border_left_width_inline.html
css1/basic/id_as_selector.html
css1/box_properties/border_left.html
css1/box_properties/border_bottom_inline.html
css1/basic/containment.html
css1/box_properties/border_bottom.html
css1/box_properties/border_left_inline.html
css1/basic/contextual_selectors.html
Comment 14 Simon Fraser (smfr) 2011-09-21 09:13:08 PDT
Comment on attachment 108107 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=108107&action=review

> LayoutTests/fast/borders/border-groove.html:7
> +            border: 20px groove black;

You might as well make the border 100px here.

> LayoutTests/fast/borders/border-inset.html:7
> +            border: 20px inset black;

Ditto.

> LayoutTests/fast/borders/border-ridge.html:7
> +            border: 20px ridge black;

Ditto.
Comment 15 Antaryami Pandia 2011-09-21 22:19:15 PDT
Created attachment 108276 [details]
Patch

Updated patch.
Comment 16 WebKit Review Bot 2011-09-21 23:34:44 PDT
Comment on attachment 108276 [details]
Patch

Attachment 108276 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/9798069

New failing tests:
css1/basic/comments.html
css1/basic/grouping.html
css1/box_properties/border_bottom_width.html
css1/box_properties/border_bottom_width_inline.html
css1/box_properties/border_left_width.html
css1/basic/inheritance.html
css1/box_properties/border_color_inline.html
css1/box_properties/border_inline.html
css1/basic/class_as_selector.html
css1/box_properties/border.html
css1/box_properties/border_right.html
css1/box_properties/border_color.html
css1/box_properties/border_left_width_inline.html
css1/basic/id_as_selector.html
css1/box_properties/border_left.html
css1/box_properties/border_bottom_inline.html
css1/basic/containment.html
css1/box_properties/border_bottom.html
css1/box_properties/border_left_inline.html
css1/basic/contextual_selectors.html
Comment 17 WebKit Review Bot 2011-09-21 23:34:52 PDT
Created attachment 108281 [details]
Archive of layout-test-results from ec2-cr-linux-03

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-03  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 18 Antaryami Pandia 2011-09-25 21:35:03 PDT
Hi Simon.
Please help committing the patch.

BR,
-Antaryami
Comment 19 Simon Fraser (smfr) 2011-09-26 10:33:07 PDT
Comment on attachment 108276 [details]
Patch

Clearing flags on attachment: 108276

Committed r95960: <http://trac.webkit.org/changeset/95960>
Comment 20 Simon Fraser (smfr) 2011-09-26 10:33:14 PDT
All reviewed patches have been landed.  Closing bug.
Comment 21 Mihai Parparita 2011-09-26 12:36:48 PDT
Was this supposed to change how table border rendering is done? For example:

Expected result: http://svn.webkit.org/repository/webkit/trunk/LayoutTests/platform/mac/css1/box_properties/border_bottom_width_inline-expected.png
New actual result after this patch: http://build.chromium.org/f/chromium/layout_test_results/Webkit_Mac10_6/results/layout-test-results/css1/box_properties/border_bottom_width_inline-actual.png

This is going to need rebaselining of >500 pixel tests, I'd rather not do it if the change wasn't intentional.
Comment 22 Mihai Parparita 2011-09-26 12:42:58 PDT
Reverted r95960 for reason:

Significantly changes table border rendering

Committed r95974: <http://trac.webkit.org/changeset/95974>
Comment 23 Mihai Parparita 2011-09-26 12:44:44 PDT
I reverted after discussing with Simon in #webkit.
Comment 24 Simon Fraser (smfr) 2011-09-26 12:45:31 PDT
Antaryami: I think we need to tweak the patch. Rather than lightening one side and darkening the other (which leaves the two colors two steps apart), I think we should just darken one for light colors, and lighten the other for dark colors. Check Firefox, IE and Opera to see what they do.
Comment 25 Antaryami Pandia 2011-09-27 01:57:15 PDT
Thanks Simon.

(In reply to comment #24)
> Antaryami: I think we need to tweak the patch. Rather than lightening one side and darkening the other (which leaves the two colors two steps apart), I think we should just darken one for light colors, and lighten the other for dark colors. Check Firefox, IE and Opera to see what they do.

Yes.I will make necessary changes.
Comment 26 Antaryami Pandia 2011-10-04 23:54:23 PDT
Created attachment 109747 [details]
firefox-behavior
Comment 27 Antaryami Pandia 2011-10-04 23:55:03 PDT
Created attachment 109748 [details]
opera-behavior
Comment 28 Antaryami Pandia 2011-10-04 23:55:39 PDT
Created attachment 109749 [details]
IE7-behavior
Comment 29 Antaryami Pandia 2011-10-05 00:03:22 PDT
Added the snapshots for respective browser behavior with border styles as :-

1. border: 20px solid red;
and
2. border: 20px inset red;

I used the border color with solid red to compare it with the color at which borders are drawn when the style is inset.

Observation:-

1. Firefox:- Darken the left and top border, lighten the right and bottom border.
2. Opera:- Darken the left and top border, lighten the right and bottom border . Note:- But the color with which the right and bottom border are drawn are still darker than the one with which corresponding border for firefox are drawn.
3. IE7 :- The IE rendering is completely different form Opera, firefox.
Comment 30 Antaryami Pandia 2011-10-05 00:20:50 PDT
Created attachment 109752 [details]
Patch for getting Simon's feedback.

Patch for getting Simon's feedback only. (Doesn't contains pixel test results)
Comment 31 Antaryami Pandia 2011-10-05 00:23:20 PDT
(In reply to comment #25)
> Thanks Simon.
> 
> (In reply to comment #24)
> > Antaryami: I think we need to tweak the patch. Rather than lightening one side and darkening the other (which leaves the two colors two steps apart), I think we should just darken one for light colors, and lighten the other for dark colors.

I have uploaded a modified patch. Please provide your feedback.
Comment 32 Antaryami Pandia 2011-10-11 23:14:49 PDT
Created attachment 110641 [details]
Updated patch

Updated patch.

Using "baseDarkColor" and "baseLightColor" color to determine whether a color is on darker side or lighter side.
For e.g. if the color has a value which is between black and baseDarkColor, then its assumed that it's a darker color.

Please note that the values of "baseDarkColor" and "baseLightColor" are set based on some observation and can be modified.
Comment 33 Antaryami Pandia 2011-10-11 23:15:53 PDT
Comment on attachment 110641 [details]
Updated patch

Forgot to tick "patch" .
Comment 34 WebKit Review Bot 2011-10-11 23:40:24 PDT
Comment on attachment 110641 [details]
Updated patch

Attachment 110641 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/10027781

New failing tests:
css1/basic/grouping.html
css1/box_properties/border_bottom_width.html
css1/box_properties/border_bottom_width_inline.html
css1/box_properties/border_right_width.html
css1/box_properties/border_left_width.html
css1/basic/inheritance.html
css1/box_properties/border_color_inline.html
css1/box_properties/border_inline.html
css1/basic/class_as_selector.html
css1/box_properties/border.html
css1/box_properties/border_right.html
css1/box_properties/border_color.html
css1/box_properties/border_left_width_inline.html
css1/basic/id_as_selector.html
css1/box_properties/border_left.html
css1/box_properties/border_bottom_inline.html
css1/box_properties/border_right_inline.html
css1/box_properties/border_bottom.html
css1/box_properties/border_left_inline.html
css1/basic/contextual_selectors.html
Comment 35 Adam Barth 2011-10-15 01:52:37 PDT
Comment on attachment 108107 [details]
Patch

Cleared Simon Fraser's review+ from obsolete attachment 108107 [details] so that this bug does not appear in http://webkit.org/pending-commit.
Comment 36 Eric Seidel (no email) 2011-12-19 11:13:26 PST
Comment on attachment 110641 [details]
Updated patch

This needs ChangeLogs, otherwise looks pretty good.
Comment 37 Antaryami Pandia 2011-12-22 02:50:58 PST
Created attachment 120295 [details]
patch with changelog

patch with change log.
Comment 38 WebKit Review Bot 2011-12-22 03:22:08 PST
Comment on attachment 120295 [details]
patch with changelog

Attachment 120295 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/11001396

New failing tests:
css1/box_properties/border_bottom_inline.html
css1/classification/list_style.html
css1/box_properties/border_bottom_width.html
css1/box_properties/border_bottom_width_inline.html
css1/cascade/cascade_order.html
css1/cascade/important.html
css1/basic/inheritance.html
css1/box_properties/border_color_inline.html
css1/box_properties/border_inline.html
css1/basic/class_as_selector.html
css1/box_properties/border.html
css1/box_properties/border_color.html
css1/basic/id_as_selector.html
css1/basic/grouping.html
css1/classification/display.html
css1/box_properties/border_bottom.html
css1/basic/contextual_selectors.html
Comment 39 WebKit Review Bot 2011-12-22 03:22:17 PST
Created attachment 120302 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: Chromium  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 40 Simon Fraser (smfr) 2011-12-22 08:23:07 PST
Comment on attachment 120295 [details]
patch with changelog

Doesn't this change the pixel results of a lot more tests (like table tests)?
Comment 41 Antaryami Pandia 2011-12-22 21:59:55 PST
I run the test on "LayoutTests/fast/table" and found 157 tests failing. The break-up is as follows:-

Image+text - 1
Image - 125
no expected result found - 31

I ran the tests with below commands:-
new-run-webkit-tests --gtk --pixel LayoutTests/fast/table

When I ran the test without "--pixel", all tests passed.

Request you to help with below doubts:-
1. Is there any way I any change the expected result for "n" number of tests using a single command. (is it --rebaseline ??)
2. For tests which don't have any expected result currently (31 tests), should I generate a new one?
3. With the new expected result the patch will be significantly big.So how to upload a big patch?
Comment 42 Simon Fraser (smfr) 2012-04-19 14:38:49 PDT
Comment on attachment 120295 [details]
patch with changelog

Code change looks OK but r- for lack of new baselines
Comment 43 Antaryami Pandia 2013-11-26 22:00:10 PST
Hi Simon,
Now that chromium port is out, can we check in the patch (The patch was earlier failing for chromium-ews).
I will create a patch with latest webkit code base and then upload.

-Antaryami
Comment 44 Adenilson Cavalcanti Silva 2014-11-25 14:00:55 PST
Created attachment 242209 [details]
Vanilla test results (OSX 10.9.5): fast/css fast/table fast/borders
Comment 45 Adenilson Cavalcanti Silva 2014-11-25 14:01:41 PST
Created attachment 242210 [details]
Patched test results (OSX 10.9.5): fast/css fast/table fast/borders
Comment 46 Adenilson Cavalcanti Silva 2014-11-25 14:13:35 PST
Created attachment 242211 [details]
Updated patch
Comment 47 Adenilson Cavalcanti Silva 2014-11-25 14:39:46 PST
Created attachment 242213 [details]
Adding the pixel test output.
Comment 48 Adenilson Cavalcanti Silva 2014-11-25 17:18:43 PST
Created attachment 242217 [details]
Vanilla tests: fast/css fast/table fast/borders
Comment 49 Adenilson Cavalcanti Silva 2014-11-25 17:24:21 PST
Created attachment 242218 [details]
Patched: fast/css fast/table fast/borders
Comment 50 Adenilson Cavalcanti Silva 2014-11-30 17:27:58 PST
Created attachment 242296 [details]
Rewrote function, added rebaseline in TestExpectations
Comment 51 WebKit Commit Bot 2014-11-30 17:30:04 PST
Attachment 242296 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderObject.cpp:2458:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderObject.cpp:2463:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 52 Simon Fraser (smfr) 2014-12-01 11:06:21 PST
Comment on attachment 242296 [details]
Rewrote function, added rebaseline in TestExpectations

View in context: https://bugs.webkit.org/attachment.cgi?id=242296&action=review

Does this patch fix the table rendering issues that the earlier patch broke?

> LayoutTests/TestExpectations:143
> +webkit.org/b/58608 fast/borders/mixed-border-style2.html [ Rebaseline ]

Why did you add this here? It's just adding technical debt that someone has to clean up later.

> LayoutTests/fast/borders/mixed-border-style2.html:7
> +      -moz-box-sizing: border-box;

You can remove this.

> Source/WebCore/rendering/RenderObject.cpp:2451
> +    const RGBA32 baseDarkColor = 0xFF202020;
> +    const RGBA32 baseLightColor = 0xFFEBEBEB;

Where did these magic numbers come from?

> Source/WebCore/rendering/RenderObject.cpp:2455
> +    Operation operation = (side == BSTop || side == BSLeft) == (style == INSET) ?
> +        Darken : Lighten;

This line doesn't need to wrap.

> Source/WebCore/rendering/RenderObject.cpp:2467
> +    if (operation == Darken) {
> +        if (differenceSquared(color, Color::black) >
> +            differenceSquared(baseDarkColor, Color::black)) {
> +            color = color.dark();
> +        }
> +    } else {
> +        if (differenceSquared(color, Color::white) >
> +            differenceSquared(baseLightColor, Color::white)) {
> +            color = color.light();
> +        }
> +    }

Would be nice to add a comment justifying this behavior.
Comment 53 Adenilson Cavalcanti Silva 2014-12-01 12:20:39 PST
Created attachment 242327 [details]
css1 tests output

All CSS1 tests are running OK.
Comment 54 Adenilson Cavalcanti Silva 2014-12-01 12:28:10 PST
Simon

Thanks for reviewing the patch. I will comment inline.

> Does this patch fix the table rendering issues that the earlier patch broke?
> 

From the old messages, it seems that CSS1 was failing. I just attached the output for those tests and all is good. 

I also tested (fast/css, fast/table, fast/borders) and there are no regressions.


> 
> Why did you add this here? It's just adding technical debt that someone has
> to clean up later.
> 

I'm a bit unfamiliar with new tests when they aren't reftests. Should I remove it from TestExpectations?


> > LayoutTests/fast/borders/mixed-border-style2.html:7
> > +      -moz-box-sizing: border-box;
> 
> You can remove this.
> 

Coolio, will upload a new patch addressing this.



> 
> Where did these magic numbers come from?
> 

They came from the original patch, I tried other values but those seems to work better.


> > Source/WebCore/rendering/RenderObject.cpp:2455
> > +    Operation operation = (side == BSTop || side == BSLeft) == (style == INSET) ?
> > +        Darken : Lighten;
> 
> This line doesn't need to wrap.
> 

All in a single line then? Ok.



> 
> Would be nice to add a comment justifying this behavior.

What about '// Here we will darken the border decoration color when needed. This will yield a similar behavior as in FF.' ?
Comment 55 Adenilson Cavalcanti Silva 2014-12-01 13:31:20 PST
Created attachment 242339 [details]
Addressing reviewer's comments.
Comment 56 WebKit Commit Bot 2014-12-01 13:34:03 PST
Attachment 242339 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderObject.cpp:2458:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderObject.cpp:2463:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 57 Adenilson Cavalcanti Silva 2014-12-01 13:39:39 PST
Created attachment 242342 [details]
Addressing reviewer's comments, updated test baseline.
Comment 58 WebKit Commit Bot 2014-12-01 13:41:27 PST
Attachment 242342 [details] did not pass style-queue:


ERROR: Source/WebCore/rendering/RenderObject.cpp:2458:  Multi line control clauses should use braces.  [whitespace/braces] [4]
ERROR: Source/WebCore/rendering/RenderObject.cpp:2463:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 2 in 8 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 59 Adenilson Cavalcanti Silva 2014-12-01 16:00:34 PST
Created attachment 242358 [details]
Patched: Tables layout tests

Table layout tests are ok too.
Comment 60 Chris Dumez 2014-12-02 11:04:17 PST
Comment on attachment 242342 [details]
Addressing reviewer's comments, updated test baseline.

View in context: https://bugs.webkit.org/attachment.cgi?id=242342&action=review

> LayoutTests/TestExpectations:143
> +

Unrelated change.

> LayoutTests/fast/borders/mixed-border-style2.html:17
> +  <div class="box"></div>

It is not obvious from this test what the expected output is. How does someone rebaselining know if the pixel result is correct?
Also, could this be made a ref test somehow?

> Source/WebCore/rendering/RenderObject.cpp:2495
> +

Unrelated change.

> Source/WebCore/rendering/RenderObject.h:875
> +    void calculateBorderStyleColor(const EBorderStyle&, const BoxSide&, Color&) const;

Can this be static?
Comment 61 Simon Fraser (smfr) 2014-12-02 14:00:52 PST
Comment on attachment 242342 [details]
Addressing reviewer's comments, updated test baseline.

View in context: https://bugs.webkit.org/attachment.cgi?id=242342&action=review

> Source/WebCore/rendering/RenderObject.cpp:2451
> +    const RGBA32 baseDarkColor = 0xFF202020;
> +    const RGBA32 baseLightColor = 0xFFEBEBEB;

I would still like to see a comment that says "derived empirically" or something.

> Source/WebCore/rendering/RenderObject.cpp:2459
> +        if (differenceSquared(color, Color::black) >
> +            differenceSquared(baseDarkColor, Color::black)) {

Please unwrap.

> Source/WebCore/rendering/RenderObject.cpp:2464
> +        if (differenceSquared(color, Color::white) >
> +            differenceSquared(baseLightColor, Color::white)) {

Please unwrap.
Comment 62 Adenilson Cavalcanti Silva 2014-12-02 15:29:53 PST
Created attachment 242456 [details]
Updated + fixes
Comment 63 WebKit Commit Bot 2014-12-04 14:01:05 PST
Comment on attachment 242456 [details]
Updated + fixes

Clearing flags on attachment: 242456

Committed r176816: <http://trac.webkit.org/changeset/176816>
Comment 64 WebKit Commit Bot 2014-12-04 14:01:15 PST
All reviewed patches have been landed.  Closing bug.
Comment 65 Alexey Proskuryakov 2014-12-05 11:53:49 PST
This patch added results for mac-mavericks, which also covers mac-mountainlion, please land results for yosemite into platform/mac.
Comment 66 Adenilson Cavalcanti Silva 2014-12-09 15:32:42 PST
An update: image and text results are the same in Yosemite X Mavericks.

So I moved the image result to mac folder on changeset: http://trac.webkit.org/changeset/177042

Previous changeset #176927 had moved the text result.