Fix incremental layout bugs for absolute and cleared elements #17581

mbrubeck · 2017-06-30T21:44:22Z

Fixes #17307. ~~There were two underlying bugs:~~

~~Nodes were not marked dirty when their style attribute changed.~~
BaseFlow flags set during flow construction could get out of sync with the element's style if repair_style was called.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Incremental layout bug in matter.js demo. #17307
There are tests for these changes

This change is

highfive · 2017-06-30T21:44:26Z

Heads up! This PR modifies the following files:

@bholley: components/style/servo/restyle_damage.rs
@KiChjang: components/script/dom/element.rs
@fitzgen: components/script/dom/element.rs
@emilio: components/layout/Cargo.toml, components/style/servo/restyle_damage.rs, components/layout/flow.rs

highfive · 2017-06-30T21:44:27Z

Warning

These commits include an empty title element (<title></title>). Consider adding appropriate metadata.

mbrubeck · 2017-06-30T21:45:42Z

r? @pcwalton

mbrubeck · 2017-06-30T21:58:53Z

@bors-servo try

bors-servo · 2017-06-30T21:58:58Z

⌛ Trying commit 3ec643c with merge b6fb1aa...

Fix incremental layout bugs for absolute and cleared elements Fixes #17307. There were two underlying bugs: * Nodes were not marked dirty when their style attribute changed. * BaseFlow flags set during flow construction could get out of sync with the element's style if repair_style was called. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #17307 - [x] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17581)

emilio · 2017-06-30T22:10:13Z

components/script/dom/element.rs

@@ -2214,6 +2214,7 @@ impl VirtualMethods for Element {
        match attr.local_name() {
            &local_name!("style") => {
                // Modifying the `style` attribute might change style.
+                node.dirty(NodeDamage::NodeStyleDamaged);


This shouldn't be needed. We handle that already here: https://github.com/servo/servo/blob/master/components/script/dom/document.rs#L2418.

Confirmed. I'll drop this change.

pcwalton · 2017-06-30T22:30:01Z

components/layout/flow.rs

+            self.flags.set(BLOCK_POSITION_IS_STATIC,
+                logical_position.block_start == LengthOrPercentageOrAuto::Auto &&
+                logical_position.block_end == LengthOrPercentageOrAuto::Auto);
+        }


How about making this a new incremental reflow flag, along the lines of RECONSTRUCT_FLOW or REFLOW_OUT_OF_FLOW?

Done. This increases the size of RestyleDamage from u8 to u16 and adds a bit more code. Let me know if you think it's worth the slight complexity cost.

bors-servo · 2017-06-30T22:30:28Z

💔 Test failed - arm32

emilio · 2017-06-30T22:32:42Z

So with this patch:

diff --git a/components/layout/query.rs b/components/layout/query.rs
index ae7a506124..f1a5747419 100644
--- a/components/layout/query.rs
+++ b/components/layout/query.rs
@@ -824,11 +824,13 @@ fn process_resolved_style_request_internal<'a, N>(requested_node: N,
             iterator.result.map(|r| r.to_css_string()).unwrap_or(String::new())
         },
 
+        /*
         LonghandId::Bottom | LonghandId::Top | LonghandId::Right | LonghandId::Left
         if applies && positioned && style.get_box().display !=
                 display::computed_value::T::none => {
             used_value_for_position_property(layout_el, layout_root, requested_node, longhand_id)
         }
+        */
         LonghandId::Width | LonghandId::Height
         if applies && style.get_box().display !=
                 display::computed_value::T::none => {

And printing the computed style:

<!DOCTYPE html>
<style>
  #rect {
    position: absolute;
    border: 1px solid red;
    width: 40px;
    height: 40px;
  }
</style>
<div id="rect"></div>
<script>
  var x = 10;
  var tick = function() {
    x += 4;
    var e = document.getElementById("rect");
    e.style.left = x + "px";
    console.log(getComputedStyle(e).left)
    window.requestAnimationFrame(tick);
  }
  requestAnimationFrame(tick);
</script>

I see the computed value updated properly, so now the thing is why the flow tree isn't getting properly updated without the dirty(..) call.

emilio · 2017-06-30T23:05:25Z

The demo definitely works with this series but removing the first patch, so it should be dropped IMO.

mbrubeck · 2017-06-30T23:34:03Z

@bors-servo try

bors-servo · 2017-06-30T23:34:07Z

⌛ Trying commit a57149a with merge 6a87c6e...

Fix incremental layout bugs for absolute and cleared elements Fixes #17307. <del>There were two underlying bugs:</del> * <del>Nodes were not marked dirty when their style attribute changed.</del> * BaseFlow flags set during flow construction could get out of sync with the element's style if repair_style was called. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #17307 - [x] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17581)

emilio

Comments on my side were addressed, looks good to me assuming @pcwalton is fine with UPDATE_FLOW_FLAGS.

emilio · 2017-06-30T23:51:11Z

components/layout/flow.rs

+        }
+
+        // For absolutely-positioned flows, changes to top/bottom/left/right can cause these flags
+        // to get out of date:


