Issue 1457323004: Don't render border images when border-style is none

Issue 1457323004: Don't render border images when border-style is none (Closed)

Created:
5 years, 1 month ago by Savago

Modified:
4 years, 9 months ago

Reviewers:
Timothy Loh

CC:
chromium-reviews, blink-reviews, blink-reviews-style_chromium.org, cavalcantii, jchaffraix+rendering, pdr.

Base URL:
https://chromium.googlesource.com/chromium/src.git@master

Target Ref:
refs/pending/heads/master

Project:
chromium

Visibility:
Public.

More Reviews

Description

Blink is non compliant to the css-backgrounds spec where a border with an image should not be rendered if no border style is defined. As this patch ensures that Blink will behave like FF and IE, previous border image tests had to be edited to ensure that border will be rendered. Relevant spec: http://www.w3.org/TR/css3-background/#border-images W3C test case: http://test.csswg.org/suites/css-backgrounds-3_dev/nightly-unstable/html4/border-image-slice-003.htm BUG=559258

Patch Set 1 #

Total comments: 6

Patch Set 2 : Addressing reviewer's requests. #

Patch Set 3 : Edits to more tests. #

Total comments: 4

Patch Set 4 : Adding rebaselines, changes on tests. #

Patch Set 5 : Fixed a few tests. #

Patch Set 6 : Last relevant test. #

Patch Set 7 : Rebased, removed unnecessary border colors, etc. #

Created: 5 years ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+121 lines, -14 lines)			Patch
M	third_party/WebKit/LayoutTests/TestExpectations	View	1 2 3 4 5 6	1 chunk	+34 lines, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/css3/images/pixelated-border-image.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/borders/border-image-01.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/borders/border-image-border-radius.html	View	1 2 3 4 5 6	2 chunks	+2 lines, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/borders/border-image-longhand.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/borders/border-image-massive-scale.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/borders/border-image-omit-right-slice.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/borders/border-image-outset.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/borders/border-image-outset-in-shorthand.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/borders/border-image-outset-split-inline.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/borders/border-image-outset-split-inline-vertical-lr.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/borders/border-image-repeat.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/borders/border-image-repeat-round.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/borders/border-image-rotate-transform.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/borders/border-image-scale-transform.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/borders/border-image-scaled.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/borders/border-image-scrambled.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/fast/borders/border-image-should-not-display.html	View	1 2 3	1 chunk	+21 lines, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/fast/borders/border-image-should-not-display-expected.html	View	1 2 3	1 chunk	+15 lines, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/borders/border-image-side-reduction.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/borders/border-image-slice-constrained.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/borders/border-image-slices.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/borders/border-image-source.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/borders/border-image-trumps-radius.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
A	third_party/WebKit/LayoutTests/fast/borders/resources/red.png	View		Binary file		0 comments	Download
M	third_party/WebKit/LayoutTests/fast/borders/scaled-border-image.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/css/border-image-style-none.html	View	1 2 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/css/border-image-style-none-expected.txt	View	1 2 3 4 5	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/gradients/border-image-gradient.html	View	1 2 3 4 5 6	1 chunk	+2 lines, -1 line	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/hidpi/image-set-border-image-comparison.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/hidpi/image-set-border-image-comparison-expected.html	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/images/webp-flip.html	View	1 2 3 4 5 6	1 chunk	+1 line, -1 line	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/repaint/border-outline-0.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/writing-mode/border-image-vertical-lr.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/fast/writing-mode/border-image-vertical-rl.html	View	1 2 3 4 5 6	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/inspector/tracing/decode-resize.html	View	1 2 3 4	1 chunk	+1 line, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/scrollbars/listbox-scrollbar-combinations.html	View	1 2 3 4 5 6	2 chunks	+2 lines, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/svg/as-border-image/svg-as-border-image.html	View	1 2 3 4 5 6	1 chunk	+3 lines, -0 lines	0 comments	Download
M	third_party/WebKit/LayoutTests/svg/as-border-image/svg-as-border-image-2.html	View	1 2 3 4 5 6	1 chunk	+3 lines, -0 lines	0 comments	Download
M	third_party/WebKit/Source/core/style/BorderData.h	View	1	2 chunks	+5 lines, -6 lines	0 comments	Download
M	third_party/WebKit/Source/core/style/BorderValue.h	View		2 chunks	+4 lines, -4 lines	0 comments	Download

