Initial (very rudimentary) flexbox implementation. #7154

zentner-kyle · 2015-08-11T06:04:13Z

This "flexbox implementation" is essentially a copy-and-paste / meld of code from various Flow types.

All of the flexbox related CSS properties should be parsed correctly. All of the children of the flex container (flex items) are (correctly) made into unfloated blocks. If the flexbox is a row or reverse-row, the total inline size is evenly distributed among the children, and they are laid out horizontally. Otherwise, the flexbox acts like a block with unfloated children.

I'd like to improve upon this code in a later pull request.

Closes #6636. I think?

r? @pcwalton

highfive · 2015-08-11T06:04:17Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @metajack (or someone else) soon.

highfive · 2015-08-11T06:04:19Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify layout code, but no reftests are modified. Please consider adding a reftest!

pcwalton · 2015-08-11T07:16:53Z

Wow! I'll take a look at this tomorrow.

zentner-kyle · 2015-08-13T01:15:00Z

Thanks for the feedback!

I think I've addressed the above comments? There weren't any on the later changes.

Yes, flex-done.rs was accidentally included.

Is rebasing mid-review like this an acceptable workflow?

pcwalton · 2015-08-13T02:58:41Z

Yup, works for me. I'll finish the review ASAP.

pcwalton · 2015-08-13T16:53:18Z

components/layout/display_list_builder.rs