Perhaps note that we don't need to update the IS_ABSOLUTELY_POSITIONED flag because we reconstruct on position changes?

emilio · 2017-06-30T23:51:34Z

components/layout/flow.rs

@@ -1361,6 +1386,7 @@ impl<'a> MutableFlowUtils for &'a mut Flow {
    /// calling them individually, since there is no reason not to perform both operations.
    fn repair_style_and_bubble_inline_sizes(self, style: &::StyleArc<ServoComputedValues>) {
        self.repair_style(style);
+        mut_base(self).update_flags(style);


(And maybe call the function update_flags_if_needed?)

mbrubeck · 2017-07-01T00:10:24Z

Removed the new RestyleDamage flag; we can just use REFLOW_OUT_OF_FLOW as @pcwalton suggested on IRC.

This is necessary because the CLEARS_LEFT and CLEARS_RIGHT flags are set during flow construction.

pcwalton · 2017-07-01T00:32:33Z

Looks good. r=me if you like

mbrubeck · 2017-07-01T01:02:03Z

@bors-servo r=pcwalton

bors-servo · 2017-07-01T01:02:03Z

📌 Commit 3981884 has been approved by pcwalton

bors-servo · 2017-07-01T02:06:11Z

⌛ Testing commit 3981884 with merge 496eb4c...

Fix incremental layout bugs for absolute and cleared elements Fixes #17307. <del>There were two underlying bugs:</del> * <del>Nodes were not marked dirty when their style attribute changed.</del> * BaseFlow flags set during flow construction could get out of sync with the element's style if repair_style was called. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #17307 - [x] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17581)

bors-servo · 2017-07-01T02:43:41Z

💔 Test failed - mac-rel-css2

Fixes servo#17307.

mbrubeck · 2017-07-01T05:15:23Z

@bors-servo r=pcwalton

Updated test metadata

bors-servo · 2017-07-01T05:15:24Z

📌 Commit 2aef2af has been approved by pcwalton

bors-servo · 2017-07-01T05:15:28Z

⌛ Testing commit 2aef2af with merge ece35f9...

Fix incremental layout bugs for absolute and cleared elements Fixes #17307. <del>There were two underlying bugs:</del> * <del>Nodes were not marked dirty when their style attribute changed.</del> * BaseFlow flags set during flow construction could get out of sync with the element's style if repair_style was called. --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #17307 - [x] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/17581)

bors-servo · 2017-07-01T06:17:24Z

☀️ 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: pcwalton
Pushing ece35f9 to master...

highfive assigned glennw Jun 30, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jun 30, 2017

highfive assigned pcwalton and unassigned glennw Jun 30, 2017

mbrubeck force-pushed the incremental branch from 1403c4c to 3ec643c Compare June 30, 2017 21:52

emilio requested changes Jun 30, 2017

View reviewed changes

pcwalton reviewed Jun 30, 2017

View reviewed changes

highfive added the S-tests-failed The changes caused existing tests to fail. label Jun 30, 2017

mbrubeck force-pushed the incremental branch from 3ec643c to 1f02c8a Compare June 30, 2017 23:23

highfive removed the S-tests-failed The changes caused existing tests to fail. label Jun 30, 2017

mbrubeck force-pushed the incremental branch from 1f02c8a to a57149a Compare June 30, 2017 23:31

emilio approved these changes Jun 30, 2017

View reviewed changes

mbrubeck force-pushed the incremental branch from a57149a to 389158f Compare July 1, 2017 00:09

Reconstruct flows if the 'clear' property changes.

f01e1da

This is necessary because the CLEARS_LEFT and CLEARS_RIGHT flags are set during flow construction.

mbrubeck force-pushed the incremental branch from 389158f to 3981884 Compare July 1, 2017 00:11

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jul 1, 2017

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Jul 1, 2017

Update flags for absolutely positioned elements on style changes

2aef2af

Fixes servo#17307.

mbrubeck force-pushed the incremental branch from 3981884 to 2aef2af Compare July 1, 2017 05:14

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-tests-failed The changes caused existing tests to fail. labels Jul 1, 2017

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Jul 1, 2017

bors-servo merged commit 2aef2af into servo:master Jul 1, 2017

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Jul 1, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix incremental layout bugs for absolute and cleared elements #17581

Fix incremental layout bugs for absolute and cleared elements #17581

mbrubeck commented Jun 30, 2017 •

edited

highfive commented Jun 30, 2017

highfive commented Jun 30, 2017

mbrubeck commented Jun 30, 2017

mbrubeck commented Jun 30, 2017

bors-servo commented Jun 30, 2017

emilio Jun 30, 2017

mbrubeck Jun 30, 2017

pcwalton Jun 30, 2017

mbrubeck Jun 30, 2017

bors-servo commented Jun 30, 2017

emilio commented Jun 30, 2017 •

edited

emilio commented Jun 30, 2017

mbrubeck commented Jun 30, 2017

bors-servo commented Jun 30, 2017

emilio left a comment

emilio Jun 30, 2017

mbrubeck Jul 1, 2017

emilio Jun 30, 2017

mbrubeck Jul 1, 2017

mbrubeck commented Jul 1, 2017

pcwalton commented Jul 1, 2017

mbrubeck commented Jul 1, 2017

bors-servo commented Jul 1, 2017

bors-servo commented Jul 1, 2017

bors-servo commented Jul 1, 2017

mbrubeck commented Jul 1, 2017

bors-servo commented Jul 1, 2017

bors-servo commented Jul 1, 2017

bors-servo commented Jul 1, 2017

Fix incremental layout bugs for absolute and cleared elements #17581

Fix incremental layout bugs for absolute and cleared elements #17581

Conversation

mbrubeck commented Jun 30, 2017 • edited

highfive commented Jun 30, 2017

highfive commented Jun 30, 2017

mbrubeck commented Jun 30, 2017

mbrubeck commented Jun 30, 2017

bors-servo commented Jun 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bors-servo commented Jun 30, 2017

emilio commented Jun 30, 2017 • edited

emilio commented Jun 30, 2017

mbrubeck commented Jun 30, 2017

bors-servo commented Jun 30, 2017

emilio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbrubeck commented Jul 1, 2017

pcwalton commented Jul 1, 2017

mbrubeck commented Jul 1, 2017

bors-servo commented Jul 1, 2017

bors-servo commented Jul 1, 2017

bors-servo commented Jul 1, 2017

mbrubeck commented Jul 1, 2017

bors-servo commented Jul 1, 2017

bors-servo commented Jul 1, 2017

bors-servo commented Jul 1, 2017

mbrubeck commented Jun 30, 2017 •

edited

emilio commented Jun 30, 2017 •

edited