Bug 58608 – Groove/inset/outset borders show solid if the color is black

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Simon Fraser (smfr)]

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.

[Simon Fraser (smfr)]

Created attachment 89689 [details]
Testcase

[Simon Fraser (smfr)]

Created attachment 90923 [details]
IE9 rendering for the test in the URL

[Simon Fraser (smfr)]

Created attachment 90928 [details]
Firefox 4 rendering

[Antaryami Pandia]

Created attachment 107810 [details]
Patch.

[WebKit Review Bot]

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

[Simon Fraser (smfr)]

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.

[Antaryami Pandia]

Created attachment 107970 [details]
Patch

Updated patch as per review comments.

[WebKit Review Bot]

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

[Antaryami Pandia]

Comment on attachment 107970 [details]
Patch

It's still failing for cr-linux.Will check and upload a modified patch

[Antaryami Pandia]

(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
> > +		 
> 
> No need for that.

Ok.

[Simon Fraser (smfr)]

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.

[Antaryami Pandia]

Created attachment 108107 [details]
Patch

Modified patch.

[WebKit Review Bot]

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

[Simon Fraser (smfr)]

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.

[Antaryami Pandia]

Created attachment 108276 [details]
Patch

Updated patch.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Antaryami Pandia]

Hi Simon.
Please help committing the patch.

BR,
-Antaryami

[Simon Fraser (smfr)]

Comment on attachment 108276 [details]
Patch

Clearing flags on attachment: 108276

Committed r95960: <http://trac.webkit.org/changeset/95960>

[Simon Fraser (smfr)]

All reviewed patches have been landed.  Closing bug.

[Mihai Parparita]

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.

[Mihai Parparita]

Reverted r95960 for reason:

Significantly changes table border rendering

Committed r95974: <http://trac.webkit.org/changeset/95974>

[Mihai Parparita]

I reverted after discussing with Simon in #webkit.

[Simon Fraser (smfr)]

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.

[Antaryami Pandia]

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.

[Antaryami Pandia]

Created attachment 109747 [details]
firefox-behavior

[Antaryami Pandia]

Created attachment 109748 [details]
opera-behavior

[Antaryami Pandia]

Created attachment 109749 [details]
IE7-behavior

[Antaryami Pandia]

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.

[Antaryami Pandia]

Created attachment 109752 [details]
Patch for getting Simon's feedback.

Patch for getting Simon's feedback only. (Doesn't contains pixel test results)

[Antaryami Pandia]

(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.

[Antaryami Pandia]

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.

[Antaryami Pandia]

Comment on attachment 110641 [details]
Updated patch

Forgot to tick "patch" .

[WebKit Review Bot]

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

[Adam Barth]

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.

[Eric Seidel (no email)]

Comment on attachment 110641 [details]
Updated patch

This needs ChangeLogs, otherwise looks pretty good.

[Antaryami Pandia]

Created attachment 120295 [details]
patch with changelog

patch with change log.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Simon Fraser (smfr)]

Comment on attachment 120295 [details]
patch with changelog

Doesn't this change the pixel results of a lot more tests (like table tests)?

[Antaryami Pandia]

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?

[Simon Fraser (smfr)]

Comment on attachment 120295 [details]
patch with changelog

Code change looks OK but r- for lack of new baselines

[Antaryami Pandia]

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

[Adenilson Cavalcanti Silva]

Created attachment 242209 [details]
Vanilla test results (OSX 10.9.5): fast/css fast/table fast/borders

[Adenilson Cavalcanti Silva]

Created attachment 242210 [details]
Patched test results (OSX 10.9.5): fast/css fast/table fast/borders

[Adenilson Cavalcanti Silva]

Created attachment 242211 [details]
Updated patch

[Adenilson Cavalcanti Silva]

Created attachment 242213 [details]
Adding the pixel test output.

[Adenilson Cavalcanti Silva]

Created attachment 242217 [details]
Vanilla tests: fast/css fast/table fast/borders

[Adenilson Cavalcanti Silva]

Created attachment 242218 [details]
Patched: fast/css fast/table fast/borders

[Adenilson Cavalcanti Silva]

Created attachment 242296 [details]
Rewrote function, added rebaseline in TestExpectations

[WebKit Commit Bot]

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.

[Simon Fraser (smfr)]

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.

[Adenilson Cavalcanti Silva]

Created attachment 242327 [details]
css1 tests output

All CSS1 tests are running OK.

[Adenilson Cavalcanti Silva]

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.' ?

[Adenilson Cavalcanti Silva]

Created attachment 242339 [details]
Addressing reviewer's comments.

[WebKit Commit Bot]

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.

[Adenilson Cavalcanti Silva]

Created attachment 242342 [details]
Addressing reviewer's comments, updated test baseline.

[WebKit Commit Bot]

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.

[Adenilson Cavalcanti Silva]

Created attachment 242358 [details]
Patched: Tables layout tests

Table layout tests are ok too.

[Chris Dumez]

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?

[Simon Fraser (smfr)]

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.

[Adenilson Cavalcanti Silva]

Created attachment 242456 [details]
Updated + fixes

[WebKit Commit Bot]

Comment on attachment 242456 [details]
Updated + fixes

Clearing flags on attachment: 242456

Committed r176816: <http://trac.webkit.org/changeset/176816>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This patch added results for mac-mavericks, which also covers mac-mountainlion, please land results for yosemite into platform/mac.

[Adenilson Cavalcanti Silva]

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.