Messages

Total messages: 16 (2 generated)

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages

Savago

I'm assuming that to have bots generating new rebaselines the only thing I need to ...

5 years, 1 month ago (2015-11-20 19:59:31 UTC) #4

Timothy Loh

IMO this could do with a better patch title, for example "Don't render border images ...

5 years, 1 month ago (2015-11-23 06:23:19 UTC) #5

Savago

Timothy Thanks for having a look on this, I will comment inline. > IMO this ...

5 years, 1 month ago (2015-11-23 18:52:01 UTC) #6

Timothy

Thanks for having a look on this, I will comment inline.

> IMO this could do with a better patch title, for example "Don't render border
> images when border-style is none".
> 

Sure thing, I can change it. Just keep in mind this is about spec compliance.
What about "Don't render border images unless a border is defined"?

> It doesn't look like you should need to use NeedsRebaseline anywhere. This is
> only necessary for pixel tests where the image changes, but since you're
adding
> a border style they shouldn't be changing.
> 

The text expectations will change, as an example:
https://gist.github.com/Adenilson/53e2f4c16dcd4f3c397d

> Please run git cl try after uploading a patch with the suggested fixes, this
> will run all the try bots so it is easy to see whether the tests are passing.
> 

I didn't initially to spare bots and wait for feedback (which is why I commented
that this is not for commit).

Next patch set will include all bots for sure.


> Do we need to add the border-color declarations everywhere?
> 
Border style solid + white color yielded the best results (i.e. keep the
previous tests looking the same or really close). Border hidden will be just
ignored (thus not rendering anything) and the default border color (i.e. dark)
will create artifacts in some tests.

> <style>
> <!doctype html> is preferred
> 

Only 1/3 of the tests on the same folder have it (57/163), finally quoting the
HTML5 standard "DOCTYPEs are required for legacy reasons.".

I followed the same idea when comes to the 'body' tag element, again quoting the
standard: "An html element's start tag can be omitted if the first thing inside
the html element is not a comment.". Reference:
http://www.w3.org/TR/html5/semantics.html#the-html-element

> CSS comments are written with /* .. */ syntax.

I stand corrected (my editor tricked me on this one). Will submit fix.



> Since you only have one div, just put all the styling in the div { .. } and
> don't use an id.
> no need for the prefixed property

Coolio, will do both edits.

> third_party/WebKit/Source/core/style/BorderData.h:86: if (!m_image.hasImage()
&&
> (m_top.style() == BNONE || m_top.style() == BHIDDEN))
> you missed this one :S

Nice catch!

Savago

For reference, running fast/borders tests locally, I got: Regressions: Unexpected text-only failures (19) fast/borders/border-image-01.html [ ...

5 years, 1 month ago (2015-11-23 20:26:18 UTC) #8

Timothy Loh

On 2015/11/23 18:52:01, Savago wrote: > Timothy > > Thanks for having a look on ...

5 years, 1 month ago (2015-11-24 00:09:48 UTC) #9

On 2015/11/23 18:52:01, Savago wrote:
> Timothy
> 
> Thanks for having a look on this, I will comment inline.
> 
> > IMO this could do with a better patch title, for example "Don't render
border
> > images when border-style is none".
> > 
> 
> Sure thing, I can change it. Just keep in mind this is about spec compliance.
> What about "Don't render border images unless a border is defined"?

I don't think that's clearer, the spec doesn't really talk about defining a
border. This patch also affects if you have a border-image specified and also
explicitly set border-style to none.

BTW, we generally prefer links to editor's drafts in patch descriptions (i.e.
https://drafts.csswg.org/css-backgrounds-3/), and the first line of the patch
description should match the patch title.

> > It doesn't look like you should need to use NeedsRebaseline anywhere. This
is
> > only necessary for pixel tests where the image changes, but since you're
> adding
> > a border style they shouldn't be changing.
> > 
> 
> The text expectations will change, as an example:
> https://gist.github.com/Adenilson/53e2f4c16dcd4f3c397d
> 
> > Please run git cl try after uploading a patch with the suggested fixes, this
> > will run all the try bots so it is easy to see whether the tests are
passing.
> > 
> 
> I didn't initially to spare bots and wait for feedback (which is why I
commented
> that this is not for commit).
> 
> Next patch set will include all bots for sure.

