Add layout RPC query for getting an element's style #19881

jonleighton · 2018-01-27T20:34:08Z

This enables us to implement Element::has_css_layout_box() in a more
direct way, and also enables us to remove some of the existing more
specific queries.

Fixes #19811.

r? @emilio

This change is

highfive · 2018-01-27T20:34:12Z

Heads up! This PR modifies the following files:

@bholley: components/style/values/specified/box.rs, components/style/data.rs
@canaltinova: components/style/values/specified/box.rs, components/style/data.rs
@emilio: components/style/values/specified/box.rs, components/style/data.rs, components/layout/query.rs
@fitzgen: components/script/dom/node.rs, components/script/dom/element.rs, components/script/dom/window.rs, components/script/devtools.rs
@KiChjang: components/script/dom/node.rs, components/script/dom/element.rs, components/script/dom/window.rs, components/script/devtools.rs
@asajeffrey: components/script/dom/node.rs, components/script/dom/element.rs, components/script/dom/window.rs, components/script/devtools.rs

highfive · 2018-01-27T20:34:13Z

Warning

These commits modify style, layout, and script code, but no tests are modified. Please consider adding a test!

jonleighton · 2018-01-27T20:36:19Z

Would computed_style_query be a clearer name, or do you think style_query is already clear enough?

jonleighton · 2018-01-27T20:38:51Z

components/style/values/specified/box.rs

+
+    /// Returns true if the value is `None`
+    #[inline]
+    pub fn is_none(&self) -> bool {


Perhaps controversial to add this? I felt like it made the code read a bit nicer, and it avoided an extra use declaration for the Display type, but I don't mind much either way. I can see how adding these sorts of methods all the time could get out of hand. (Note that it also reduced the implementation of ElementStyles::is_display_none().)

Looks fine to me.

jonleighton · 2018-01-27T20:41:57Z

components/script/dom/element.rs

-        overflow_pair.x == overflow_x::computed_value::T::Visible
-    }
+    /// Computed value of overflow-x or overflow-y is "visible"
+    fn has_visible_overflow(&self) -> bool {


There's duplication between has_visible_overflow and has_hidden_overflow. Ideally I'd prefer to have a more generic fn has_some_overflow(&self, value: OverflowType) but that would need overflow-x and overflow-y to both use the same value type I think.

Yeah, that implies moving them outside of mako, which is not too bad now, but should definitely be a followup.

What about noting that this can be simplified after fixing #19183?

emilio

Looks great! Just a few documentation requests (it's easy to know why it's sound when writing it, but we should probably make sure that someone reading it knows it too).

Thanks a lot for this, this query can be super useful for all sorts of stuff! (I'm pretty sure we can use this to simplify and make stuff faster in canvasrenderingcontext2d.rs, and script_thread.rs, which right now call directly GetComputedStyle).

Those should of course be filed separately.

Thanks a lot for doing this again! :)

emilio · 2018-01-28T02:44:02Z

components/script_layout_interface/rpc.rs

-        }
-    }
-}
+pub struct StyleResponse(pub Option<Arc<ComputedValues>>);


Maybe document when this is none?

emilio · 2018-01-28T02:44:02Z

components/style/values/specified/box.rs

