889964 - "border-image-repeat: round" uses ceil instead of round in tile width calculation

Status: RESOLVED FIXED

(Core :: Web Painting, defect)

Description

[Jeremy Roman (Chromium)]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/28.0.1500.52 Safari/537.36

Steps to reproduce:

Launch Firefox (I used Nightly) and open a document in there is a border image with the 'round' repeat style, and the number of tiles which fit in the available length of a border is between k and k+1/2 for some integer k.

A trivial such example is
data:text/html,<div style="border:27px solid transparent; border-image:url(http://www.w3.org/TR/css3-background/border.png) 27 27 round; width:28px"></div>


Actual results:

One extra copy of the tile is rendered, and all tiles are rescaled to fit that extra copy.

In the example above, two narrow yellow diamonds are rendered instead of a single yellow diamond in each of the top and bottom borders (for a total of 8 diamonds visible).


Expected results:

The number of tiles to be rendered should conform to the specification:

"If X ≠ 0 is the width of the image after step one and W is the width of the background positioning area, then the rounded width X' = W / round(W / X) where round() is a function that returns the nearest natural number (integer greater than zero)." (http://www.w3.org/TR/css3-background/).

This formula applies to the border as well as background since the spec for border-image-repeat says "If the first keyword is ‘round’, the top, middle and bottom images are resized in width, so that exactly a whole number of them fit in the middle part of the border-image area, exactly as for ‘round’ in the ‘background-repeat’ property.".

In example given above, it is expected that one yellow diamond appear on each of the top and bottom edges of the border (for a total of 6 diamonds visible).

This appears to be a result of using "ceil" in the DrawBorderImageComponent in layout/base/nsCSSRendering.cpp, when what is desired is "round" (restricted to positive integers to avoid division by zero).

Comment 1

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The code has now moved to the ComputeTile helper function just above DrawBorderImageComponent.

:ethlin, any chance you could fix this one as well given that you're in the area?  (It looks like this will make us match Chromium on the testcase; I didn't test Edge, although it's probably worth checking.)

Comment 2

[Ethan Lin[:ethlin]]

Sure.

Comment 3

[Ethan Lin[:ethlin]]

Attachment: Part 1. Fix round calculation.

The round calculation of border-image-repeat is wrong. I applied the background-repeat calculation to fix it. I will write testcase later.

Comment 4

[Jeremy Roman (Chromium)]

Comment on attachment 8758171 [details] [diff] [review]
Part 1. Fix round calculation.

Review of attachment 8758171 [details] [diff] [review]:
-----------------------------------------------------------------

I hope you don't mind the drive-by comment, but it seems like this should have changes to both the horizontal and vertical cases.

::: layout/base/nsCSSRendering.cpp
@@ +5409,5 @@
>      tile.width = aUnitSize.width;
>      break;
>    case NS_STYLE_BORDER_IMAGE_REPEAT_ROUND:
>      tile.x = aFill.x;
> +    tile.width = ComputeRoundedSize(aUnitSize.width, aFill.width);

There's a similar computation for tile.height a few lines down that presumably needs the same change.

Comment 5

[Ethan Lin[:ethlin]]

(In reply to jbroman from comment #4)
> Comment on attachment 8758171 [details] [diff] [review]
> Part 1. Fix round calculation.
> 
> Review of attachment 8758171 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I hope you don't mind the drive-by comment, but it seems like this should
> have changes to both the horizontal and vertical cases.
> 
> ::: layout/base/nsCSSRendering.cpp
> @@ +5409,5 @@
> >      tile.width = aUnitSize.width;
> >      break;
> >    case NS_STYLE_BORDER_IMAGE_REPEAT_ROUND:
> >      tile.x = aFill.x;
> > +    tile.width = ComputeRoundedSize(aUnitSize.width, aFill.width);
> 
> There's a similar computation for tile.height a few lines down that
> presumably needs the same change.

Okay. Thanks.

Comment 6

[Ethan Lin[:ethlin]]

Attachment: Part 1. Fix round calculation.

I applied the background-repeat calculation to border-image's round calculations.

Comment 7

[Ethan Lin[:ethlin]]

Attachment: Part 2. Add reftest for border-image-repeat: round

I added a testcase for border-image-repeat: round for this problem.

Comment 8

[Ethan Lin[:ethlin]]

Attachment: Part 2. Add reftest for border-image-repeat: round

Remove redundant settings.

Comment 9

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8758547 [details] [diff] [review]
Part 1. Fix round calculation.

># HG changeset patch
># User Ethan Lin <[e-mail]>
>
>Bug 889964 - Part 1. Fix the caluculation of boarder image: round. r=dbaron

Please fix the spelling of "calculation" (remove the first "u") and "border-image" (no "a", with "-")

r=dbaron with that

Comment 10

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8758556 [details] [diff] [review]
Part 2. Add reftest for border-image-repeat: round

>+    <meta name="assert" content="The test checks whether border-image-repeat: 'round' works correctly or not.">

"works correctly or not" should be clearer.  Perhaps "uses the correct rounding formula"?

Does the test fail without the patch?  And does it test the <1 case (in particular, with something that's less than 0.5)?  If not, it should (for both questions).


Please put the test in the background/ directory instead of creating a new border-image/ directory.

(Note that for the creation of the border-image subdirectory to be correct, you would have needed to add to the parent directory's reftest.list.  But you shouldn't do that.)

r=dbaron with that

Comment 11

[Ethan Lin[:ethlin]]

(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) (vacation June 4-12) from comment #10)
> Comment on attachment 8758556 [details] [diff] [review]
> Part 2. Add reftest for border-image-repeat: round
> 
> >+    <meta name="assert" content="The test checks whether border-image-repeat: 'round' works correctly or not.">
> 
> "works correctly or not" should be clearer.  Perhaps "uses the correct
> rounding formula"?
> 
Okay, I'll change that.

> Does the test fail without the patch?  And does it test the <1 case (in
> particular, with something that's less than 0.5)?  If not, it should (for
> both questions).
> 
The test will fail without the patch, but it doesn't test <1 case. I will add <1 case.
> 
> Please put the test in the background/ directory instead of creating a new
> border-image/ directory.
> 
Okay, I'll move it.

> (Note that for the creation of the border-image subdirectory to be correct,
> you would have needed to add to the parent directory's reftest.list.  But
> you shouldn't do that.)
> 
> r=dbaron with that

Comment 12

[Ethan Lin[:ethlin]]

Attachment: Part 1. Fix round calculation. (carry r+: dbaron)

Address :dbaron's comment.

Comment 13

[Ethan Lin[:ethlin]]

Attachment: Part 2. Add reftest for border-image-repeat: round

I added another test case for <1 and moved the tests to background folder.

Comment 14

[Ethan Lin[:ethlin]]

Attachment: Part 3. Turn on background tests

I forget to include the reftest.list in bug 548372. Turn on it so try server will run it.

Comment 15

[Ethan Lin[:ethlin]]

try server result:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=63aa99990f35&selectedJob=21919213

Comment 16

[Ethan Lin[:ethlin]]

Attachment: Part 1. Fix round calculation. (carry r+: dbaron)

It was a wrong patch. Correct it.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 8759579 [details] [diff] [review]
Part 3. Turn on background tests

Though, actually, instead of adding a new line at the bottom, this should uncomment:

# include background3/reftest.list

and remove the "3". in both that line and the comment line above it.

Comment 18

[Ethan Lin[:ethlin]]

(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) (vacation June 4-12) from comment #17)
> Comment on attachment 8759579 [details] [diff] [review]
> Part 3. Turn on background tests
> 
> Though, actually, instead of adding a new line at the bottom, this should
> uncomment:
> 
> # include background3/reftest.list
> 
> and remove the "3". in both that line and the comment line above it.

Okay, I'll change it.

Comment 19

[Ethan Lin[:ethlin]]

Attachment: Part 3. Turn on background tests (r=dbaron)

Address :dbaron's comment.

Comment 20

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2ce152b78b405f545225ba5804366faa61d601bf
Bug 889964 - Part 1. Fix the calculation of border-image: round. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/0631776a11b102b0041c53b9f265d856f0b68108
Bug 889964 - Part 2. Add reftest for boarder image: round. r=dbaron

https://hg.mozilla.org/integration/mozilla-inbound/rev/3e6967898d93bc686b291a310b715f3371c19e08
Bug 889964 - Part 3. Turn on background reftest. r=dbaron

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/2ce152b78b40
https://hg.mozilla.org/mozilla-central/rev/0631776a11b1
https://hg.mozilla.org/mozilla-central/rev/3e6967898d93