Recompute styles on viewport size change if they contain viewport percentages #9876

mbrubeck · 2016-03-04T19:08:22Z

Fixes #8754. Depends on servo/rust-cssparser#99. r? @SimonSapin

highfive · 2016-03-04T19:08:26Z

Heads up! This PR modifies the following files:

@bholley: components/style/selector_matching.rs, components/style/stylesheets.rs

highfive · 2016-03-04T19:08:27Z

Warning

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

mbrubeck · 2016-03-04T19:11:42Z

components/script/dom/htmlmetaelement.rs

@@ -73,6 +73,7 @@ impl HTMLMetaElement {
                        rules: vec![CSSRule::Viewport(translated_rule)],
                        origin: Origin::Author,
                        media: None,
+                        depends_on_viewport_size: true,


On second thought, maybe this stylesheet (one that contains a generated @viewport rule) should have the flag set to false, since viewport constraints are already recalculated on resize and shouldn't require the whole page to be recomputed.

SimonSapin · 2016-03-18T23:29:13Z

The is_device_dirty bit causes Stylist::update to re-create the selector hash maps in Stylist in case some conditional rules (@media, @supports when we support it) start or stop applying. But this seems to be a lot more work than necessary for just updating the computed values of vh and vw units, which could be done by running cascade on elements again. (Since we don’t save the specified values for an element, intermediate result of cascade before to_computed_value.)

And the dirty_on_viewport_size_change bits are only effective if none of the stylesheets use vh/vw at all.

To sum up, I think that this PR is correct but that we can do better (possibly later in follow ups) to avoid no-op work. Maybe tracking which elements (rather than which stylesheets) are affected by vh/vw. (Though would have to "infect" subtrees, because of inheritance.)

Reviewed 3 of 3 files at r2.
Review status: all files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.

components/style/selector_matching.rs, line 228 [r2] (raw file):
Is it possible to reach this code path without changing the viewport size (as used by vh/vw)? If so, this code might set the dirty bit more often than necessary.

Comments from the review on Reviewable.io

mbrubeck · 2016-03-19T00:01:01Z

To sum up, I think that this PR is correct but that we can do better (possibly later in follow ups) to avoid no-op work. Maybe tracking which elements (rather than which stylesheets) are affected by vh/vw.

Yes, I agree.

Review status: 2 of 3 files reviewed at latest revision, 2 unresolved discussions.

components/style/selector_matching.rs, line 228 [r2] (raw file):
It is possible. I've added a check to fix this.

Comments from the review on Reviewable.io

SimonSapin · 2016-03-19T00:05:02Z

r=me, pending servo/rust-cssparser#99

Reviewed 1 of 1 files at r3.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

Comments from the review on Reviewable.io

mbrubeck · 2016-03-19T18:50:15Z

@bors-servo r=SimonSapin

bors-servo · 2016-03-19T18:50:16Z

📌 Commit f4598fc has been approved by SimonSapin

bors-servo · 2016-03-19T19:11:26Z

⌛ Testing commit f4598fc with merge fb375d5...


        Auto merge of #9876 - mbrubeck:seen-viewport-percentages, r=SimonSapin

@SimonSapin

Recompute styles on viewport size change if they contain viewport percentages Fixes #8754. Depends on servo/rust-cssparser#99. r? @SimonSapin  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9876)

bors-servo · 2016-03-19T19:23:27Z

💔 Test failed - linux-dev

SimonSapin · 2016-03-19T19:54:36Z

/home/servo/buildbot/slave/linux-dev/build/tests/unit/style/stylesheets.rs:29:28: 143:6 error: missing field `dirty_on_viewport_size_change` in initializer of `style::stylesheets::Stylesheet<style::selector_impl::ServoSelectorImpl>` [E0063]
/home/servo/buildbot/slave/linux-dev/build/tests/unit/style/stylesheets.rs: 29     assert_eq!(stylesheet, Stylesheet {


        Recompute viewport-dependent styles on viewport size change

Fixes #8754.

mbrubeck · 2016-03-19T20:24:02Z

@bors-servo r=SimonSapin

fixed the unit test error

bors-servo · 2016-03-19T20:24:04Z

📌 Commit 25c1bce has been approved by SimonSapin

bors-servo · 2016-03-19T20:50:54Z

⌛ Testing commit 25c1bce with merge a4251c8...


        Auto merge of #9876 - mbrubeck:seen-viewport-percentages, r=SimonSapin

@SimonSapin

Recompute styles on viewport size change if they contain viewport percentages Fixes #8754. Depends on servo/rust-cssparser#99. r? @SimonSapin  [<img src="https://reviewable.io/review_button.svg" height="40" alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/9876)

bors-servo · 2016-03-19T21:51:55Z

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt, status-appveyor


        Don't re-add stylesheets to recompute vw/vh lengths

This is a follow-up to servo#9876. It avoids clearing and rebuilding SelectorMaps when vh and vw units need to be recomputed. Instead it just dirties all nodes, to force elements to be re-cascaded. Filed servo#10104 for later follow-up work to dirty only affected nodes.


        Auto merge of #10105 - mbrubeck:dirty-viewport-percentages, r=SimonSapin

@SimonSapin

Don't re-add stylesheets to recompute vw/vh lengths This is a follow-up to #9876. It avoids clearing and rebuilding SelectorMaps when vh and vw units need to be recomputed. Instead it just dirties all nodes, to force elements to be re-cascaded. Filed #10104 for later follow-up work to dirty only affected nodes. r? @SimonSapin  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10105)


        Don't re-add stylesheets to recompute vw/vh lengths

This is a follow-up to servo#9876. It avoids clearing and rebuilding SelectorMaps when vh and vw units need to be recomputed. Instead it just dirties all nodes, to force elements to be re-cascaded. Filed servo#10104 for later follow-up work to dirty only affected nodes.

highfive assigned SimonSapin Mar 4, 2016

highfive added the S-awaiting-review label Mar 4, 2016

mbrubeck added the S-blocked-on-external label Mar 4, 2016

mbrubeck reviewed Mar 4, 2016
View changes

mbrubeck force-pushed the mbrubeck:seen-viewport-percentages branch from 070b1fd to 88d9b65 Mar 4, 2016

mbrubeck force-pushed the mbrubeck:seen-viewport-percentages branch from 88d9b65 to 7cc42c3 Mar 19, 2016

mbrubeck force-pushed the mbrubeck:seen-viewport-percentages branch 2 times, most recently from d3d424d to f4598fc Mar 19, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Mar 19, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Mar 19, 2016

Recompute viewport-dependent styles on viewport size change

Loading status checks…

25c1bce

Fixes #8754.

mbrubeck force-pushed the mbrubeck:seen-viewport-percentages branch from f4598fc to 25c1bce Mar 19, 2016

highfive added S-awaiting-review and removed S-tests-failed labels Mar 19, 2016

highfive removed the S-awaiting-review label Mar 19, 2016

highfive added the S-awaiting-merge label Mar 19, 2016

bors-servo merged commit 25c1bce into servo:master Mar 19, 2016
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

mbrubeck mentioned this pull request Mar 21, 2016

Using viewport percentage lengths (e.g. vh, vw) causes unnecessary style recalc on resize #10104

Closed

mbrubeck mentioned this pull request Mar 21, 2016

Don't re-add stylesheets to recompute vw/vh lengths #10105

Merged

mbrubeck deleted the mbrubeck:seen-viewport-percentages branch May 11, 2016

servo / servo

Recompute styles on viewport size change if they contain viewport percentages #9876

Recompute styles on viewport size change if they contain viewport percentages #9876

mbrubeck commented Mar 4, 2016

highfive commented Mar 4, 2016

highfive commented Mar 4, 2016

This comment has been minimized.

SimonSapin commented Mar 18, 2016

mbrubeck commented Mar 19, 2016

SimonSapin commented Mar 19, 2016

mbrubeck commented Mar 19, 2016

bors-servo commented Mar 19, 2016

bors-servo commented Mar 19, 2016

bors-servo commented Mar 19, 2016

SimonSapin commented Mar 19, 2016

mbrubeck commented Mar 19, 2016

bors-servo commented Mar 19, 2016

bors-servo commented Mar 19, 2016

bors-servo commented Mar 19, 2016

servo / servo

Join GitHub today

Recompute styles on viewport size change if they contain viewport percentages #9876

Recompute styles on viewport size change if they contain viewport percentages #9876

Conversation

mbrubeck commented Mar 4, 2016

highfive commented Mar 4, 2016

highfive commented Mar 4, 2016

This comment has been minimized.

mbrubeck Mar 4, 2016

SimonSapin commented Mar 18, 2016

mbrubeck commented Mar 19, 2016

SimonSapin commented Mar 19, 2016

mbrubeck commented Mar 19, 2016

bors-servo commented Mar 19, 2016

bors-servo commented Mar 19, 2016

bors-servo commented Mar 19, 2016

SimonSapin commented Mar 19, 2016

mbrubeck commented Mar 19, 2016

bors-servo commented Mar 19, 2016

bors-servo commented Mar 19, 2016

bors-servo commented Mar 19, 2016