Preserve original border colors for border-style inset | outset.

Preserve original border colors for border-style inset | outset.

This will fix the case where a color is set to a parent element and
the expected behavior is children elements to render with the same
color previously defined.

BUG=316559

[cavalcantii1, 2016-10-06 22:42:39]

The CQ bit was checked by cavalcantii@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-10-06 22:43:21]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[cavalcantii1, 2016-10-06 23:29:34]

cavalcantii@chromium.org changed reviewers:
+ alancutter@chromium.org, meade@chromium.org, rbyers@google.com

[Rick Byers, 2016-10-07 00:10:25]

Description was changed from

==========
Preserve original border colors for border-style inset | outset.

This will fix the case where a color is set to a parent element and
the expected behavior is children elements to render with the same
color previously defined.

BUG=316559
==========

to

==========
Preserve original border colors for border-style inset | outset.

This will fix the case where a color is set to a parent element and
the expected behavior is children elements to render with the same
color previously defined.

BUG=316559
==========

[Rick Byers, 2016-10-07 00:10:25]

rbyers@chromium.org changed reviewers:
- rbyers@google.com

[Rick Byers, 2016-10-07 00:11:20]

Thanks for the fix!  I don't work in the style code myself, so removing myself
as a reviewer.  But Alan or Eddy should be good reviewers.  I suspect they may
ask you for a test though :-)

[alancutter (OOO until 2018), 2016-10-07 00:39:28]

+1 to adding tests. Are there tests for the colour of ridge and groove border
styles?

[commit-bot: I haz the power, 2016-10-07 00:41:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-10-07 00:41:17]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[meade_UTC10, 2016-10-07 04:03:29]

I'll also +1 tests. Also, there are 485 layout tests broken by this change.
Please check that all the changes are what you intended, and then you'll need to
rebaseline them as well.

[cavalcantii1, 2016-10-08 00:54:49]

@Rick
No problem, this one looked like an easy fix.

@Alan
Sure thing, I will write a layout test.

@Eddy
I'm planning to go through the failing tests, but most of them look like:

-  LayoutBlockFlow {HR} at (0,113) size 769x2 [border: (1px inset #EEEEEE)]
+  LayoutBlockFlow {HR} at (0,113) size 769x2 [border: (1px inset #000000)]

[meade_UTC10, 2016-10-11 06:22:10]

On 2016/10/08 00:54:49, cavalcantii1 wrote:
> @Rick
> No problem, this one looked like an easy fix.
> 
> @Alan
> Sure thing, I will write a layout test.
> 
> @Eddy
> I'm planning to go through the failing tests, but most of them look like:
> 
> -  LayoutBlockFlow {HR} at (0,113) size 769x2 [border: (1px inset #EEEEEE)]
> +  LayoutBlockFlow {HR} at (0,113) size 769x2 [border: (1px inset #000000)]

Those actually seem like a problem (as do the many image diff failures) - the
border colour and/or thickness is being changed, making lots of things look
different. A good (random) example of a change that is probably not what you
want is this one:
https://storage.googleapis.com/chromium-layout-test-archives/linux_chromium_r...

[cavalcantii1, 2016-10-11 17:45:33]

@Eddy
Thanks for pointing it out, I will have a closer look on it.

That being said, it probably would be fixed followed by this:
https://bugs.chromium.org/p/chromium/issues/detail?id=436990

I was trying to break it down in 2 patches as there are 2 rendering bugs
involved (i.e. solid black borders being rendered instead of a decorated border
X preserving the border colors).

[meade_UTC10, 2017-02-27 02:45:02]

meade@chromium.org changed reviewers:
- meade@chromium.org

[mstensho (USE GERRIT), 2017-06-01 13:06:33]

mstensho@opera.com changed reviewers:
+ mstensho@opera.com

[mstensho (USE GERRIT), 2017-06-01 13:06:34]

Why keep this behavior for ridge and groove at all?

[cavalcantii1, 2017-06-02 00:12:01]

As groove and ridge has other issues and I was planning to fix it in other patch
like I did in Safari (i.e. 
https://bugs.webkit.org/show_bug.cgi?id=58608).

[mstensho (USE GERRIT), 2017-06-08 09:17:01]

On 2017/06/02 00:12:01, cavalcantii1 wrote:
> As groove and ridge has other issues and I was planning to fix it in other
patch
> like I did in Safari (i.e. 
> https://bugs.webkit.org/show_bug.cgi?id=58608).

What other issues? Can you file a bug?

We also need tests here.

[cavalcantii1, 2017-06-08 18:09:49]

When I fixed Safari (as also Mozilla Servo), I noticed that the spec could be a
bit clearer on how to  handle border groove/ridge + color black.

For a discussion on Groove/Ridge issues, please see my message to w3c css
mailing list:
https://lists.w3.org/Archives/Public/www-style/2015Sep/0106.html

It was later decided by the CSS WG that the spec would be amended to clarify the
issue.

Concerning Blink, there is already an issue for it at:
https://bugs.chromium.org/p/chromium/issues/detail?id=436990

As also a patch I've submitted at the time:
https://codereview.chromium.org/759373002/

Main issue at the time is that this would require to rebase around 800 layout
tests IIRC.

[cavalcantii1, 2017-06-08 18:19:10]

As it was decided in the w3c css wg conf call, there would be an amend on level
3 of the spec:
https://lists.w3.org/Archives/Public/www-style/2015Sep/0252.html

Quote: "RESOLVED: Clarify in level 3 you can't make it solid black."

I see they added a note concerning the case of colors close to black in level 3
of the spec, quote:
"Note: Border colors close to black or white may need different color
calculations than colors in between in order to create the required “3D” effect
of ‘groove’, ‘ridge’, ‘inset’, or ‘outset’."

[cavalcantii1, 2017-06-08 18:37:58]

I would be willing to rebase/port the fix on the issue if there is some level of
support (e.g. people to review the patch, advice on the best way to handle the
800-ish tests).

That would be nice to close the full circle: 
a) Fix affected web engines: WebKit (Done), Servo (Done), Blink (Pending)
b) Fix the spec (Done).

:-)

[mstensho (USE GERRIT), 2017-06-09 07:42:23]

On 2017/06/08 18:37:58, cavalcantii1 wrote:
> I would be willing to rebase/port the fix on the issue if there is some level
of
> support (e.g. people to review the patch, advice on the best way to handle the
> 800-ish tests).
> 
> That would be nice to close the full circle: 
> a) Fix affected web engines: WebKit (Done), Servo (Done), Blink (Pending)
> b) Fix the spec (Done).
> 
> :-)

Sorry - I still don't understand the conceptual difference between ridge/groove
and inset/outset here. You need to come up with two colors slightly different
from border-color, regardless of which one of those four styles you're dealing
with, no? They should have the exact same problems and challenges with black or
any other color, as far as I understand? Ignoring "color" if "border-color"
isn't specified and falling back to #eeeeee should be equally wrong for all 4
styles.

"Cramming" it all together into one CL therefore makes sense to me.

Rebaselining 800 tests is fine, as long as the change is correct. :) Use
"Tools/Scripts/webkit-patch rebaseline-cl" to rebaseline.