Background follow ups #25594

SimonSapin · 2020-01-24T13:28:42Z

Fix background-clip + border-radius
Fix background-clip + background-color
Render linear and radial gradients
Enable css/css-background/ in WPT


        Layout 2020: enable web-platform-tests/css/css-background


        Expected results for newly-enabled tests


        Make `layout_2020::display_list` a directory-module


        Add a `display_list::background` module


        Rename `clipping_area` to `painting_area`

https://drafts.csswg.org/css-backgrounds/#background-painting-area


        Fix combinations of `border-radius` and `background-clip`


        Apply `background-clip` to `background-color`


        Render gradients


        Update WPT expectations

highfive · 2020-01-24T13:28:48Z

Heads up! This PR modifies the following files:

@jgraham: tests/wpt/metadata-layout-2020/css/css-backgrounds/background-size/vector/tall--cover--percent-width-omitted-height-viewbox.html.ini, tests/wpt/metadata-layout-2020/css/css-backgrounds/background-size/vector/background-size-vector-006.html.ini, tests/wpt/metadata-layout-2020/css/css-backgrounds/background-size/vector/tall--cover--nonpercent-width-nonpercent-height-viewbox.html.ini, tests/wpt/metadata-layout-2020/css/css-backgrounds/background-size/vector/background-size-vector-008.html.ini, tests/wpt/metadata-layout-2020/css/css-backgrounds/parsing/border-image-width-computed.html.ini and 325 more

mrobinson

This looks pretty good to me modulo a few small doubts.

mrobinson · 2020-01-24T16:25:54Z

components/layout_2020/display_list/background.rs