+
+    /// Returns true if the value is `None`
+    #[inline]
+    pub fn is_none(&self) -> bool {


Looks fine to me.

emilio · 2018-01-28T02:44:02Z

components/script/dom/element.rs

-        overflow_pair.x == overflow_x::computed_value::T::Visible
-    }
+    /// Computed value of overflow-x or overflow-y is "visible"
+    fn has_visible_overflow(&self) -> bool {


Yeah, that implies moving them outside of mako, which is not too bad now, but should definitely be a followup.

What about noting that this can be simplified after fixing #19183?

emilio · 2018-01-28T02:44:02Z

components/script/dom/element.rs

    fn has_css_layout_box(&self) -> bool {
-        self.upcast::<Node>().bounding_content_box().is_some()
+        let style = self.upcast::<Node>().style();
+        style.map_or(false, |s| !s.get_box().clone_display().is_none())


Please document why it's accurate? (because we won't return styles for elements in display: none subtrees)

emilio · 2018-01-28T02:44:02Z

components/script/dom/element.rs

-        let overflow_pair = window.overflow_query(self.upcast::<Node>().to_trusted_node_address());
-        overflow_pair.y == overflow_y::computed_value::T::Hidden
+    /// Computed value of overflow-x or overflow-y is "hidden"
+    fn has_hidden_overflow(&self) -> bool {


nit: Maybe has_any_hidden_overflow? (and same for visible?)

That way makes clear whether the semantics are all or any.

emilio · 2018-01-28T04:36:15Z

Would computed_style_query be a clearer name, or do you think style_query is already clear enough?

Sorry I didn't read this before... Both look fine to me honestly :)


        Add layout RPC query for getting an element's style

This enables us to implement Element::has_css_layout_box() in a more direct way, and also enables us to remove some of the existing more specific queries. Fixes #19811.

jonleighton · 2018-01-28T10:13:10Z

Thanks, I've addressed your comments. Also filed #19885 for the follow-up stuff you mentioned.

emilio

r=me, moving to Element can be a followup if you want, since callers right now are fine with it.

emilio · 2018-01-28T11:49:19Z

components/script/devtools.rs

@@ -154,12 +154,13 @@ pub fn handle_get_layout(documents: &Documents,
 }

 fn determine_auto_margins(window: &Window, node: &Node) -> AutoMargins {
-    let margin = window.margin_style_query(node.to_trusted_node_address());
+    let style = window.style_query(node.to_trusted_node_address()).unwrap();


I just noticed this... This unwrap may not be safe, I don't think anything guarantees that the element is not in a display: none subtree... Though it was a pre-existing bug, and I don't know where this code runs.

emilio · 2018-01-28T11:49:19Z

components/script/dom/node.rs

@@ -619,6 +620,10 @@ impl Node {
        window_from_node(self).client_rect_query(self.to_trusted_node_address())
    }

+    pub fn style(&self) -> Option<Arc<ComputedValues>> {


This should probably move to Element, since this getting called on, e.g., a text-node, will panic the layout thread.

Maybe worth doing before it lands, maybe fine as a followup? A doc comment here would be nice too :)

emilio · 2018-01-28T11:52:52Z

@bors-servo r+

Will file a followup for the documentation and moving it to Element :)

bors-servo · 2018-01-28T11:52:53Z

📌 Commit fe583fc has been approved by emilio

bors-servo · 2018-01-28T11:52:57Z

⌛ Testing commit fe583fc with merge 2a46067...


        Auto merge of #19881 - jonleighton:issue-19811, r=emilio

@emilio

Add layout RPC query for getting an element's style This enables us to implement Element::has_css_layout_box() in a more direct way, and also enables us to remove some of the existing more specific queries. Fixes #19811. r? @emilio  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/19881)

bors-servo · 2018-01-28T12:42:30Z

☀️ 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 2a46067 to master...

jonleighton · 2018-01-28T13:41:34Z

I just noticed this... This unwrap may not be safe, I don't think anything guarantees that the element is not in a display: none subtree... Though it was a pre-existing bug, and I don't know where this code runs.

Good point. I filed #19887.

highfive assigned emilio Jan 27, 2018

highfive added the S-awaiting-review label Jan 27, 2018

jonleighton force-pushed the jonleighton:issue-19811 branch from c66eece to d4d17ba Jan 27, 2018

jonleighton reviewed Jan 27, 2018

View changes

emilio approved these changes Jan 28, 2018

View changes

Add layout RPC query for getting an element's style

Loading status checks…

fe583fc

This enables us to implement Element::has_css_layout_box() in a more direct way, and also enables us to remove some of the existing more specific queries. Fixes #19811.

jonleighton force-pushed the jonleighton:issue-19811 branch from d4d17ba to fe583fc Jan 28, 2018

jonleighton mentioned this pull request Jan 28, 2018

Avoid `Window::GetComputedStyle` when checking for `display: none` #19885

Closed

emilio approved these changes Jan 28, 2018

View changes

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

bors-servo merged commit fe583fc into servo:master Jan 28, 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 28, 2018

bors-servo mentioned this pull request Jan 28, 2018

Implement element.innerText getter #19754

Merged

jonleighton mentioned this pull request Jan 28, 2018

Handle `None` in `script::devtools::determine_auto_margins` #19887

Open

ferjm mentioned this pull request Feb 19, 2018

Option::unwrap() in window impl ::overflow_query #18630

Closed

servo / servo

Add layout RPC query for getting an element's style #19881

Add layout RPC query for getting an element's style #19881

jonleighton commented Jan 27, 2018 •

edited by SimonSapin

highfive commented Jan 27, 2018

highfive commented Jan 27, 2018

jonleighton commented Jan 27, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

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.

emilio commented Jan 28, 2018

jonleighton commented Jan 28, 2018

emilio left a comment

This comment has been minimized.

This comment has been minimized.

emilio commented Jan 28, 2018

bors-servo commented Jan 28, 2018

bors-servo commented Jan 28, 2018

bors-servo commented Jan 28, 2018

jonleighton commented Jan 28, 2018

servo / servo

Join GitHub today

Add layout RPC query for getting an element's style #19881

Add layout RPC query for getting an element's style #19881

Conversation

jonleighton commented Jan 27, 2018 • edited by SimonSapin

highfive commented Jan 27, 2018

highfive commented Jan 27, 2018

jonleighton commented Jan 27, 2018

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

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.

emilio commented Jan 28, 2018

jonleighton commented Jan 28, 2018

emilio left a comment

This comment has been minimized.

This comment has been minimized.

emilio commented Jan 28, 2018

bors-servo commented Jan 28, 2018

bors-servo commented Jan 28, 2018

bors-servo commented Jan 28, 2018

jonleighton commented Jan 28, 2018

jonleighton commented Jan 27, 2018 •

edited by SimonSapin