Closed
Bug 720531
Opened 13 years ago
Closed 8 years ago
Implement border-image-repeat space keyword
Categories
(Core :: CSS Parsing and Computation, defect)
Core
CSS Parsing and Computation
Tracking
()
RESOLVED
FIXED
mozilla50
Tracking | Status | |
---|---|---|
firefox50 | --- | fixed |
People
(Reporter: nonsensickle, Assigned: ethlin)
References
(Blocks 2 open bugs)
Details
(Keywords: dev-doc-complete, Whiteboard: [parity-webkit])
Attachments
(4 files, 8 obsolete files)
2.14 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
4.38 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
8.68 KB,
patch
|
dbaron
:
review+
|
Details | Diff | Splinter Review |
44.29 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•13 years ago
|
||
This implementation will be similar to implementation of the background-repeat property in bug 548372.
Reporter | ||
Comment 2•13 years ago
|
||
Added dependence on bug 548372 because it will implement most of the required functionality so fixing this will be easier after it lands.
Safari 9.3 supported: https://developer.apple.com/library/prerelease/mac/releasenotes/General/WhatsNewInSafari/Articles/Safari_9_1.html
Updated•9 years ago
|
Keywords: dev-doc-needed
Whiteboard: [parity-webkit]
I updated the MDN document. https://developer.mozilla.org/en-US/docs/Web/CSS/border-image-repeat
Is this easy following bug 548372?
Flags: needinfo?(ethlin)
Assignee | ||
Comment 8•8 years ago
|
||
Add space property to border image.
Attachment #8764525 -
Flags: review?(dbaron)
Assignee | ||
Comment 9•8 years ago
|
||
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".
Attachment #8764528 -
Flags: review?(dbaron)
Assignee | ||
Comment 10•8 years ago
|
||
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.
Attachment #8764530 -
Flags: review?(dbaron)
Assignee | ||
Comment 11•8 years ago
|
||
Add the keyword 'space' for border.
Attachment #8764533 -
Flags: review?(dbaron)
Assignee | ||
Comment 12•8 years ago
|
||
Add one more test case for the case there are multiple border images on each side.
Attachment #8764530 -
Attachment is obsolete: true
Attachment #8764530 -
Flags: review?(dbaron)
Attachment #8764535 -
Flags: review?(dbaron)
Attachment #8764525 -
Flags: review?(dbaron) → review+
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.
Attachment #8764528 -
Flags: review?(dbaron) → review-
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.)
Attachment #8764535 -
Flags: review?(dbaron) → review-
Attachment #8764533 -
Flags: review?(dbaron) → review+
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.
Assignee | ||
Comment 16•8 years ago
|
||
(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.
Assignee | ||
Comment 17•8 years ago
|
||
(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
Assignee | ||
Comment 18•8 years ago
|
||
(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.
Assignee | ||
Comment 19•8 years ago
|
||
Add 'space' keyword in property_database.js.
Attachment #8764525 -
Attachment is obsolete: true
Attachment #8769587 -
Flags: review?(dbaron)
Assignee | ||
Comment 20•8 years ago
|
||
Add 2 reftests for exactly fitting cases.
Attachment #8764535 -
Attachment is obsolete: true
Attachment #8769589 -
Flags: review?(dbaron)
Assignee | ||
Comment 21•8 years ago
|
||
Address dbaron's comments.
Attachment #8764528 -
Attachment is obsolete: true
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-.)
Flags: needinfo?(ethlin)
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'.
Attachment #8769587 -
Flags: review?(dbaron) → review+
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.
Attachment #8769589 -
Flags: review?(dbaron) → review-
Assignee | ||
Comment 25•8 years ago
|
||
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.
Flags: needinfo?(ethlin)
Attachment #8769606 -
Flags: review?(dbaron)
Assignee | ||
Comment 26•8 years ago
|
||
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'.
Assignee | ||
Comment 27•8 years ago
|
||
(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.
Assignee | ||
Comment 28•8 years ago
|
||
(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.
Assignee | ||
Comment 29•8 years ago
|
||
Add 'space' to other-value in 'border-image'.
Attachment #8769587 -
Attachment is obsolete: true
Attachment #8770395 -
Flags: review?(dbaron)
Assignee | ||
Comment 30•8 years ago
|
||
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'.
Attachment #8769589 -
Attachment is obsolete: true
Attachment #8770396 -
Flags: review?(dbaron)
Attachment #8770395 -
Flags: review?(dbaron) → review+
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.
Flags: needinfo?(ethlin)
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
Attachment #8770396 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 33•8 years ago
|
||
(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.
Flags: needinfo?(ethlin)
Assignee | ||
Comment 34•8 years ago
|
||
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.
Attachment #8769606 -
Attachment is obsolete: true
Attachment #8769606 -
Flags: review?(dbaron)
Attachment #8771363 -
Flags: review?(dbaron)
Assignee | ||
Comment 35•8 years ago
|
||
Address dbaron's comment. Remove link rel="match" in the border-image-repeat-space-{4,5}-ref-2.
Attachment #8770396 -
Attachment is obsolete: true
Attachment #8771363 -
Flags: review?(dbaron) → review+
Assignee | ||
Comment 36•8 years ago
|
||
try server: https://treeherder.mozilla.org/#/jobs?repo=try&revision=411909ff630e&selectedJob=24278030
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 37•8 years ago
|
||
Pushed by cbook@mozilla.com: 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
Keywords: checkin-needed
Comment 38•8 years ago
|
||
bugherder |
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
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox50:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Comment 39•8 years ago
|
||
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
Keywords: dev-doc-needed → dev-doc-complete
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•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/c237fc190702
You need to log in
before you can comment on or make changes to this bug.
Description
•