Yeah, just add NeedsRebaseline for the changed tests (these should only be
changes to the layout tree dump, there shouldn't be any pixel differences).

> > Do we need to add the border-color declarations everywhere?
> > 
> Border style solid + white color yielded the best results (i.e. keep the
> previous tests looking the same or really close). Border hidden will be just
> ignored (thus not rendering anything) and the default border color (i.e. dark)
> will create artifacts in some tests.

But aren't we only adding these for borders which already have a border-image?
In these cases, the border-color shouldn't matter.

> > <style>
> > <!doctype html> is preferred
> > 
> 
> Only 1/3 of the tests on the same folder have it (57/163), finally quoting the
> HTML5 standard "DOCTYPEs are required for legacy reasons.".
> 
> I followed the same idea when comes to the 'body' tag element, again quoting
the
> standard: "An html element's start tag can be omitted if the first thing
inside
> the html element is not a comment.". Reference:
> http://www.w3.org/TR/html5/semantics.html#the-html-element

In particular, *DOCTYPEs are required*. This makes it obvious that we aren't
trying to test quirks-mode behaviour.

Timothy Loh

5 years, 1 month ago (2015-11-24 00:10:00 UTC) #10

Savago

https://codereview.chromium.org/1457323004/diff/40001/third_party/WebKit/LayoutTests/fast/borders/border-image-omit-right-slice.html File third_party/WebKit/LayoutTests/fast/borders/border-image-omit-right-slice.html (right): https://codereview.chromium.org/1457323004/diff/40001/third_party/WebKit/LayoutTests/fast/borders/border-image-omit-right-slice.html#newcode13 third_party/WebKit/LayoutTests/fast/borders/border-image-omit-right-slice.html:13: border-style: solid; Coolio. https://codereview.chromium.org/1457323004/diff/40001/third_party/WebKit/LayoutTests/scrollbars/listbox-scrollbar-combinations.html File third_party/WebKit/LayoutTests/scrollbars/listbox-scrollbar-combinations.html (right): https://codereview.chromium.org/1457323004/diff/40001/third_party/WebKit/LayoutTests/scrollbars/listbox-scrollbar-combinations.html#newcode290 third_party/WebKit/LayoutTests/scrollbars/listbox-scrollbar-combinations.html:290: ...

5 years, 1 month ago (2015-11-24 00:42:52 UTC) #11

davve

The description mentions that this patch aligns Blink with IE. This matches my testing too. ...

5 years ago (2015-11-28 13:53:00 UTC) #12

Savago

Hi there, David Thanks for the input, I've tested on IE and not on Edge. ...

5 years ago (2015-11-28 18:03:39 UTC) #13

Savago

Touching the base on this, it still rebases cleanly and I feel that all pending ...

5 years ago (2015-12-01 17:40:19 UTC) #15

cavalcantii1

4 years, 10 months ago (2016-02-03 23:33:48 UTC) #16

I'm closing this as I no longer have access to the @samsung account.

Development moved to:
https://codereview.chromium.org/1652813004/

Expand Messages | Collapse Messages | Show Generated Messages | Hide Generated Messages