Implement core float layout for layout 2020 #27216

pcwalton · 2020-07-09T03:06:14Z

Add an implementation of the core float and clear placement logic in layout 2020, not yet wired to the rest of layout.

This commit implements an object that handles the 10 rules in CSS 2.1:

https://www.w3.org/TR/CSS2/visuren.html#float-position

The implementation strategy is that of a persistent balanced binary search tree
of float bands. Binary search trees are commonly used for implementing float
positioning; e.g. by WebKit. Persistence enables each object that interacts
with floats to efficiently contain a snapshot of the float list at the time
that object was laid out. That way, incremental layout can invalidate and start
reflow at any point in a containing block.

This commit features extensive use of
QuickCheck to ensure that the rules
of the CSS specification are followed.

Because this is not yet connected to layout, floats will not actually be laid
out in Web pages yet.

Note that unit tests as set up in Servo currently require types that they
access to be public. Therefore, some internal layout 2020 types that were
previously private have been made public. This is somewhat unfortunate.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #___ (GitHub issue number if applicable)

There are tests for these changes OR
These changes do not require tests because ___

pcwalton · 2020-07-09T11:55:44Z

This is #25167.

pcwalton · 2020-07-11T03:17:54Z

This should be ready for review now.

r? @Manishearth

pcwalton · 2020-07-11T03:19:06Z

@jdm I get Package quickcheck 0.9.2 depends on blocked package rand when I run mach test-tidy. How should I proceed?

pcwalton · 2020-07-11T03:38:05Z

I added quickcheck to the whitelist in servo-tidy.toml, noting that it is only used in tests.

Manishearth · 2020-07-11T06:17:35Z

@pcwalton while I'd love to review this I'm not familiar enough with layout 2020 to be the primary reviewer (I'm not yet done understanding the flexbox code!)

pcwalton · 2020-07-11T16:43:01Z

@Manishearth Well, @jdm suggested that you should be the reviewer since our main layout folks are out at the moment. Do you know who else would be a good reviewer?

Note that there is basically no dependency on layout 2020 in this code, since it's not hooked up to the rest of layout yet.

Manishearth · 2020-07-11T16:44:31Z

Oh, in that case I'll go for it! Might take a while since I'm still understand things

Manishearth

I think a deeper explanation of how the bands are supposed to work would help: seems like a float can exist in multiple bands?

Still not done reading everything, and I have to go through the tests which seem to give a clearer idea of how this is supposed to be used

Manishearth · 2020-07-17T00:21:30Z

components/layout_2020/flow/float.rs

+        }
+    }
+
+    /// Returns the current ceiling value. No floats may be placed (logically) above this line.


No new floats

Manishearth · 2020-07-17T00:21:30Z

components/layout_2020/flow/float.rs

