Bug 58608

Summary:

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

Product:

WebKit

Reporter:

Simon Fraser (smfr) <simon.fraser>

Component:

CSS

Assignee:

Nobody <webkit-unassigned>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

apandia.ttg, cdumez, commit-queue, dglazkov, esprehn+autocc, glenn, hyatt, kondapallykalyan, savagobr, simon.fraser, webkit.review.bot, xqb748

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

OS X 10.5

URL:

http://smfr.org/misc/tables.html

Bug Depends on:

Bug Blocks:

139190

Attachments:

Description	Flags
Testcase	none
IE9 rendering for the test in the URL	none
Firefox 4 rendering	none
Patch.	simon.fraser: review-, webkit.review.bot: commit-queue-
Patch	simon.fraser: review-, webkit.review.bot: commit-queue-
Patch	none
Patch	none
Archive of layout-test-results from ec2-cr-linux-03	none
firefox-behavior	none
opera-behavior	none
IE7-behavior	none
Patch for getting Simon's feedback.	none
Updated patch	eric: review-, webkit.review.bot: commit-queue-
patch with changelog	simon.fraser: review-, webkit.review.bot: commit-queue-
Archive of layout-test-results from ec2-cr-linux-01	none
Vanilla test results (OSX 10.9.5): fast/css fast/table fast/borders	none
Patched test results (OSX 10.9.5): fast/css fast/table fast/borders	none
Updated patch	none
Adding the pixel test output.	none
Vanilla tests: fast/css fast/table fast/borders	none
Patched: fast/css fast/table fast/borders	none
Rewrote function, added rebaseline in TestExpectations	simon.fraser: review+, simon.fraser: commit-queue-
css1 tests output	none
Addressing reviewer's comments.	none
Addressing reviewer's comments, updated test baseline.	none
Patched: Tables layout tests	none
Updated + fixes	none

Simon Fraser (smfr)

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

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
Show Obsolete (13) View All Add attachment proposed patch, testcase, etc.

Simon Fraser (smfr)

Comment 1 2011-04-14 17:09:18 PDT

Created attachment 89689 [details] Testcase

Simon Fraser (smfr)

Comment 2 2011-04-25 10:05:02 PDT

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

Simon Fraser (smfr)

Comment 3 2011-04-25 10:39:30 PDT

Created attachment 90928 [details] Firefox 4 rendering

Antaryami Pandia

Comment 4 2011-09-19 00:05:25 PDT

Created attachment 107810 [details] Patch.

WebKit Review Bot

Comment 5 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

Simon Fraser (smfr)

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

Antaryami Pandia

Comment 7 2011-09-19 23:42:53 PDT

Created attachment 107970 [details] Patch Updated patch as per review comments.

WebKit Review Bot

Comment 8 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

Antaryami Pandia

Comment 9 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

Antaryami Pandia

Comment 10 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 > > +   > > No need for that. Ok.

Simon Fraser (smfr)

Comment 11 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.

Antaryami Pandia

Comment 12 2011-09-20 22:07:11 PDT

Created attachment 108107 [details] Patch Modified patch.

WebKit Review Bot

Comment 13 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

Simon Fraser (smfr)

Comment 14 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.

Antaryami Pandia

Comment 15 2011-09-21 22:19:15 PDT

Created attachment 108276 [details] Patch Updated patch.

WebKit Review Bot

Comment 16 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

WebKit Review Bot

Comment 17 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

Antaryami Pandia

Comment 18 2011-09-25 21:35:03 PDT

Hi Simon. Please help committing the patch. BR, -Antaryami

Simon Fraser (smfr)

Comment 19 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>

Simon Fraser (smfr)

Comment 20 2011-09-26 10:33:14 PDT

All reviewed patches have been landed. Closing bug.

Mihai Parparita

Comment 21 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.

Mihai Parparita

Comment 22 2011-09-26 12:42:58 PDT

