Unify background placement code #19651

pyfisch · 2017-12-27T21:49:40Z

Merges the implementations for background-image placement
from gradients and images. Add missing parts and fix bugs.

Now supported are the CSS properties:

background-attachment (except for local value, see #19650)
background-clip
background-origin
background-position-x/y
background-repeat
background-size

It should be noted that backgrounds are not clipped to
rounded border corners.
(This was done before but worked only in simple cases)
See: #19649

This solves the following issues:
closes #19626
closes #16657
closes #19482 (examples from http://lea.verou.me/css3patterns/ are rendered perfectly but the round border is completely ignored now)
closes #19577

./mach build -d does not report any errors
./mach test-tidy does not report any errors

I enabled a few tests with the first commit but I have written about a dozen manual tests I will try to turn into ref tests either before or after this patch lands.

@bors-servo try

The relationship between the different inputs is visualized in this flowchart:

This change is

highfive · 2017-12-27T21:49:43Z

Heads up! This PR modifies the following files:

@emilio: components/layout/display_list_builder.rs, components/layout/fragment.rs, components/layout/webrender_helpers.rs

emilio

This looks like a pretty neat cleanup!

Haven't gone through the math yet, but looks nice overall.

emilio · 2017-12-28T11:43:08Z

components/layout/display_list_builder.rs

+    // normal but only display a single tile.
+    if image_size * 2 >= *size {
+        tile_image(position,
+                                        size,


nit: Indentation here is off.

emilio · 2017-12-28T11:43:08Z

components/layout/display_list_builder.rs

-                            bounds.size.height)
-            }
+    fn compute_background_placement(&self,
+                                    state: &mut DisplayListBuildState,


nit: Maybe indenting these using block indentation, like:

fn compute_background_placement( &self, state: &mut ..., ) -> (Rect<Au>, Size2D<Au>, Size2D<Au>, Rect<Au>) {

is nicer. We should reformat this file to that style eventually anyway.

emilio · 2017-12-28T11:43:08Z

components/layout/display_list_builder.rs

+                                    absolute_bounds: Rect<Au>,
+                                    intrinsic_size: Option<Size2D<Au>>,
+                                    index: usize)
+                                    -> (Rect<Au>,


nit: Maybe a struct would be nicer instead of a tuple? It's not clear what each field means without looking at the function implementation.


        Unify background placement code

Merges the implementations for background-image placement from gradients and images. Add missing parts and fix bugs. Now supported are the CSS properties: * background-attachment (except for local value) * background-clip * background-origin * background-position-x/y * background-repeat * background-size It should be noted that backgrounds are not clipped to rounded border corners.

pyfisch · 2017-12-28T19:48:17Z

Fixed nits.

pyfisch · 2017-12-28T23:06:47Z

Another bug is that background-attachment: fixed is scrolled with the window contents while it should stay attached to the viewport. (This was not broken by this PR) See these two tests:

servo/webrender#1301 https://staktrace.github.io/moz-pages/bug/1368496/
servo/tests/wpt/web-platform-tests/css/css-backgrounds/background-attachment-fixed.html

Can you give me an example how to accomplish the fixed positioning? The webrender issue mentions push_clip_and_scroll_info but how exactly is it used here?

emilio · 2017-12-29T11:52:08Z

Can you give me an example how to accomplish the fixed positioning? The webrender issue mentions push_clip_and_scroll_info but how exactly is it used here?

Probably @glennw or @kvark can :)

kvark · 2017-12-29T16:12:25Z

Also cc @mrobinson

emilio

This looks fine to me. Not sure if you want to wait for the fixed image stuff or land this before, or prefer review from @glennw / @kvark / @mrobinson.

emilio · 2017-12-31T14:02:36Z

components/layout/display_list_builder.rs

+              absolute_anchor_origin: Au,
+              image_size: Au) {
+    // Avoid division by zero below!
+    if image_size == Au(0) {


I think it'd make sense to comment why the outparams are sane in this case, or why it doesn't matter..

emilio · 2017-12-31T14:02:36Z

components/layout/display_list_builder.rs

+    let own_size = if let Some(size) = intrinsic_size {
+        size
+    } else {
+        return match bg_size {


This may be cleaner with match intrinsic_size { ..., but no strong preference I guess

I am not sure if understand this. match bg_size { ... matches on the CSS property background-size. Maybe you meant I should replace own_size with intrinsic_size?

As a replacement for if let Some(size) = intrinsic_size on line 1050

Thanks. Will change it.

emilio · 2017-12-31T14:02:36Z

components/layout/display_list_builder.rs

+            &mut tile_spacing.height,
+            pos_y,
+            css_clip.origin.y,
+            css_clip.size.height);


nit: Paren on the next line, here and in the rest of the calls using this format.

emilio · 2017-12-31T14:02:36Z

components/layout/display_list_builder.rs

+            css_clip.origin.y,
+            css_clip.size.height);
+
+        return BackgroundPlacement { bounds, tile_size, tile_spacing, css_clip }


nit: No need for return.

pyfisch · 2018-01-01T16:45:25Z

Force pushed the fixed nits.

pyfisch · 2018-01-01T16:46:55Z

This looks fine to me. Not sure if you want to wait for the fixed image stuff or land this before, or prefer review from @glennw / @kvark / @mrobinson.

I'd like to land this as is.

emilio

Ok, let's land it as soon as that nit is addressed :)

emilio · 2018-01-01T18:24:28Z

components/layout/display_list_builder.rs

+                                 -> Size2D<Au> {
+    match intrinsic_size {
+        None => {
+            match bg_size {


Well, I would've keep the early return, just using match, but it's not a big deal, this looks fine too, thanks!

emilio · 2018-01-01T18:24:28Z

components/layout/display_list_builder.rs

+    // Images with a zero width or height are not displayed.
+    // Therefore the positions do not matter and can be left unchanged.
+    // NOTE: A possible optimization is not to build
+    // display lists in this case at all.


nit: s/display lists/display items/

emilio · 2018-01-01T18:24:48Z

Mind filing issues for the remaining bits @pyfisch? Thanks a lot!

pyfisch · 2018-01-01T19:11:33Z

nit: s/display lists/display items/

done.

I've opened #19650, #19669, #19672 for the remaining bits. Anything missing?

pyfisch · 2018-01-01T19:17:47Z

Travis failure is: oci runtime error: exec failed: container_linux.go:265: starting container process caused "could not create session key: disk quota exceeded"

emilio · 2018-01-01T21:50:39Z

Yeah, same failure happened over #19668. We don't gate on travis though, so should be gtg, thanks!

@bors-servo r+

bors-servo · 2018-01-01T21:50:40Z

📌 Commit 9849295 has been approved by emilio

bors-servo · 2018-01-01T22:01:31Z

⌛ Testing commit 9849295 with merge f6111c5...


        Auto merge of #19651 - pyfisch:background-placement, r=emilio

@bors-servo

Unify background placement code Merges the implementations for background-image placement from gradients and images. Add missing parts and fix bugs. Now supported are the CSS properties: * background-attachment (except for local value, see #19650) * background-clip * background-origin * background-position-x/y * background-repeat * background-size It should be noted that backgrounds are not clipped to rounded border corners. (This was done before but worked only in simple cases) See: #19649 This solves the following issues: closes #19626 closes #16657 closes #19482 (examples from http://lea.verou.me/css3patterns/ are rendered perfectly but the round border is completely ignored now) closes #19577 - `./mach build -d` does not report any errors - `./mach test-tidy` does not report any errors I enabled a few tests with the first commit but I have written about a dozen manual tests I will try to turn into ref tests either before or after this patch lands. @bors-servo try The relationship between the different inputs is visualized in this flowchart: ![flowchart-background](https://user-images.githubusercontent.com/2781017/34394430-5a06c72c-eb59-11e7-9d51-3d23e2215f07.png)  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19651)

bors-servo · 2018-01-01T22:42:46Z

💔 Test failed - linux-rel-wpt


        Test with fixes for background repeat: spaced

Test the interaction between background-repeat: spaced and CSS borders. Fix tile_image_axis() function.

pyfisch · 2018-01-02T13:05:16Z

Sorry for the failing tests. My computer is really slow at running them all. Are there options to speed up ./mach test-wpt? I just saw that the builders run test-wpt --release --processes 24 --total-chunks 2 --this-chunk 1 --always-succeed. Are the tests much faster with release builds and what number of processes should be used on a quad-core Laptop? Are the other parameters relevant?

At some point I was allowed to start bors with

@bors-servo try

but this doesn't work anymore. (Or does it?)

bors-servo · 2018-01-02T13:05:21Z

⌛ Trying commit cf12e27 with merge 62d2179...


        Auto merge of #19651 - pyfisch:background-placement, r=<try>

@bors-servo

Unify background placement code Merges the implementations for background-image placement from gradients and images. Add missing parts and fix bugs. Now supported are the CSS properties: * background-attachment (except for local value, see #19650) * background-clip * background-origin * background-position-x/y * background-repeat * background-size It should be noted that backgrounds are not clipped to rounded border corners. (This was done before but worked only in simple cases) See: #19649 This solves the following issues: closes #19626 closes #16657 closes #19482 (examples from http://lea.verou.me/css3patterns/ are rendered perfectly but the round border is completely ignored now) closes #19577 - `./mach build -d` does not report any errors - `./mach test-tidy` does not report any errors I enabled a few tests with the first commit but I have written about a dozen manual tests I will try to turn into ref tests either before or after this patch lands. @bors-servo try The relationship between the different inputs is visualized in this flowchart: ![flowchart-background](https://user-images.githubusercontent.com/2781017/34394430-5a06c72c-eb59-11e7-9d51-3d23e2215f07.png)  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19651)

bors-servo · 2018-01-02T13:16:08Z

💔 Test failed - mac-rel-wpt3

emilio · 2018-01-02T13:39:57Z

It does! And those tests are unexpected passes, so we need to mark them passing, not fixing anything :)

pyfisch · 2018-01-02T13:50:41Z

so we need to mark them passing, not fixing anything :)

Bad word choice on my side. I meant to mark them as passing by deleting the metadata files. The tests should pass now. (Only Mac crashed while building for some reason.)

pyfisch · 2018-01-02T14:19:23Z

All tests passed. (Except on Mac where some builders failed.)

@emilio Can you please approve?

emilio · 2018-01-02T14:21:03Z

Sure thing! Thanks a lot @pyfisch!

@bors-servo r+

bors-servo · 2018-01-02T14:21:04Z

📌 Commit cf12e27 has been approved by emilio

bors-servo · 2018-01-02T14:47:26Z

⌛ Testing commit cf12e27 with merge 691bff8...


        Auto merge of #19651 - pyfisch:background-placement, r=emilio

@bors-servo

Unify background placement code Merges the implementations for background-image placement from gradients and images. Add missing parts and fix bugs. Now supported are the CSS properties: * background-attachment (except for local value, see #19650) * background-clip * background-origin * background-position-x/y * background-repeat * background-size It should be noted that backgrounds are not clipped to rounded border corners. (This was done before but worked only in simple cases) See: #19649 This solves the following issues: closes #19626 closes #16657 closes #19482 (examples from http://lea.verou.me/css3patterns/ are rendered perfectly but the round border is completely ignored now) closes #19577 - `./mach build -d` does not report any errors - `./mach test-tidy` does not report any errors I enabled a few tests with the first commit but I have written about a dozen manual tests I will try to turn into ref tests either before or after this patch lands. @bors-servo try The relationship between the different inputs is visualized in this flowchart: ![flowchart-background](https://user-images.githubusercontent.com/2781017/34394430-5a06c72c-eb59-11e7-9d51-3d23e2215f07.png)  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19651)

bors-servo · 2018-01-02T16:00:19Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: emilio
Pushing 691bff8 to master...

CYBAI · 2018-01-03T03:27:18Z

@pyfisch Congrats!!!!!!


        Test with fixes for background repeat: spaced

Test the interaction between background-repeat: spaced and CSS borders. Fix tile_image_axis() function. Upstreamed from servo/servo#19651 [ci skip]

highfive assigned mbrubeck Dec 27, 2017

highfive added the S-awaiting-review label Dec 27, 2017

emilio reviewed Dec 28, 2017

View changes

pyfisch force-pushed the pyfisch:background-placement branch from 2888a19 to 3b3d4a9 Dec 28, 2017

emilio reviewed Dec 31, 2017

View changes

pyfisch force-pushed the pyfisch:background-placement branch from f074c8d to b97e6a5 Jan 1, 2018

emilio approved these changes Jan 1, 2018

View changes

pyfisch force-pushed the pyfisch:background-placement branch from b97e6a5 to 9849295 Jan 1, 2018

highfive assigned emilio and unassigned mbrubeck Jan 1, 2018

highfive added S-awaiting-merge and removed S-awaiting-review labels Jan 1, 2018

highfive added S-tests-failed and removed S-awaiting-merge labels Jan 1, 2018

Test with fixes for background repeat: spaced

Loading status checks…

cf12e27

Test the interaction between background-repeat: spaced and CSS borders. Fix tile_image_axis() function.

pyfisch force-pushed the pyfisch:background-placement branch from 9849295 to cf12e27 Jan 2, 2018

highfive added S-awaiting-review and removed S-tests-failed labels Jan 2, 2018

highfive added the S-tests-failed label Jan 2, 2018

highfive added S-awaiting-merge and removed S-awaiting-review S-tests-failed labels Jan 2, 2018

bors-servo merged commit cf12e27 into servo:master Jan 2, 2018
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jan 2, 2018

bors-servo mentioned this pull request Jan 2, 2018

style: Move border-image-repeat outside of mako. #19210

Closed

3 of 3 tasks complete

pyfisch deleted the pyfisch:background-placement branch Jan 2, 2018

jdm added a commit to web-platform-tests/wpt that referenced this pull request Jan 4, 2018

Test with fixes for background repeat: spaced

1b93a7d

Test the interaction between background-repeat: spaced and CSS borders. Fix tile_image_axis() function. Upstreamed from servo/servo#19651 [ci skip]

servo-wpt-sync mentioned this pull request Jan 4, 2018

Test with fixes for background repeat: spaced web-platform-tests/wpt#8919

Merged

pyfisch mentioned this pull request Jan 5, 2018

Browser.html tile corners no longer rendered correctly #19680

Closed

servo / servo

Join GitHub today

Unify background placement code #19651

Unify background placement code #19651

Conversation

pyfisch commented Dec 27, 2017 • edited

highfive commented Dec 27, 2017

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

pyfisch commented Dec 28, 2017

pyfisch commented Dec 28, 2017

emilio commented Dec 29, 2017

kvark commented Dec 29, 2017

emilio left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

pyfisch commented Jan 1, 2018

pyfisch commented Jan 1, 2018

emilio left a comment

This comment has been minimized.

This comment has been minimized.

emilio commented Jan 1, 2018

pyfisch commented Jan 1, 2018

pyfisch commented Jan 1, 2018

emilio commented Jan 1, 2018

bors-servo commented Jan 1, 2018

bors-servo commented Jan 1, 2018

bors-servo commented Jan 1, 2018

pyfisch commented Jan 2, 2018

bors-servo commented Jan 2, 2018

bors-servo commented Jan 2, 2018

emilio commented Jan 2, 2018

pyfisch commented Jan 2, 2018 • edited

pyfisch commented Jan 2, 2018

emilio commented Jan 2, 2018

bors-servo commented Jan 2, 2018

bors-servo commented Jan 2, 2018

bors-servo commented Jan 2, 2018

CYBAI commented Jan 3, 2018

pyfisch commented Dec 27, 2017 •

edited

pyfisch commented Jan 2, 2018 •

edited