+    /// Places a new float and adds it to the list. Returns the start corner of its margin box.
+    pub fn add_float(&mut self, new_float: FloatInfo) -> Vec2<Length> {
+        // Find the first band this float fits in.
+        let mut first_band = self.bands.find(self.ceiling).unwrap();


would it make sense to cache the current band value and update it whenever the ceiling is updated or bands change?

I'd want to measure before doing that; it doesn't seem obviously a win as find is just O(log n).

Manishearth · 2020-07-17T00:21:30Z

components/layout_2020/flow/float.rs

+#[derive(Clone, Copy, Debug, PartialEq)]
+pub struct FloatBand {
+    pub top: Length,
+    pub left: Option<Length>,


document what None means here?

Also what is left here anyway? Available space? Distance from left edge of box?

Every band (terminology from Gecko) represents a vertical region where floats might be placed. Left and right are distances from the left/right edge of the container to the first legal horizontal position where floats might be placed. In other words, floats have to go in between the "left edge + left" and "right edge - right".

None is there to differentiate between the cases of a zero-width float on some side and no floats at all on that side. If we didn't have this, then, for example, "clear" would not work properly with zero-width left floats, as the left field would be zero even if zero-width left floats are present and we would be unable to figure out how to clear past them.

Could you document this?

Also, what happens when a band is "full"? I don't think I see code handling that (is it supposed to be handled?)

If a band is "full", then float_fits() will return false and the loop at the beginning of add_float() will go on to the next band.

Manishearth · 2020-07-17T00:21:30Z

components/layout_2020/flow/float.rs

+
+        if band.top < this.band.top {
+            this.left = this.left.insert(band);
+            return FloatBandLink(Some(Arc::new(this))).skew().split();


aren't skew and split inverses of each other?

No, because the level checks are different. Skew operates if level(T) = level(L) and split operates if level(T) = level(X).

Oh, I didn't notice the level checks

Manishearth · 2020-07-17T00:21:30Z

components/layout_2020/flow/float.rs

+
+        // Split the first band if necessary.
+        first_band.top = new_float_rect.start_corner.block;
+        self.bands = self.bands.insert(first_band);


Does insert() actually split the band? It seems to me that it just changes it

Insert and split are the same operation, because bands only store their top value. e.g. if our bands have top values of 0px, 1px, 5px before, and we're inserting a float at y = 3px, then it becomes 0px, 1px, 3px, 5px after, effectively splitting the 1px band.

Manishearth

r=me


        Add an implementation of the core float and clear placement logic in …

…layout 2020, not yet wired to the rest of layout. This commit implements an object that handles the 10 rules in CSS 2.1: https://www.w3.org/TR/CSS2/visuren.html#float-position The implementation strategy is that of a persistent balanced binary search tree of float bands. Binary search trees are commonly used for implementing float positioning; e.g. by WebKit. Persistence enables each object that interacts with floats to efficiently contain a snapshot of the float list at the time that object was laid out. That way, incremental layout can invalidate and start reflow at any point in a containing block. This commit features extensive use of [QuickCheck](https://github.com/BurntSushi/quickcheck) to ensure that the rules of the CSS specification are followed. Because this is not yet connected to layout, floats will not actually be laid out in Web pages yet. Note that unit tests as set up in Servo currently require types that they access to be public. Therefore, some internal layout 2020 types that were previously private have been made public. This is somewhat unfortunate. Part of #25167.

pcwalton · 2020-07-20T19:53:06Z

@bors-servo: r=Manishearth

bors-servo · 2020-07-20T19:53:07Z

📌 Commit 5b36d21 has been approved by Manishearth

bors-servo · 2020-07-20T20:03:34Z

⌛ Testing commit 5b36d21 with merge 2e9c029...


        Auto merge of #27216 - pcwalton:floats-2020, r=Manishearth

Implement core float layout for layout 2020  Add an implementation of the core float and clear placement logic in layout 2020, not yet wired to the rest of layout. This commit implements an object that handles the 10 rules in CSS 2.1: https://www.w3.org/TR/CSS2/visuren.html#float-position The implementation strategy is that of a persistent balanced binary search tree of float bands. Binary search trees are commonly used for implementing float positioning; e.g. by WebKit. Persistence enables each object that interacts with floats to efficiently contain a snapshot of the float list at the time that object was laid out. That way, incremental layout can invalidate and start reflow at any point in a containing block. This commit features extensive use of [QuickCheck](https://github.com/BurntSushi/quickcheck) to ensure that the rules of the CSS specification are followed. Because this is not yet connected to layout, floats will not actually be laid out in Web pages yet. Note that unit tests as set up in Servo currently require types that they access to be public. Therefore, some internal layout 2020 types that were previously private have been made public. This is somewhat unfortunate. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___

bors-servo · 2020-07-20T20:55:36Z

💔 Test failed - status-taskcluster

bors-servo · 2020-07-20T23:34:10Z

💔 Test failed - status-taskcluster

jdm · 2020-07-21T00:05:58Z

@bors-servo retry

#24622

bors-servo · 2020-07-21T00:17:19Z

⌛ Testing commit 5b36d21 with merge def5bad...


        Auto merge of #27216 - pcwalton:floats-2020, r=Manishearth

Implement core float layout for layout 2020  Add an implementation of the core float and clear placement logic in layout 2020, not yet wired to the rest of layout. This commit implements an object that handles the 10 rules in CSS 2.1: https://www.w3.org/TR/CSS2/visuren.html#float-position The implementation strategy is that of a persistent balanced binary search tree of float bands. Binary search trees are commonly used for implementing float positioning; e.g. by WebKit. Persistence enables each object that interacts with floats to efficiently contain a snapshot of the float list at the time that object was laid out. That way, incremental layout can invalidate and start reflow at any point in a containing block. This commit features extensive use of [QuickCheck](https://github.com/BurntSushi/quickcheck) to ensure that the rules of the CSS specification are followed. Because this is not yet connected to layout, floats will not actually be laid out in Web pages yet. Note that unit tests as set up in Servo currently require types that they access to be public. Therefore, some internal layout 2020 types that were previously private have been made public. This is somewhat unfortunate. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___

bors-servo · 2020-07-21T00:43:53Z

💔 Test failed - status-taskcluster

pcwalton · 2020-07-21T01:35:31Z

_RegisterApplication(), FAILED TO establish the default connection to the WindowServer, _CGSDefaultConnection() is NULL. Assuming intermittent.

pcwalton · 2020-07-21T01:35:53Z

@bors-servo: retry

jdm · 2020-07-21T01:47:42Z

Filed #27342 about that particular brand of intermittent failure. The _RegisterApplication thing is unrelated and doesn't appear to impact tests.


        Auto merge of #27216 - pcwalton:floats-2020, r=Manishearth

Implement core float layout for layout 2020  Add an implementation of the core float and clear placement logic in layout 2020, not yet wired to the rest of layout. This commit implements an object that handles the 10 rules in CSS 2.1: https://www.w3.org/TR/CSS2/visuren.html#float-position The implementation strategy is that of a persistent balanced binary search tree of float bands. Binary search trees are commonly used for implementing float positioning; e.g. by WebKit. Persistence enables each object that interacts with floats to efficiently contain a snapshot of the float list at the time that object was laid out. That way, incremental layout can invalidate and start reflow at any point in a containing block. This commit features extensive use of [QuickCheck](https://github.com/BurntSushi/quickcheck) to ensure that the rules of the CSS specification are followed. Because this is not yet connected to layout, floats will not actually be laid out in Web pages yet. Note that unit tests as set up in Servo currently require types that they access to be public. Therefore, some internal layout 2020 types that were previously private have been made public. This is somewhat unfortunate. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [ ] These changes fix #___ (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___

bors-servo · 2020-07-21T02:19:14Z

⌛ Testing commit 5b36d21 with merge b185f91...

bors-servo · 2020-07-21T02:59:24Z

💔 Test failed - status-taskcluster

jdm · 2020-07-21T03:03:55Z

@bors-servo retry

#26895

bors-servo · 2020-07-21T04:09:02Z

⌛ Testing commit 5b36d21 with merge 132d8b4...

bors-servo · 2020-07-21T04:58:41Z

☀️ Test successful - status-taskcluster
Approved by: Manishearth
Pushing 132d8b4 to master...

pcwalton changed the title ~~Implement floats for layout 2020~~ Implement core float layout for layout 2020 Jul 9, 2020

highfive added the S-awaiting-review label Jul 10, 2020

pcwalton force-pushed the pcwalton:floats-2020 branch from 78a2dee to b76448d Jul 11, 2020

pcwalton marked this pull request as ready for review Jul 11, 2020

pcwalton requested a review from Manishearth Jul 11, 2020

highfive assigned Manishearth Jul 11, 2020

pcwalton force-pushed the pcwalton:floats-2020 branch 2 times, most recently from b41acbe to 8e39047 Jul 11, 2020

pcwalton force-pushed the pcwalton:floats-2020 branch from 8e39047 to 88119e3 Jul 11, 2020

Manishearth reviewed Jul 17, 2020

View changes

pcwalton force-pushed the pcwalton:floats-2020 branch 2 times, most recently from 6f0c748 to 0aac7be Jul 17, 2020

Manishearth approved these changes Jul 20, 2020

View changes

pcwalton force-pushed the pcwalton:floats-2020 branch from 0aac7be to 5b36d21 Jul 20, 2020

highfive added S-awaiting-merge and removed S-awaiting-review labels Jul 20, 2020

highfive removed the S-awaiting-merge label Jul 20, 2020

highfive added the S-awaiting-merge label Jul 20, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 20, 2020

highfive added S-awaiting-merge and removed S-tests-failed labels Jul 21, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 21, 2020

highfive added S-awaiting-merge and removed S-tests-failed labels Jul 21, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels Jul 21, 2020

highfive added S-awaiting-merge and removed S-tests-failed labels Jul 21, 2020

bors-servo merged commit 132d8b4 into servo:master Jul 21, 2020
2 checks passed

2 checks passed

Community-TC (pull_request) TaskGroup: success
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Jul 21, 2020

servo / servo

Join GitHub today

Implement core float layout for layout 2020 #27216

Implement core float layout for layout 2020 #27216

Conversation

pcwalton commented Jul 9, 2020 • edited

pcwalton commented Jul 9, 2020

pcwalton commented Jul 11, 2020 • edited

pcwalton commented Jul 11, 2020

pcwalton commented Jul 11, 2020

Manishearth commented Jul 11, 2020

pcwalton commented Jul 11, 2020 • edited

Manishearth commented Jul 11, 2020

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

pcwalton Jul 17, 2020 • edited

This comment has been minimized.

This comment has been minimized.

pcwalton Jul 20, 2020 • edited

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.

Manishearth left a comment

pcwalton commented Jul 20, 2020

bors-servo commented Jul 20, 2020

bors-servo commented Jul 20, 2020

bors-servo commented Jul 20, 2020

bors-servo commented Jul 20, 2020

jdm commented Jul 21, 2020

bors-servo commented Jul 21, 2020

bors-servo commented Jul 21, 2020

pcwalton commented Jul 21, 2020 • edited

pcwalton commented Jul 21, 2020

jdm commented Jul 21, 2020

bors-servo commented Jul 21, 2020

bors-servo commented Jul 21, 2020

jdm commented Jul 21, 2020

bors-servo commented Jul 21, 2020

bors-servo commented Jul 21, 2020

pcwalton commented Jul 9, 2020 •

edited

pcwalton commented Jul 11, 2020 •

edited

pcwalton commented Jul 11, 2020 •

edited

pcwalton Jul 17, 2020 •

edited

pcwalton Jul 20, 2020 •

edited

pcwalton commented Jul 21, 2020 •

edited