Reverted r95960 for reason: Significantly changes table border rendering Committed r95974: <http://trac.webkit.org/changeset/95974>

Mihai Parparita

Comment 23 2011-09-26 12:44:44 PDT

I reverted after discussing with Simon in #webkit.

Simon Fraser (smfr)

Comment 24 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.

Antaryami Pandia

Comment 25 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.

Antaryami Pandia

Comment 26 2011-10-04 23:54:23 PDT

Created attachment 109747 [details] firefox-behavior

Antaryami Pandia

Comment 27 2011-10-04 23:55:03 PDT

Created attachment 109748 [details] opera-behavior

Antaryami Pandia

Comment 28 2011-10-04 23:55:39 PDT

Created attachment 109749 [details] IE7-behavior

Antaryami Pandia

Comment 29 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.

Antaryami Pandia

Comment 30 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)

Antaryami Pandia

Comment 31 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.

Antaryami Pandia

Comment 32 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.

Antaryami Pandia

Comment 33 2011-10-11 23:15:53 PDT

Comment on attachment 110641 [details] Updated patch Forgot to tick "patch" .

WebKit Review Bot

Comment 34 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

Adam Barth

Comment 35 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.

Eric Seidel (no email)

Comment 36 2011-12-19 11:13:26 PST

Comment on attachment 110641 [details] Updated patch This needs ChangeLogs, otherwise looks pretty good.

Antaryami Pandia

Comment 37 2011-12-22 02:50:58 PST

Created attachment 120295 [details] patch with changelog patch with change log.

WebKit Review Bot

Comment 38 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

WebKit Review Bot

Comment 39 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

Simon Fraser (smfr)

Comment 40 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)?

Antaryami Pandia

Comment 41 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?

Simon Fraser (smfr)

Comment 42 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

Antaryami Pandia

Comment 43 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

Adenilson Cavalcanti Silva

Comment 44 2014-11-25 14:00:55 PST

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

Adenilson Cavalcanti Silva

Comment 45 2014-11-25 14:01:41 PST

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

Adenilson Cavalcanti Silva

Comment 46 2014-11-25 14:13:35 PST

Created attachment 242211 [details] Updated patch

Adenilson Cavalcanti Silva

Comment 47 2014-11-25 14:39:46 PST

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

Adenilson Cavalcanti Silva

Comment 48 2014-11-25 17:18:43 PST

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

Adenilson Cavalcanti Silva

Comment 49 2014-11-25 17:24:21 PST

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

Adenilson Cavalcanti Silva

Comment 50 2014-11-30 17:27:58 PST

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

WebKit Commit Bot

Comment 51 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.

Simon Fraser (smfr)

Comment 52 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.

Adenilson Cavalcanti Silva

Comment 53 2014-12-01 12:20:39 PST

Created attachment 242327 [details] css1 tests output All CSS1 tests are running OK.

Adenilson Cavalcanti Silva

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

Adenilson Cavalcanti Silva

Comment 55 2014-12-01 13:31:20 PST

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

WebKit Commit Bot

Comment 56 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.

Adenilson Cavalcanti Silva

Comment 57 2014-12-01 13:39:39 PST

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

WebKit Commit Bot

Comment 58 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.

Adenilson Cavalcanti Silva

Comment 59 2014-12-01 16:00:34 PST

Created attachment 242358 [details] Patched: Tables layout tests Table layout tests are ok too.

Chris Dumez

Comment 60 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?

Simon Fraser (smfr)

Comment 61 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.

Adenilson Cavalcanti Silva

Comment 62 2014-12-02 15:29:53 PST

Created attachment 242456 [details] Updated + fixes

WebKit Commit Bot

Comment 63 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>

WebKit Commit Bot

Comment 64 2014-12-04 14:01:15 PST

All reviewed patches have been landed. Closing bug.

Alexey Proskuryakov

Comment 65 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.

Adenilson Cavalcanti Silva

Comment 66 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.

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