720531 - Implement border-image-repeat space keyword

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Lazar Sumar [:nonsensickle]]

The current version of the spec, found at 
  http://dev.w3.org/csswg/css3-background/#the-border-image-repeat,
adds the 'space' keyword to the border-image-repeat property.

border-image properties were implemented in bug 497995 which should have landed. The spec is now CR. This bug might/might not be dependent on bug 713643.

Comment 1

[Lazar Sumar [:nonsensickle]]

This implementation will be similar to implementation of the background-repeat property in bug 548372.

Comment 2

[Lazar Sumar [:nonsensickle]]

Added dependence on bug 548372 because it will implement most of the required functionality so fixing this will be easier after it lands.

Comment 4

[yisibl]

Safari 9.3 supported: https://developer.apple.com/library/prerelease/mac/releasenotes/General/WhatsNewInSafari/Articles/Safari_9_1.html

Comment 5

[yisibl]

I updated the MDN document.

https://developer.mozilla.org/en-US/docs/Web/CSS/border-image-repeat

Comment 6

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

Is this easy following bug 548372?

Comment 7

[Ethan Lin[:ethlin]]

I'll try.

Comment 8

[Ethan Lin[:ethlin]]

Attachment: Part 1. parser changes.

Add space property to border image.

Comment 9

[Ethan Lin[:ethlin]]

Attachment: Part 2. rendering of border-image-repeat: space

Handle the rendering of border-image-repeat: space. Basically I reused the rendering of background-repeat, besides the calculation of space. The spec says "The gap before the first tile, the gap after the last tile, and the gaps between tiles are equalized".

Comment 10

[Ethan Lin[:ethlin]]

Attachment: Part 3. Add test case for border-image-repeat: space

Add test cases for border-image-repeat: space. There are 2 test cases. One is the size of border image larger than div size, the other one is reverse.

Comment 11

[Ethan Lin[:ethlin]]

Attachment: Part 4. Add 'space' for border in shorthand property test

Add the keyword 'space' for border.

Comment 12

[Ethan Lin[:ethlin]]

Attachment: Part 3. Add test case for border-image-repeat: space

Add one more test case for the case there are multiple border images on each side.

Comment 13

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

Comment on attachment 8764528 [details] [diff] [review]
Part 2. rendering of border-image-repeat: space

>+  int32_t count = NSToIntFloor(aAvailableSpace / aImageDimension);

You're dividing two integers here.  There's no need to convert to a float
and call NSToIntFloor in order to floor it; that already happens.


Why do you need to add a Clip and PopClip in DrawBorderImage?

Also, where do you handle the rule that, for 'space', if one image
doesn't fit, then no images at all are drawn?


In ComputeTile, you should introduce {} inside the cases for
NS_STYLE_BORDER_IMAGE_REPEAT_SPACE and make |space| a local variable
inside the {}.

>+    CSSIntSize imageSize(nsPresContext::AppUnitsToIntCSSPixels(srcRect.width),
>+                         nsPresContext::AppUnitsToIntCSSPixels(srcRect.height));

Can this introduce rounding error?


Can you explain how DrawBackgroundImage differs from DrawImage?  Is it
more expensive?  Is it true that in the non-'space' cases it should
yield the same behavior, other than the change in sampling filter, and
the change in ExtendMode (from CLAMP to REPEAT)?  What are the
implications of that change in sampling filter, and the change in
ExtendMode?

>+  if (repeatSize.width == 0 || repeatSize.height == 0) {
>+    printf_stderr("@@@Got Zero!!!\n");
>+  }

Please don't leave printfs in patches for review.


Marking review- since I'd like to understand the answers to the questions above.

Comment 14

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

Comment on attachment 8764535 [details] [diff] [review]
Part 3. Add test case for border-image-repeat: space

The link rel="help" for all of the tests points to background-repeat
when it should point to border-image-repeat.

The meta name="assert" could also add:
 for 1 - "when a single image fits"
 for 2 - "when no images fit"
 for 3 - "when multiple images fit"
or something like that, to explain how the tests differ.

It would probably be good to also have a test where a single image fits exactly, and where multiple images fit exactly.  (You could test those both against similar references to these, and against other values of 'border-image-repeat'.)

It also seems like at least some, if not all, of the tests should use
'fill' so that you test the middle piece as well.

I also suspect test 2 is wrong, since I think the spec says that
no images should be drawn if there isn't room for one.  (What do
other browsers do?  It's possible the spec should be changed instead
of our code.)

Comment 15

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

Comment on attachment 8764525 [details] [diff] [review]
Part 1. parser changes.

Actually, though, this patch should also have a change to add values with 'space' to property_database.js, probably both for the longhand and the shorthand.

Comment 16

[Ethan Lin[:ethlin]]

