Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Layout 2020 box tree support for Flexbox, behind a pref #26755

Merged
merged 6 commits into from Jun 4, 2020
Merged

Conversation

@SimonSapin
Copy link
Member

SimonSapin commented Jun 2, 2020

CC #26639

Layout support will come in future PRs. This one generates a zero-size fragment with no content.

@highfive
Copy link

highfive commented Jun 2, 2020

Heads up! This PR modifies the following files:

  • @emilio: components/style/properties/longhands/position.mako.rs, components/style/values/specified/box.rs
@nox
Copy link
Member

nox commented Jun 3, 2020

@bors-servo
Copy link
Contributor

bors-servo commented Jun 3, 2020

📌 Commit 1ca05db has been approved by nox

@highfive highfive assigned nox and unassigned asajeffrey Jun 3, 2020
@bors-servo
Copy link
Contributor

bors-servo commented Jun 3, 2020

Testing commit 1ca05db with merge 833dca9...

bors-servo added a commit that referenced this pull request Jun 3, 2020
Add Layout 2020 box tree support for Flexbox, behind a pref

CC #26639

Layout support will come in future PRs. This one generates a zero-size fragment with no content.
@bors-servo
Copy link
Contributor

bors-servo commented Jun 3, 2020

💔 Test failed - status-taskcluster

@SimonSapin
Copy link
Member Author

SimonSapin commented Jun 3, 2020

Failures are legit, I messed up the cfg for serialization of display: inline-table. Pushed a fixup commit for easier review, I’ll squash it before landing.

I’ve also pushed other commits for sprinkling some rayon on.

@SimonSapin SimonSapin force-pushed the flexbox branch from 68a8bb3 to 5fa3312 Jun 4, 2020
@SimonSapin
Copy link
Member Author

SimonSapin commented Jun 4, 2020

@bors-servo r=nox

@bors-servo
Copy link
Contributor

bors-servo commented Jun 4, 2020

📌 Commit 5fa3312 has been approved by nox

bors-servo added a commit that referenced this pull request Jun 4, 2020
Add Layout 2020 box tree support for Flexbox, behind a pref

CC #26639

Layout support will come in future PRs. This one generates a zero-size fragment with no content.
@bors-servo
Copy link
Contributor

bors-servo commented Jun 4, 2020

Testing commit 5fa3312 with merge af723c3...

@bors-servo
Copy link
Contributor

bors-servo commented Jun 4, 2020

💔 Test failed - status-taskcluster

@SimonSapin
Copy link
Member Author

SimonSapin commented Jun 4, 2020

This PR adds a FlexLevel variant to the LayoutBox enum. PR #26741 which landed in the meantime added code that matches on LayoutBox. So the merge does not conflict but fails to compile.

The fix is straightforward:

    fixup! Flex items in the box tree

diff --git components/layout_2020/flow/root.rs components/layout_2020/flow/root.rs
index 61f6ca888b..79285767d3 100644
--- components/layout_2020/flow/root.rs
+++ components/layout_2020/flow/root.rs
@@ -10,6 +10,7 @@ use crate::display_list::stacking_context::{
 };
 use crate::dom_traversal::{iter_child_nodes, Contents, NodeExt};
 use crate::element_data::LayoutBox;
+use crate::flexbox::FlexLevelBox;
 use crate::flow::construct::ContainsFloats;
 use crate::flow::float::FloatBox;
 use crate::flow::inline::InlineLevelBox;
@@ -119,6 +120,7 @@ impl BoxTree {
         enum UpdatePoint {
             AbsolutelyPositionedBlockLevelBox(ArcRefCell<BlockLevelBox>),
             AbsolutelyPositionedInlineLevelBox(ArcRefCell<InlineLevelBox>),
+            AbsolutelyPositionedFlexLevelBox(ArcRefCell<FlexLevelBox>),
         }
 
         fn update_point<'dom, Node>(
@@ -188,6 +190,16 @@ impl BoxTree {
                         },
                         _ => return None,
                     },
+                    LayoutBox::FlexLevel(flex_level_box) => match &*flex_level_box.borrow() {
+                        FlexLevelBox::OutOfFlowAbsolutelyPositionedBox(_)
+                            if box_style.position.is_absolutely_positioned() =>
+                        {
+                            UpdatePoint::AbsolutelyPositionedFlexLevelBox(
+                                flex_level_box.clone(),
+                            )
+                        },
+                        _ => return None,
+                    },
                 };
             Some((primary_style.clone(), display_inside, update_point))
         }
@@ -217,6 +229,12 @@ impl BoxTree {
                                 out_of_flow_absolutely_positioned_box,
                             );
                     },
+                    UpdatePoint::AbsolutelyPositionedFlexLevelBox(flex_level_box) => {
+                        *flex_level_box.borrow_mut() =
+                            FlexLevelBox::OutOfFlowAbsolutelyPositionedBox(
+                                out_of_flow_absolutely_positioned_box,
+                            );
+                    },
                 }
                 return true;
             }
@SimonSapin
Copy link
Member Author

SimonSapin commented Jun 4, 2020

@bors-servo r=nox

@bors-servo
Copy link
Contributor

bors-servo commented Jun 4, 2020

📌 Commit b9069d4 has been approved by nox

@bors-servo
Copy link
Contributor

bors-servo commented Jun 4, 2020

Testing commit b9069d4 with merge 3d6fed8...

@bors-servo
Copy link
Contributor

bors-servo commented Jun 4, 2020

☀️ Test successful - status-taskcluster
Approved by: nox
Pushing 3d6fed8 to master...

@bors-servo bors-servo merged commit 3d6fed8 into master Jun 4, 2020
2 checks passed
2 checks passed
Community-TC (pull_request) TaskGroup: success
Details
homu Test successful
Details
@bors-servo bors-servo deleted the flexbox branch Jun 4, 2020
}

/// https://drafts.csswg.org/css-text/#white-space
fn is_only_document_white_space(string: &str) -> bool {

This comment has been minimized.

@Manishearth

Manishearth Jul 8, 2020

Member

is this something that should be cached?

This comment has been minimized.

@SimonSapin

SimonSapin Jul 13, 2020

Author Member

It could be, but I’m not sure that would be a win. I’d guess that white-space only text nodes are typically small (at most a couple newlines plus indentation in non-minified HTML) so this should run quickly, and in other cases we’ll typically find a non-white-space character not far from the start where Iterator::all will short-circuit and we’ll also return quickly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

6 participants
You can’t perform that action at this time.