Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
759373002:
Better handling border color style decoration (Closed)
DescriptionHandling border color decoration in a similar way as Firefox does, lightening the border side colors if needed.
BUG=436990
Patch Set 1 : Groove/inset/outset borders show solid if the color is black #
Total comments: 8
Patch Set 2 #
Total comments: 1
Patch Set 3 : Remove unset border color in RenderStyle #
Total comments: 3
Patch Set 4 : Changing the test per reviewer's request. #
Total comments: 6
Messages
Total messages: 28 (3 generated)
mstensho@opera.com changed reviewers: + chrishtr@chromium.org, mstensho@opera.com
I'm not much of a color guy, so I cannot really assess the correctness of the patch (although what this change does seems good to me). Added an OWNER, to finish this review. I just commented on some style issues. A couple of more issues: I'm pretty sure that we cannot do better then a pixel test in this case (i.e. there's no way to write a good reftest, is there?). So your test is what we need, but then we need to generate results for the other platforms too, so you need to add a [ NeedsRebaseline ] to LayoutTests/TestExpectations for this new test. Also, you should fix your commit comment. The first line should just be a good and short summary of what the patch does. Right now the title of this CL is "This patch will lighten the border side side colors, handling border" :) The first line should be followed by a blank line, followed by a more elaborate description of the patch. https://codereview.chromium.org/759373002/diff/1/LayoutTests/fast/borders/mix... File LayoutTests/fast/borders/mixed-border-style2.html (right): https://codereview.chromium.org/759373002/diff/1/LayoutTests/fast/borders/mix... LayoutTests/fast/borders/mixed-border-style2.html:2: <html> HTML, HEAD and BODY tags can be omitted. https://codereview.chromium.org/759373002/diff/1/LayoutTests/fast/borders/mix... LayoutTests/fast/borders/mixed-border-style2.html:4: <style type="text/css">.box { No need for the type attribute. And please move the selector to the next line. https://codereview.chromium.org/759373002/diff/1/LayoutTests/fast/borders/mix... LayoutTests/fast/borders/mixed-border-style2.html:6: box-sizing: border-box; Setting display and box-sizing seems unnecessary? https://codereview.chromium.org/759373002/diff/1/LayoutTests/fast/borders/mix... LayoutTests/fast/borders/mixed-border-style2.html:7: -moz-box-sizing: border-box; No need for this. https://codereview.chromium.org/759373002/diff/1/LayoutTests/fast/borders/mix... LayoutTests/fast/borders/mixed-border-style2.html:13: border-style: solid; This declaration has no effect. https://codereview.chromium.org/759373002/diff/1/Source/core/paint/ObjectPain... File Source/core/paint/ObjectPainter.cpp (right): https://codereview.chromium.org/759373002/diff/1/Source/core/paint/ObjectPain... Source/core/paint/ObjectPainter.cpp:342: void ObjectPainter::calculateBorderStyleColor(const EBorderStyle& style, const BoxSide& side, Color& color) Should probably be called modifyBorderColorForStyleIfNeeded() or something like that. https://codereview.chromium.org/759373002/diff/1/Source/core/paint/ObjectPain... Source/core/paint/ObjectPainter.cpp:347: if ((side == BSTop || side == BSLeft)) { Extraneous pair of parentheses. https://codereview.chromium.org/759373002/diff/1/Source/core/paint/ObjectPain... Source/core/paint/ObjectPainter.cpp:357: if ((side == BSBottom || side == BSRight)) { Should be "else if", or actually just "else" (I mean, are there more than those four sides?) Also extraneous pair of parentheses. But it strikes me that this block is very similar to the block above. How about simplifying everything to: enum Operation { Darken, Lighten }; Operation operation = (side == BSTop || side == BSLeft) == (style == INSET) ? Darken : Lighten; 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(); }
mstencho Thanks for reviewing the patch, I will be addressing the comments in a new patch pretty soon. I specially liked the suggestion concerning the simplification of the color setting function.
On 2014/12/01 02:14:29, Savago wrote: Do we want to remove our special-casing of un-set borders (see the bottom of RenderStyle::colorIncludingFallback)?
https://codereview.chromium.org/759373002/diff/20001/Source/core/paint/Object... File Source/core/paint/ObjectPainter.cpp (right): https://codereview.chromium.org/759373002/diff/20001/Source/core/paint/Object... Source/core/paint/ObjectPainter.cpp:96: void ObjectPainter::drawLineForBoxSide(GraphicsContext* graphicsContext, int x1, int y1, int x2, int y2, Do we need to update BoxPainter::drawBoxSideFromPath too?
On 2014/12/01 02:37:08, Timothy Loh wrote: > On 2014/12/01 02:14:29, Savago wrote: > > Do we want to remove our special-casing of un-set borders (see the bottom of > RenderStyle::colorIncludingFallback)? Timothy Removing the unsetting border code at RenderStyle seems to fix both issues #276231 and #316559. I uploaded screenshots of the test cases in the bug I reported.
Looks fine (and getting rid of the hack in RenderStyle is great), but I think timloh should take over from here. One of his issues is still unaddressed BTW (regarding updating BoxPainter), right? https://codereview.chromium.org/759373002/diff/40001/LayoutTests/fast/borders... File LayoutTests/fast/borders/mixed-border-style2.html (right): https://codereview.chromium.org/759373002/diff/40001/LayoutTests/fast/borders... LayoutTests/fast/borders/mixed-border-style2.html:2: <html> No need for HTML, HEAD and BODY. https://codereview.chromium.org/759373002/diff/40001/LayoutTests/fast/borders... LayoutTests/fast/borders/mixed-border-style2.html:16: <!-- Test mixed border styles. --> Could you add some test expectation text here? I.e., what should we see? https://codereview.chromium.org/759373002/diff/40001/Source/core/paint/Object... File Source/core/paint/ObjectPainter.cpp (right): https://codereview.chromium.org/759373002/diff/40001/Source/core/paint/Object... Source/core/paint/ObjectPainter.cpp:344: const RGBA32 baseDarkColor = 0xFF202020; Instead of hard-coding it here, could these be static members of Color? Or would that count as pollution there? I don't know. I guess timloh would have an opinion.
mstensho Thanks for the comments, I will answer inline. > Looks fine (and getting rid of the hack in RenderStyle is great), but I think > timloh should take over from here. One of his issues is still unaddressed BTW > (regarding updating BoxPainter), right? > I rather do it in a second patch, as this one is already addressing bugs #316559, #276231 and #436990. > https://codereview.chromium.org/759373002/diff/40001/LayoutTests/fast/borders... > File LayoutTests/fast/borders/mixed-border-style2.html (right): > > https://codereview.chromium.org/759373002/diff/40001/LayoutTests/fast/borders... > LayoutTests/fast/borders/mixed-border-style2.html:2: <html> > No need for HTML, HEAD and BODY. > > https://codereview.chromium.org/759373002/diff/40001/LayoutTests/fast/borders... > LayoutTests/fast/borders/mixed-border-style2.html:16: <!-- Test mixed border > styles. --> > Could you add some test expectation text here? I.e., what should we see? > Coolio, will upload a patch fixing all those issues. > https://codereview.chromium.org/759373002/diff/40001/Source/core/paint/Object... > File Source/core/paint/ObjectPainter.cpp (right): > > https://codereview.chromium.org/759373002/diff/40001/Source/core/paint/Object... > Source/core/paint/ObjectPainter.cpp:344: const RGBA32 baseDarkColor = > 0xFF202020; > Instead of hard-coding it here, could these be static members of Color? Or would > that count as pollution there? I don't know. I guess timloh would have an > opinion. I rather keep changes local where they are needed. If in future we see the need of using this variables in other places, then I favor the idea of moving it to Color.
https://codereview.chromium.org/759373002/diff/60001/Source/core/paint/Object... File Source/core/paint/ObjectPainter.cpp (right): https://codereview.chromium.org/759373002/diff/60001/Source/core/paint/Object... Source/core/paint/ObjectPainter.cpp:344: const RGBA32 baseDarkColor = 0xFF202020; This needs a comment explaining where these numbers come from. Please link to the spec it came from. https://codereview.chromium.org/759373002/diff/60001/Source/core/paint/Object... File Source/core/paint/ObjectPainter.h (right): https://codereview.chromium.org/759373002/diff/60001/Source/core/paint/Object... Source/core/paint/ObjectPainter.h:36: static void modifyBorderColorForStyleIfNeeded(const EBorderStyle&, const BoxSide&, Color&); Should be private, not public.
timloh@chromium.org changed reviewers: + timloh@chromium.org
This changes >1000 test expectations. I think we should be careful here, since we don't want to accidentally change what <hr>s and table borders look like. Maybe we can add UA rules for these to minimise the change here? Ideally we could keep the unset border color but I can't see how to do this in CSS :/
On 2014/12/01 23:02:30, Timothy Loh wrote: > This changes >1000 test expectations. I think we should be careful here, since > we don't want to accidentally change what <hr>s and table borders look like. > Maybe we can add UA rules for these to minimise the change here? Ideally we > could keep the unset border color but I can't see how to do this in CSS :/ Timothy So I decided to try to separate the variables to have a better understanding. The change on RenderStyle makes 739 tests change their results (versus 724 when only the border color code in ObjectPainter is changed), so not a big effect on the original patch. Speaking of tables, there are 685 image-only regressions for tables. But a closer look on those tests reveals that indeed, the borders of table cells are painted in a different way (as expected), but the behavior now looks more correct (i.e. closer to the way that FF renders). I just attached some screenshots in the original bug showing the changes (tests tables/layering/paint-test-layering1.html and tables/mozilla/bugs/bug10009.html). So, what are our options here? I can see: a) Just left border coloring as it is (i.e. kind of broken). That is not desirable as there are cases (e.g. #316559) where the current state is clearly wrong. b) Update the tests results (not ideal), as there are tons of tests and it would require to look at each one to see if the changed results is indeed an improvement or not. c) Don't apply the decoration color change if the object is a Table element? (not sure if that is feasible or not, really). d) Something else?
https://codereview.chromium.org/759373002/diff/60001/Source/core/paint/Object... File Source/core/paint/ObjectPainter.h (right): https://codereview.chromium.org/759373002/diff/60001/Source/core/paint/Object... Source/core/paint/ObjectPainter.h:36: static void modifyBorderColorForStyleIfNeeded(const EBorderStyle&, const BoxSide&, Color&); On 2014/12/01 22:51:31, chrishtr wrote: > Should be private, not public. It makes sense, but observe that all the static class member functions in ObjectPainter are public too (makes sense to follow the same style to maintain consistency).
pdr@chromium.org changed reviewers: + pdr@chromium.org
Drive by comments. Do you have a link to where Gecko computes this? https://codereview.chromium.org/759373002/diff/60001/Source/core/paint/Object... File Source/core/paint/ObjectPainter.cpp (right): https://codereview.chromium.org/759373002/diff/60001/Source/core/paint/Object... Source/core/paint/ObjectPainter.cpp:352: differenceSquared(baseDarkColor, Color::black)) { differenceSquared(baseDarkColor, Color::black) and differenceSquared(baseLightColor, Color::white) are constant so we should be able to compute them ahead of time. https://codereview.chromium.org/759373002/diff/60001/Source/core/paint/Object... File Source/core/paint/ObjectPainter.h (right): https://codereview.chromium.org/759373002/diff/60001/Source/core/paint/Object... Source/core/paint/ObjectPainter.h:36: static void modifyBorderColorForStyleIfNeeded(const EBorderStyle&, const BoxSide&, Color&); On 2014/12/02 at 01:02:39, Savago wrote: > On 2014/12/01 22:51:31, chrishtr wrote: > > Should be private, not public. > > It makes sense, but observe that all the static class member functions in ObjectPainter are public too (makes sense to follow the same style to maintain consistency). This was a mistake, lets not be consistently wrong :)
https://codereview.chromium.org/759373002/diff/60001/Source/core/paint/Object... File Source/core/paint/ObjectPainter.h (right): https://codereview.chromium.org/759373002/diff/60001/Source/core/paint/Object... Source/core/paint/ObjectPainter.h:36: static void modifyBorderColorForStyleIfNeeded(const EBorderStyle&, const BoxSide&, Color&); On 2014/12/02 02:55:49, pdr wrote: > On 2014/12/02 at 01:02:39, Savago wrote: > > On 2014/12/01 22:51:31, chrishtr wrote: > > > Should be private, not public. > > > > It makes sense, but observe that all the static class member functions in > ObjectPainter are public too (makes sense to follow the same style to maintain > consistency). > > This was a mistake, lets not be consistently wrong :) Some of those are public because they are used from another class (e.g. drawLineForBoxSide() ). For the ones that do not need to be public, please make them private while you are at it.
> Some of those are public because they are used from another class (e.g. > drawLineForBoxSide() ). For the ones that do not need to be public, please make > them private > while you are at it. Since we still have an ongoing discussion on the tests subject, I pushed the change related to private methods on: https://codereview.chromium.org/773893002/
Can we get some progress here?
On 2015/01/07 10:07:52, mstensho wrote: > Can we get some progress here? mstensho I'm on vacations now, planning to resume this patch when I'm back. Regards Adenilson
mstensho Perhaps relevant, basically the same patch has landed successfully on WebKit last December: https://bugs.webkit.org/show_bug.cgi?id=58608 And while I was at it, I also fixed the same issue on Mozilla's Servo (landed 26 days ago): https://github.com/servo/servo/pull/4324 With that, FF + WebKit + IE + Servo have the similar behavior that I'm trying to implement in blink. As it was pointed on the discussion by Mozilla's developers, the *exact* behavior of groove/inset/outset is not specified on http://dev.w3.org/csswg/css-backgrounds/#the-border-style. I'm planning to work to fix the spec and present a suggestion to W3C CSS WG to address it (as I'm member of the WG). As you know, it may take some time to get this into the spec. As we are changing the way that rendering is performed on Blink on this patch, I'm unable to see a way to maintain the same old test expectations. Any one have any suggestions? Adenilson
On 2015/01/07 19:55:57, Savago wrote: > As we are changing the way that rendering is performed on Blink on this patch, > I'm unable to see a way to maintain the same old test expectations. Any one have > any suggestions? Judging from the comments here, I think the question is whether we want to change from one behavior to another when there's no spec for it, as long as this doesn't solve any problem on the world wild web. It also doesn't sound too good to affect the rendering of HRs.
Any progress? If not, I suggest that we drop this CL.
On 2015/02/19 13:52:34, mstensho wrote: > Any progress? If not, I suggest that we drop this CL. Until the spec is updated to address this case, I'm going to close the CL.
Message was sent while issue was closed.
On 2015/03/03 23:04:36, Savago wrote: > On 2015/02/19 13:52:34, mstensho wrote: > > Any progress? If not, I suggest that we drop this CL. > > Until the spec is updated to address this case, I'm going to close the CL. While the spec does not provide any details on the exact color-picking algorithm for grooves, it nonetheless does specify that it should look 3D: “Looks as if it were carved in the canvas. (This is typically achieved by creating a “shadow” from two colors that are slightly lighter and darker than the ‘border-color’.)” Since a flat black line does not, in any sense, look "as if it were carved in the canvas", Chromium is in violation of the spec as it stands.
Message was sent while issue was closed.
An update: The W3C CSS WG has decided that the current behavior in Chrome is wrong and will append the spec to make it clear: https://lists.w3.org/Archives/Public/www-style/2015Sep/0252.html That enables to continue work on this. My plan is to port/re-write the patch and submit it for review in a couple days. Adenilson Cavalcanti |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