(In reply to David Baron :dbaron: ⌚️UTC-4 (review requests must explain patch) from comment #15)
> Comment on attachment 8764525 [details] [diff] [review]
> Part 1. parser changes.
> 
> Actually, though, this patch should also have a change to add values with
> 'space' to property_database.js, probably both for the longhand and the
> shorthand.

Okay, I will add the value to property_database.js.

Comment 17

[Ethan Lin[:ethlin]]

(In reply to David Baron :dbaron: ⌚️UTC-4 (review requests must explain patch) from comment #14)
> Comment on attachment 8764535 [details] [diff] [review]
> Part 3. Add test case for border-image-repeat: space
> 
> The link rel="help" for all of the tests points to background-repeat
> when it should point to border-image-repeat.
> 
> The meta name="assert" could also add:
>  for 1 - "when a single image fits"
>  for 2 - "when no images fit"
>  for 3 - "when multiple images fit"
> or something like that, to explain how the tests differ.
>
Okay. 

> It would probably be good to also have a test where a single image fits
> exactly, and where multiple images fit exactly.  (You could test those both
> against similar references to these, and against other values of
> 'border-image-repeat'.)
> 
> It also seems like at least some, if not all, of the tests should use
> 'fill' so that you test the middle piece as well.
>

I will add more tests in the next patch.
 
> I also suspect test 2 is wrong, since I think the spec says that
> no images should be drawn if there isn't room for one.  (What do
> other browsers do?  It's possible the spec should be changed instead
> of our code.)
I think I implemented wrong here. By [1], safari and IE support the 'space' value. I just tried safari and the image wasn't drawn when there is no room. I will fix it.  

[1] https://developer.mozilla.org/zh-TW/docs/Web/CSS/border-image-repeat

Comment 18

[Ethan Lin[:ethlin]]

(In reply to David Baron :dbaron: ⌚️UTC-4 (away July 7-10) (review requests must explain patch) from comment #13)
> Comment on attachment 8764528 [details] [diff] [review]
> Part 2. rendering of border-image-repeat: space
> 
> >+  int32_t count = NSToIntFloor(aAvailableSpace / aImageDimension);
> 
> You're dividing two integers here.  There's no need to convert to a float
> and call NSToIntFloor in order to floor it; that already happens.
> 
> 
> Why do you need to add a Clip and PopClip in DrawBorderImage?
> 

When calculating the drawing area[1], the rendering position may be out of the destination. I need to add the clip area for the each border since the original clip area is larger.

[1] https://hg.mozilla.org/mozilla-central/annotate/23dc78b7b57e9f91798ea44c242a04e112c37db0/layout/base/nsLayoutUtils.cpp#l6957

> Also, where do you handle the rule that, for 'space', if one image
> doesn't fit, then no images at all are drawn?
> 

I didn't. I will handle it.

> 
> In ComputeTile, you should introduce {} inside the cases for
> NS_STYLE_BORDER_IMAGE_REPEAT_SPACE and make |space| a local variable
> inside the {}.

Okay.

> 
> >+    CSSIntSize imageSize(nsPresContext::AppUnitsToIntCSSPixels(srcRect.width),
> >+                         nsPresContext::AppUnitsToIntCSSPixels(srcRect.height));
> 
> Can this introduce rounding error?
> 

Background does this too[2]. The parameter is just for svg context in DrawBackgroundImage, so I think it's fine here.
[2] https://hg.mozilla.org/mozilla-central/annotate/23dc78b7b57e9f91798ea44c242a04e112c37db0/layout/base/nsCSSRendering.cpp#l5298

> 
> Can you explain how DrawBackgroundImage differs from DrawImage?  Is it
> more expensive?  Is it true that in the non-'space' cases it should
> yield the same behavior, other than the change in sampling filter, and
> the change in ExtendMode (from CLAMP to REPEAT)?  What are the
> implications of that change in sampling filter, and the change in
> ExtendMode?

Two functions will call DrawImageInternal in the end. In my view, DrawBackgroundImage additionally handles svg image and repeat size(for space). So yes, for border image non-space cases, they should have almost the same behavior.
About the sampling filter, the DrawBackgroundImage considers the flag 'gfx.filter.nearest.force-enabled'. Without turning on the flag, there is no difference.
About ExtendMode, the CLAMP will turn to REPEAT if the FillRect larger than DestRect [3]. I should set ExtendMode as CLAMP when calling DrawBackgroundImage since Fill and Dest may equal for other parameters.

[3] https://hg.mozilla.org/mozilla-central/annotate/23dc78b7b57e9f91798ea44c242a04e112c37db0/layout/base/nsLayoutUtils.cpp#l6673

> 
> >+  if (repeatSize.width == 0 || repeatSize.height == 0) {
> >+    printf_stderr("@@@Got Zero!!!\n");
> >+  }
> 
> Please don't leave printfs in patches for review.
> 
Sorry. I forget to remove it!
> 
> Marking review- since I'd like to understand the answers to the questions
> above.

Comment 19

[Ethan Lin[:ethlin]]

Attachment: Part 1. parser changes.

Add 'space' keyword in property_database.js.

Comment 20

[Ethan Lin[:ethlin]]

Attachment: Part 3. Add test case for border-image-repeat: space

Add 2 reftests for exactly fitting cases.

Comment 21

[Ethan Lin[:ethlin]]

Attachment: Part 2. rendering of border-image-repeat: space

Address dbaron's comments.

Comment 22

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

Did you mean to ask for re-review of part 2 rather than part 1, or both?  (I marked the original part 2 as review-.)

Comment 23

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

Comment on attachment 8769587 [details] [diff] [review]
Part 1. parser changes.

In property_database.js, please also add at least one value using "space" to the list of other_values for 'border-image'.

Comment 24

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

Comment on attachment 8769589 [details] [diff] [review]
Part 3. Add test case for border-image-repeat: space

Please fix the link rel="help" as I requested in comment 14.

Please also use 'fill' in some of the tests as requested in comment 14.

The use of link rel="match" in tests 4 and 5 is incorrect.  Per
https://wiki.csswg.org/test/reftest#the-reftest-comparison-links or
http://testthewebforward.org/docs/reftests.html#matching-multiple-references
multiple rel="match" links mean that one or the other must match.  You
should instead use chained rel="match" by making the first reference
link to the second.

Comment 25

[Ethan Lin[:ethlin]]

Comment on attachment 8769606 [details] [diff] [review]
Part 2. rendering of border-image-repeat: space

I just wanted to check this patch again. I set the review? flag now.

Comment 26

[Ethan Lin[:ethlin]]

Comment on attachment 8769606 [details] [diff] [review]
Part 2. rendering of border-image-repeat: space

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

::: layout/base/nsCSSRendering.cpp
@@ +3375,5 @@
> +                              nscoord& aSpace)
> +{
> +  int32_t count = aAvailableSpace / aImageDimension;
> +  aSpace = (aAvailableSpace - aImageDimension * count) / (count + 1);
> +  return aSpace + aImageDimension;

If one image doesn't fit, the 'aSpace' will be aAvailableSpace. Which means no image will be drawn since the position of the first image is same as 'aSpace'.

Comment 27

[Ethan Lin[:ethlin]]

(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #24)
> Comment on attachment 8769589 [details] [diff] [review]
> Part 3. Add test case for border-image-repeat: space
> 
> Please fix the link rel="help" as I requested in comment 14.
> 

Sorry, I will fix it.

> Please also use 'fill' in some of the tests as requested in comment 14.
> 

Okay. 

> The use of link rel="match" in tests 4 and 5 is incorrect.  Per
> https://wiki.csswg.org/test/reftest#the-reftest-comparison-links or
> http://testthewebforward.org/docs/reftests.html#matching-multiple-references
> multiple rel="match" links mean that one or the other must match.  You
> should instead use chained rel="match" by making the first reference
> link to the second.

I see. So I should create a reference chain 'a->b->a' first. Then the test html just need one rel="match" to either a or b.

Comment 28

[Ethan Lin[:ethlin]]

(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #23)
> Comment on attachment 8769587 [details] [diff] [review]
> Part 1. parser changes.
> 
> In property_database.js, please also add at least one value using "space" to
> the list of other_values for 'border-image'.

Okay, I will add it.

Comment 29

[Ethan Lin[:ethlin]]

Attachment: Part 1. parser changes.

Add 'space' to other-value in 'border-image'.

Comment 30

[Ethan Lin[:ethlin]]

Attachment: Part 3. Add test case for border-image-repeat: space

I fixed the 'help' link and created chain reference for test 4 and test 5. I also add test 6 and tes7 for the value 'fill'.

Comment 31

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

Comment on attachment 8769606 [details] [diff] [review]
Part 2. rendering of border-image-repeat: space

(In reply to Ethan Lin[:ethlin] from comment #18)
> > Why do you need to add a Clip and PopClip in DrawBorderImage?
> > 
> 
> When calculating the drawing area[1], the rendering position may be out of
> the destination. I need to add the clip area for the each border since the
> original clip area is larger.
> 
> [1]
> https://hg.mozilla.org/mozilla-central/annotate/
> 23dc78b7b57e9f91798ea44c242a04e112c37db0/layout/base/nsLayoutUtils.cpp#l6957

I still don't understand this.  What is it that needs to be clipped?

(I thought that you needed it because you weren't correctly handling the case where you should have been drawing no image.)

> About the sampling filter, the DrawBackgroundImage considers the flag
> 'gfx.filter.nearest.force-enabled'. Without turning on the flag, there is no
> difference.

OK, I filed bug 1287011 to remove this preference, which I think we don't need anymore.


Also, the calls to ComputeBorderSpacedRepeatSize should have a break after the = to avoid going past the 80th column.

Comment 32

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

Comment on attachment 8770396 [details] [diff] [review]
Part 3. Add test case for border-image-repeat: space

border-image-repeat-space-{4,5}-ref-2 doesn't need a link rel="match",
since ref-1 already links to ref-2.

r=dbaron with that fixed

Comment 33

[Ethan Lin[:ethlin]]

(In reply to David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) from comment #31)
> Comment on attachment 8769606 [details] [diff] [review]
> Part 2. rendering of border-image-repeat: space
> 
> (In reply to Ethan Lin[:ethlin] from comment #18)
> > > Why do you need to add a Clip and PopClip in DrawBorderImage?
> > > 
> > 
> > When calculating the drawing area[1], the rendering position may be out of
> > the destination. I need to add the clip area for the each border since the
> > original clip area is larger.
> > 
> > [1]
> > https://hg.mozilla.org/mozilla-central/annotate/
> > 23dc78b7b57e9f91798ea44c242a04e112c37db0/layout/base/nsLayoutUtils.cpp#l6957
> 
> I still don't understand this.  What is it that needs to be clipped?
> 
> (I thought that you needed it because you weren't correctly handling the
> case where you should have been drawing no image.)
> 

The clip is not for handling no image case. The calculation [1] of 'firstTilePos' in DrawBackgroundImage was for background image with border. If the top-left point of 'aFill' and 'aDest' is different, [1] will calculate the correct start position to render. The test case is [2]. In this case, the border size is larger, the 'firstTilePos' is out of the 'aFill' to fill the border area. We can see the image around the border is clipped.
For border image, what we want to render should be all inside the 'aFill', but the clip area is larger than it. The start point of 'aFill' and 'aDest' is different since there is a gap between them. (It's a little bit different from background image.) And then [1] will calculate the a 'firstTilePos' which is out of 'aFill'. That's why I set clip for the border image.
I think there is an alternative way to handle it. If I cut the begin and end space area from 'aFill', then the 'firstTilePos' will be same as the top-left point of 'aFill'. And the rendering won't be over the 'aFill' area.


[1] https://hg.mozilla.org/mozilla-central/annotate/23dc78b7b57e9f91798ea44c242a04e112c37db0/layout/base/nsLayoutUtils.cpp#l6957
[2] https://dxr.mozilla.org/mozilla-central/source/layout/reftests/w3c-css/submitted/background/background-repeat-space-8.html?q=background-repeat-space-8.html&redirect_type=direct

> > About the sampling filter, the DrawBackgroundImage considers the flag
> > 'gfx.filter.nearest.force-enabled'. Without turning on the flag, there is no
> > difference.
> 
> OK, I filed bug 1287011 to remove this preference, which I think we don't
> need anymore.
> 
> 
> Also, the calls to ComputeBorderSpacedRepeatSize should have a break after
> the = to avoid going past the 80th column.

Okay.

Comment 34

[Ethan Lin[:ethlin]]

Attachment: Part 2. rendering of border-image-repeat: space

In this patch, I set a fit fill rect for border image with space parameter. Then we don't need to use clip to handle the over line rendering problem.

Comment 35

[Ethan Lin[:ethlin]]

Attachment: Part 3. Add test case for border-image-repeat: space (carry r+: dbaron)

Address dbaron's comment. Remove link rel="match" in the border-image-repeat-space-{4,5}-ref-2.

Comment 36

[Ethan Lin[:ethlin]]

try server:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=411909ff630e&selectedJob=24278030

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9760f28f2343
Part 1. Implement space of border-image-repeat CSS property. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2b5441aa9fe
Part 2. Implement rendering of border-image-repeat: space. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/45b4eb6943c8
Part 3. Add reftests for border-image-repeat: space. r=dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/2036366e1ee1
Part 4. Add space to border for shorthand property test. r=dbaron

Comment 38

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9760f28f2343
https://hg.mozilla.org/mozilla-central/rev/d2b5441aa9fe
https://hg.mozilla.org/mozilla-central/rev/45b4eb6943c8
https://hg.mozilla.org/mozilla-central/rev/2036366e1ee1

Comment 39

[Sebastian Zartner [:sebo]]

Updated compatibility info at https://developer.mozilla.org/en-US/docs/Web/CSS/border-image-repeat and added a note to https://developer.mozilla.org/en-US/Firefox/Releases/50#CSS.

Sebastian

Comment 40

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/c237fc190702ce0f252dfc8d3cd0aad61060e537
Bug 720531 followup - Modify reftest.list so that it matches the link rel=match in the tests.  No review.

Comment 41

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/c237fc190702