+use style::values::specified::background::BackgroundRepeatKeyword as Repeat;
+use webrender_api::{self as wr, units};
+
+pub(super) struct Layer {


Layer is pretty overloaded name. Maybe something like BackgroundLayer would be a bit less opaque?

mrobinson · 2020-01-24T16:25:54Z

components/layout_2020/display_list/background.rs

+
+/// Abstract over the horizontal or vertical dimension
+/// Coordinates (0, 0) for the purpose of this function are the positioning area’s origin.
+fn layout_1d(


Perhaps it makes sense to make this a factory method for Layout1DResult?

I think that would be slightly nicer API design, but perhaps not worth the rightwards drift for a private function called exactly twice

mrobinson · 2020-01-24T16:25:54Z

components/layout_2020/display_list/background.rs

+    tile_size: &mut f32,
+    tile_spacing: &mut f32,
+    mut repeat: Repeat,


Any reason to have input/output parameters and a return struct here instead of simply returning these values in Layout1DResult?

I’ve done so for tile_spacing but kept tile_size since it already has a non-trivial value before this call and is only sometimes modified.

mrobinson · 2020-01-24T16:25:54Z

components/layout_2020/display_list/gradient.rs

+
+            // `{ x, y }` is now a vector of arbitrary length
+            // with the same direction as the gradient line.
+


Extra line here?

mrobinson · 2020-01-24T16:25:54Z

components/layout_2020/display_list/gradient.rs

+}
+
+/// https://drafts.csswg.org/css-images-4/#color-stop-fixup
+fn stops_fixup(


Maybe name this fixup_stops to make the function name a verb?

mrobinson · 2020-01-24T16:25:54Z

components/layout_2020/display_list/mod.rs

+    }
+
+    fn content_edge_clip(&self, builder: &mut DisplayListBuilder) -> Option<wr::ClipId> {
+        *self.content_edge_clip_id.init_once(|| {


Nice use of init_once here and above!

mrobinson · 2020-01-24T16:25:54Z

components/layout_2020/display_list/mod.rs

    }

    fn build(&mut self, builder: &mut DisplayListBuilder) {
        let hit_info = hit_info(&self.fragment.style, self.fragment.tag, Cursor::Default);
        if hit_info.is_some() {
            let mut common = builder.common_properties(self.border_rect);
            common.hit_info = hit_info;
-            self.with_border_edge_clip(builder, &mut common);
+            if let Some(clip_id) = self.border_edge_clip(builder) {


At some point it would be good to keep a stack of clips and spatial node ids, but for the moment I don't think it's necessary.

Yes, having that stack on the call stack is the idea of:

servo/components/layout_2020/display_list.rs

Lines 63 to 70 in 12693b5

// FIXME: use this for the `overflow` property or anything else that clips an entire subtree.

#[allow(unused)]

fn clipping_and_scrolling_scope<R>(&mut self, f: impl FnOnce(&mut Self) -> R) -> R {

let previous = self.current_space_and_clip;

let result = f(self);

self.current_space_and_clip = previous;

result

}

… which I copied from Layout 2013 code, but ended up not using for backgrounds because each background layer has a potentially-different background-clip value that only applies to one display item, so pushing/popping to the stack isn’t as useful.


        Adress review comments

SimonSapin · 2020-01-25T06:40:39Z

@bors-servo r=mrobinson

bors-servo · 2020-01-25T06:40:40Z

📌 Commit 923cddf has been approved by mrobinson

bors-servo · 2020-01-25T06:40:45Z

⌛ Testing commit 37ccefb with merge d9cd4ab...


        Auto merge of #25594 - servo:gradient, r=

Background follow ups * Fix `background-clip` + `border-radius` * Fix `background-clip` + `background-color` * Render linear and radial gradients * Enable `css/css-background/` in WPT

bors-servo · 2020-01-25T07:50:10Z

☀️ Test successful - status-taskcluster
State: approved= try=False

jdm · 2020-01-25T10:32:04Z

@bors-servo retry

jdm · 2020-01-25T10:33:58Z

@bors-servo p=10

jdm · 2020-01-25T10:34:11Z

@bors-servo r-

jdm · 2020-01-25T10:34:34Z

@bors-servo r=mrobinson

bors-servo · 2020-01-25T10:34:35Z

📌 Commit 37ccefb has been approved by mrobinson

bors-servo · 2020-01-25T10:34:40Z

⌛ Testing commit 37ccefb with merge 31eafad...


        Auto merge of #25594 - servo:gradient, r=mrobinson

Background follow ups * Fix `background-clip` + `border-radius` * Fix `background-clip` + `background-color` * Render linear and radial gradients * Enable `css/css-background/` in WPT

bors-servo · 2020-01-25T10:35:58Z

☀️ Test successful - status-taskcluster
Approved by: mrobinson
Pushing 31eafad to master...

SimonSapin · 2020-01-25T11:05:20Z

What happened, here? https://community-tc.services.mozilla.com/tasks/groups/afsyHI-hQuC1cMli-ibEQQ was all green and d9cd4ab got a green Status but it wasn’t merged. Did Homu drop it on the floor?

(After the second r+ Homu made an other merge commit 31eafad and https://community-tc.services.mozilla.com/tasks/groups/dvO_ldpRSwmaBBVTrhc06w re-used successful tasks from the previous task group, as intended.)

jdm · 2020-01-25T11:07:45Z

I looked at all of the webhooks and github did not report having problems delivering all of them. The homu log on servo-master1 didn't show any useful information. It's a mystery!

SimonSapin added 9 commits Jan 20, 2020

Layout 2020: enable web-platform-tests/css/css-background

68edef1

Expected results for newly-enabled tests

42bec01

Make `layout_2020::display_list` a directory-module

6901bf9

Add a `display_list::background` module

c8c198a

Rename `clipping_area` to `painting_area`

9fedade

https://drafts.csswg.org/css-backgrounds/#background-painting-area

Fix combinations of `border-radius` and `background-clip`

ea4882a

Apply `background-clip` to `background-color`

f39c3ff

Render gradients

632e731

Update WPT expectations

Loading status checks…

923cddf

SimonSapin added the A-layout/2020 label Jan 24, 2020

SimonSapin added this to In progress in Layout 2020 via automation Jan 24, 2020

highfive assigned paulrouget Jan 24, 2020

highfive added the S-awaiting-review label Jan 24, 2020

mrobinson reviewed Jan 24, 2020

View changes

Adress review comments

Loading status checks…

37ccefb

highfive assigned mrobinson and unassigned paulrouget Jan 25, 2020

highfive added S-awaiting-merge S-awaiting-review and removed S-awaiting-review labels Jan 25, 2020

highfive removed the S-awaiting-review label Jan 25, 2020

bors-servo merged commit 37ccefb into master Jan 25, 2020
2 checks passed

2 checks passed

Community-TC (pull_request) TaskGroup: success
Details

homu Test successful
Details

Layout 2020 automation moved this from In progress to Merged / resolved Jan 25, 2020

bors-servo deleted the gradient branch Jan 25, 2020

highfive removed the S-awaiting-merge label Jan 25, 2020

servo / servo

Background follow ups #25594

Background follow ups #25594

SimonSapin commented Jan 24, 2020

highfive commented Jan 24, 2020

mrobinson 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.

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.

SimonSapin commented Jan 25, 2020

bors-servo commented Jan 25, 2020

bors-servo commented Jan 25, 2020

bors-servo commented Jan 25, 2020

jdm commented Jan 25, 2020

jdm commented Jan 25, 2020

jdm commented Jan 25, 2020

jdm commented Jan 25, 2020

bors-servo commented Jan 25, 2020

bors-servo commented Jan 25, 2020

bors-servo commented Jan 25, 2020

SimonSapin commented Jan 25, 2020

jdm commented Jan 25, 2020


		// `{ x, y }` is now a vector of arbitrary length
		// with the same direction as the gradient line.

	// FIXME: use this for the `overflow` property or anything else that clips an entire subtree.
	#[allow(unused)]
	fn clipping_and_scrolling_scope<R>(&mut self, f: impl FnOnce(&mut Self) -> R) -> R {
	let previous = self.current_space_and_clip;
	let result = f(self);
	self.current_space_and_clip = previous;
	result
	}

servo / servo

Join GitHub today

Background follow ups #25594

Background follow ups #25594

Conversation

SimonSapin commented Jan 24, 2020

highfive commented Jan 24, 2020

mrobinson 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.

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.

SimonSapin commented Jan 25, 2020

bors-servo commented Jan 25, 2020

bors-servo commented Jan 25, 2020

bors-servo commented Jan 25, 2020

jdm commented Jan 25, 2020

jdm commented Jan 25, 2020

jdm commented Jan 25, 2020

jdm commented Jan 25, 2020

bors-servo commented Jan 25, 2020

bors-servo commented Jan 25, 2020

bors-servo commented Jan 25, 2020

SimonSapin commented Jan 25, 2020

jdm commented Jan 25, 2020