+    fn build_display_list_for_flex(&mut self,
+                                   display_list: Box<DisplayList>,
+                                   layout_context: &LayoutContext) {
+        // Draw the rest of the block.


There may not be any need for this function—just call build_display_list_for_block directly in FlexFlow::build_display_list. Unless there will actually need to be special painting behavior for flexbox in the future?

pcwalton · 2015-08-13T16:54:21Z

components/layout/flex.rs

+
+        let main_mode = match flex_style(&fragment).flex_direction {
+            flex_direction::T::row_reverse    => Mode::Inline,
+            flex_direction::T::row            => Mode::Inline,


nit: you can use or-patterns (flex_direction::T::row_reverse | flex_direction::T::row => ...) to avoid a little bit of code duplication here

pcwalton · 2015-08-13T16:54:47Z

components/layout/flex.rs

+            main_mode: main_mode
+        };
+
+        this


No need for the this temporary; just write BlockFlow { ... } as the last expression.

pcwalton · 2015-08-13T16:56:05Z

components/layout/flex.rs

+
+    // TODO(zentner): This function should actually flex elements!
+    // Currently, this is the core of TableRowFlow::assign_block_size() with
+    // float related logic stripped out.


Does TableRowFlow actually have any float related logic?

Surprisingly, yes. However, it's relatively minimal (mostly a call to kid.place_float_if_applicable()).

pcwalton · 2015-08-13T16:59:31Z

Layout looks good modulo a few nits. I'm going to bump the properties.rs.mako review over to @SimonSapin

r? @SimonSapin

SimonSapin · 2015-08-13T19:20:15Z

New properties and values should be marked "experimental". That should be easy for longhand properties (adding experimental=True at the right place), but for longhands (flex: …) or values (display: flex) you may need to add some Mako template support, or just call ::util::opts::experimental_enabled() directly.

jdm · 2015-08-13T20:49:05Z

I thought we only cared about marking things experimental that are not actually standardized and therefore shouldn't be exposed to regular web content, not merely incomplete things.

metajack · 2015-08-13T20:50:27Z

This came up in the meeting a few weeks ago. Either @SimonSapin or @mbrubeck had some changes they planned to put behind -e until they were more fully baked.

SimonSapin · 2015-08-13T22:16:30Z

Maybe we can rename "experimental" or have more than one flag, but the idea is that the barrier to entry for pref’ed-off things can be lower than otherwise. You can land piece-by-piece unfinished things that can panic (assertion errors, unimplemented!(), …) or give arbitrarily wrong results.

(I’m not saying at all that this PR is bad! I haven’t looked in details yet. But it doesn’t need to get everything right the first time.)

SimonSapin · 2015-08-14T21:47:55Z

components/style/properties.mako.rs

+                    if !(${condition}) {
+                        return Err(())
+                    }
+                    Ok(SpecifiedValue::Specified(value as ${type}))


Does this warn, when type is i32? (Trivial cast, or something like that.)

That would make sense, but I don't think redundant / trivial casts ever do warn. At least, there aren't any warnings when I build this file using ./mach build, and http://is.gd/9aTZQ1 doesn't show any warnings when compiling.

SimonSapin · 2015-08-14T21:50:09Z

components/style/properties.mako.rs

+    // Flex item properties
+    ${integer_type_or_auto("order", "i32", "Specified(0)")}
+    ${integer_type_or_auto("flex-grow", "u32", "Specified(0)", "value >= 0")}
+    ${integer_type_or_auto("flex-shrink", "u32", "Specified(1)", "value >= 0")}


Does the layout code support all of these? It’s better not to parse properties or values for which we don’t have any support. (And adding @supports rules will make this more important.)

Right now, almost none of these are supported (with the exception of flex-direction). However, I do intend to add support for most of them when I can. Should I remove them, or somehow feature gate them (besides using experimental=True)?

Also, I just realized that align-self is not in the right location in this list, it should be with the other flex item properties.

Yes, please remove the parsing of unsupported properties. Especially since they are one-line macro calls, they’re easy to add back when support is added.

SimonSapin · 2015-08-14T21:56:22Z

components/style/properties.mako.rs

+        let flex_basis = if flex_basis.is_ok() {
+            flex_basis
+        } else {
+            flex_basis::parse(context, input)


Each of these four foo::parse() calls needs to be wrapped in input.try(|input| …) to avoid silently ignoring part of the input when they return Err.

SimonSapin · 2015-08-14T21:58:03Z

(Done reviewing properties.mako.rs up to 47a6a0e.)

bors-servo · 2015-08-17T23:42:03Z

☔ The latest upstream changes (presumably #7222) made this pull request unmergeable. Please resolve the merge conflicts.

paulrouget · 2015-08-21T09:09:07Z

Very excited to see this initial PR.

For the record, here is a basic HTML template showing how we use flexbox model in browser.html: #7304 (works well in Firefox 43, appears to crash with this PR (panic: attempted to divide by zero)).

Hope this will help for future work on flexbox model.


        Parse flex-direction CSS property.


        Create FlexFlow's for 'display: flex' nodes.

This commit doesn't implement any flexbox behavior at all. It just constructs FlexFlow's, which act just like the BlockFlow from which they "inherit."


        Implement FlexFlow::assign_inline_sizes().


        Implement FlexFlow::assign_block_size().


        Add reftests for basic flexbox functionality.

These tests (flex_row_direction.html and flex_column_direction.html) are correct, but only test the small amount of current flexbox functionality.


        Auto merge of #7312 - servo:flex-experimental, r=pcwalton+SimonSapin

@zentner-kyle

Initial (very rudimentary) flexbox implementation. This is #7154 with two additional commits (that I did rather than ask @zentner-kyle to do it because it was a bit tricky.) r? @pcwalton for the last two commit r=me+pcwalton in #7154 for earlier commits.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7312)


        Auto merge of #7312 - servo:flex-experimental, r=SimonSapin

@zentner-kyle

Initial (very rudimentary) flexbox implementation. This is #7154 with two additional commits (that I did rather than ask @zentner-kyle to do it because it was a bit tricky.) r? @pcwalton for the last two commit r=me+pcwalton in #7154 for earlier commits.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7312)


        Auto merge of #7312 - servo:flex-experimental, r=SimonSapin

@zentner-kyle

Initial (very rudimentary) flexbox implementation. This is #7154 with two additional commits (that I did rather than ask @zentner-kyle to do it because it was a bit tricky.) r? @pcwalton for the last two commit r=me+pcwalton in #7154 for earlier commits.  [<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/servo/7312)

bors-servo · 2015-08-22T00:03:46Z

☔ The latest upstream changes (presumably #7312) made this pull request unmergeable. Please resolve the merge conflicts.

zentner-kyle · 2015-08-22T02:27:07Z

This is effectively merged due to #7312, so I'm closing this.

Thanks Simon and Patrick!

highfive added the S-awaiting-review label Aug 11, 2015

zentner-kyle force-pushed the zentner-kyle:flexbox branch 2 times, most recently from 9c4783b to 47a6a0e Aug 11, 2015

pcwalton reviewed Aug 13, 2015
View changes

components/layout/flex.rs

main_mode: main_mode

};

this

This comment has been minimized.

Sign in to view

Copy link

pcwalton Aug 13, 2015
Contributor

No need for the this temporary; just write BlockFlow { ... } as the last expression.

pcwalton reviewed Aug 13, 2015
View changes

highfive assigned SimonSapin Aug 13, 2015

SimonSapin reviewed Aug 14, 2015
View changes

SimonSapin added S-needs-code-changes and removed S-awaiting-review labels Aug 16, 2015

highfive added S-needs-rebase and removed S-awaiting-merge labels Aug 17, 2015

Kyle Zentner added 2 commits Jul 18, 2015

Parse flex-direction CSS property.

5755d7e

Create FlexFlow's for 'display: flex' nodes.

3807ab0

This commit doesn't implement any flexbox behavior at all. It just constructs FlexFlow's, which act just like the BlockFlow from which they "inherit."

Kyle Zentner added 3 commits Aug 11, 2015

Implement FlexFlow::assign_inline_sizes().

13d1381

Implement FlexFlow::assign_block_size().

c5323c8

Add reftests for basic flexbox functionality.

Loading status checks…

4141ea9

These tests (flex_row_direction.html and flex_column_direction.html) are correct, but only test the small amount of current flexbox functionality.

zentner-kyle force-pushed the zentner-kyle:flexbox branch from 47a6a0e to 4141ea9 Aug 21, 2015

highfive added S-awaiting-review and removed S-needs-code-changes labels Aug 21, 2015

SimonSapin mentioned this pull request Aug 21, 2015

Initial (very rudimentary) flexbox implementation. #7312

Merged

highfive added S-needs-rebase and removed S-awaiting-merge labels Aug 22, 2015

zentner-kyle closed this Aug 22, 2015

shepmaster mentioned this pull request Jul 3, 2016

Basic `display: flex` layout is not supported #12191

Closed

servo / servo

Initial (very rudimentary) flexbox implementation. #7154

Initial (very rudimentary) flexbox implementation. #7154

zentner-kyle commented Aug 11, 2015

highfive commented Aug 11, 2015

highfive commented Aug 11, 2015

pcwalton commented Aug 11, 2015

zentner-kyle commented Aug 13, 2015

pcwalton commented Aug 13, 2015

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 commented Aug 13, 2015

SimonSapin commented Aug 13, 2015

jdm commented Aug 13, 2015

metajack commented Aug 13, 2015

SimonSapin commented Aug 13, 2015

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.

This comment has been minimized.

SimonSapin commented Aug 14, 2015

bors-servo commented Aug 17, 2015

paulrouget commented Aug 21, 2015

bors-servo commented Aug 22, 2015

zentner-kyle commented Aug 22, 2015

servo / servo

Join GitHub today

Initial (very rudimentary) flexbox implementation. #7154

Initial (very rudimentary) flexbox implementation. #7154

Conversation

zentner-kyle commented Aug 11, 2015

highfive commented Aug 11, 2015

highfive commented Aug 11, 2015

pcwalton commented Aug 11, 2015

zentner-kyle commented Aug 13, 2015

pcwalton commented Aug 13, 2015

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 commented Aug 13, 2015

SimonSapin commented Aug 13, 2015

jdm commented Aug 13, 2015

metajack commented Aug 13, 2015

SimonSapin commented Aug 13, 2015

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.

This comment has been minimized.

SimonSapin commented Aug 14, 2015

bors-servo commented Aug 17, 2015

paulrouget commented Aug 21, 2015

bors-servo commented Aug 22, 2015

zentner-kyle commented Aug 22, 2015