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: | P2 | ||
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
Simon Fraser (smfr)
2011-04-14 17:09:01 PDT
Created attachment 89689 [details]
Testcase
Created attachment 90923 [details]
IE9 rendering for the test in the URL
Created attachment 90928 [details]
Firefox 4 rendering
Created attachment 107810 [details]
Patch.
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 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. Created attachment 107970 [details]
Patch
Updated patch as per review comments.
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 on attachment 107970 [details]
Patch
It's still failing for cr-linux.Will check and upload a modified patch
(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. 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. Created attachment 108107 [details]
Patch
Modified patch.
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 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. Created attachment 108276 [details]
Patch
Updated patch.
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 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
Hi Simon. Please help committing the patch. BR, -Antaryami Comment on attachment 108276 [details] Patch Clearing flags on attachment: 108276 Committed r95960: <http://trac.webkit.org/changeset/95960> All reviewed patches have been landed. Closing bug. 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. Reverted r95960 for reason: Significantly changes table border rendering Committed r95974: <http://trac.webkit.org/changeset/95974> I reverted after discussing with Simon in #webkit. 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. 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. Created attachment 109747 [details]
firefox-behavior
Created attachment 109748 [details]
opera-behavior
Created attachment 109749 [details]
IE7-behavior
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. Created attachment 109752 [details]
Patch for getting Simon's feedback.
Patch for getting Simon's feedback only. (Doesn't contains pixel test results)
(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. 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 on attachment 110641 [details]
Updated patch
Forgot to tick "patch" .
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 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 on attachment 110641 [details]
Updated patch
This needs ChangeLogs, otherwise looks pretty good.
Created attachment 120295 [details]
patch with changelog
patch with change log.
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 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 on attachment 120295 [details]
patch with changelog
Doesn't this change the pixel results of a lot more tests (like table tests)?
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 on attachment 120295 [details]
patch with changelog
Code change looks OK but r- for lack of new baselines
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 Created attachment 242209 [details]
Vanilla test results (OSX 10.9.5): fast/css fast/table fast/borders
Created attachment 242210 [details]
Patched test results (OSX 10.9.5): fast/css fast/table fast/borders
Created attachment 242211 [details]
Updated patch
Created attachment 242213 [details]
Adding the pixel test output.
Created attachment 242217 [details]
Vanilla tests: fast/css fast/table fast/borders
Created attachment 242218 [details]
Patched: fast/css fast/table fast/borders
Created attachment 242296 [details]
Rewrote function, added rebaseline in TestExpectations
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 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. Created attachment 242327 [details]
css1 tests output
All CSS1 tests are running OK.
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.' ? Created attachment 242339 [details]
Addressing reviewer's comments.
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.
Created attachment 242342 [details]
Addressing reviewer's comments, updated test baseline.
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.
Created attachment 242358 [details]
Patched: Tables layout tests
Table layout tests are ok too.
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 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. Created attachment 242456 [details]
Updated + fixes
Comment on attachment 242456 [details] Updated + fixes Clearing flags on attachment: 242456 Committed r176816: <http://trac.webkit.org/changeset/176816> All reviewed patches have been landed. Closing bug. This patch added results for mac-mavericks, which also covers mac-mountainlion, please land results for yosemite into platform/mac. 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. |