1105111 - Implement "flex-basis: content" keyword for flexbox

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Daniel Holbert [:dholbert]]

In the flexbox spec, "flex-basis" now accepts the keyword "content", which "indicates automatic sizing, based on the flex item’s content."

Brief history:
 * originally, "flex-basis:auto" meant "look at my width or height property".
 * Then, flex-basis:auto was changed to mean automatic-sizing, and "main-size" was introduced as the "look at my width or height property" keyword. I implemented this in bug 1032922.
 * Then, that change was reverted (which I'm doing in bug 1093316), so "auto" once again means "look at my width or height property"; and a new 'content' keyword is being introduced to trigger automatic sizing.  (This bug here covers adding that keyword).

Comment 1

[yiorsi]

This property is now stable?

Comment 2

[Daniel Holbert [:dholbert]]

I'd consider the flex-basis property stable, yes.  (Though we don't implement its "content" keyword yet; that's what this bug covers).

(I don't think the spec authors are planning on adding any more special values for flex-basis, or changing the meaning of existing keywords again, if that's what you're asking.)

Comment 3

[yisibl]

(In reply to Daniel Holbert [:dholbert] (offline 2/18-2/21) from comment #2)
> I'd consider the flex-basis property stable, yes.  (Though we don't
> implement its "content" keyword yet; that's what this bug covers).
> 
> (I don't think the spec authors are planning on adding any more special
> values for flex-basis, or changing the meaning of existing keywords again,
> if that's what you're asking.)

Edge support 'content' keyword.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: Bug 1105111 part 1: Add support for 'flex-basis:content' in the style system (servo side).

(Patch by emilio, except for the line in gecko.mako.rs with...
  CoordDataValue::Enumerated(structs::NS_STYLE_FLEX_BASIS_CONTENT)
...which is by dholbert to integrate into gecko changes in next patch.)

Review commit: https://reviewboard.mozilla.org/r/232430/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/232430/

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: Bug 1105111 part 2: Add support for 'flex-basis:content' in the style system (gecko / getComputedStyle side).

Review commit: https://reviewboard.mozilla.org/r/232432/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/232432/

Comment 6

[Daniel Holbert [:dholbert]]

Attachment: Bug 1105111 part 3: Add support for 'flex-basis:content' in layout.

BACKGROUND:
Early in flex layout, we have to resolve the 'flex-basis' value to produce the
"flex base size" (basically, the flex-basis resolved to an absolute length).

This resolution happens in two "phases" (which both happen within
nsFlexContainer::GenerateFlexItemForChild()):

First phase: we try to resolve the flex-basis by creating a ReflowInput for the
flex item (which gets us some other things as well).  Under the hood, we use
the flex-basis when resolving this ReflowInput's main-axis size.  The code for
this lives in nsFrame::ComputeSize (and in
nsFrame::ComputeSizeWithIntrinsicDimensions, via some frame classes' overrides of
ComputeSize).

Second phase: If the first phase didn't get us a definite size, then that means
we have to do reflow to measure the content size & produce a resolved flex base
size, which we do via ResolveAutoFlexBasisAndMinSize().

NOTES ON THIS PATCH:
To add 'flex-basis:content' support to layout, this patch only needs to modify
the first phase discussed above. If it turns out we also have some second-phase
work to do (i.e. if we need to do reflow to resolve 'flex-basis:content'), this
patch causes that reflow to happen by simply making us use eStyleUnit_Auto in
the main axis's nsStyleCoord in the first phase.  (And then, if that 'auto'
nsStyleCoord really does require reflow, then that first phase will end up
producing an unconstrained main-size in the flex item's ReflowInput, which will
automatically trigger the second phase.)

Review commit: https://reviewboard.mozilla.org/r/232434/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/232434/

Comment 7

[Daniel Holbert [:dholbert]]

Attachment: Bug 1105111 part 4: Add reftests for 'flex-basis:content' in row-oriented flex container.

Review commit: https://reviewboard.mozilla.org/r/232436/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/232436/

Comment 8

[Daniel Holbert [:dholbert]]

Attachment: Bug 1105111 part 5: Add reftests for 'flex-basis:content' in column-oriented flex container.

Review commit: https://reviewboard.mozilla.org/r/232438/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/232438/

Comment 9

[Mats Palmgren (inactive)]

Comment on attachment 8963491 [details]
Bug 1105111 part 2: Add support for 'flex-basis:content' in the style system (gecko / getComputedStyle side).

https://reviewboard.mozilla.org/r/232432/#review237998

::: layout/style/nsCSSProps.cpp:1181
(Diff revision 1)
> +// This must be the same as kWidthKTable, but just with 'content' added:
> +const KTableEntry nsCSSProps::kFlexBasisKTable[] = {

Perhaps we could static_assert that the number of entries in this table is one more than kWidthKTable to enforce that?

Comment 10

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8963490 [details]
Bug 1105111 part 1: Add support for 'flex-basis:content' in the style system (servo side).

https://reviewboard.mozilla.org/r/232430/#review238010

::: servo/components/style/properties/gecko.mako.rs:1785
(Diff revision 1)
> +
> +    pub fn clone_flex_basis(&self) -> ComputedFlexBasis {
> +        use values::computed::MozLength;
> +
> +        FlexBasis::Width(
> +            MozLength::from_gecko_style_coord(&self.gecko.mFlexBasis).unwrap()

This needs to handle `Content` too. I'd recomend implementing `from_gecko_style_coord` on `FlexBasis` directly for that.

I'm happy to fix it up for you if you want, just let me know.

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 8963492 [details]
Bug 1105111 part 3: Add support for 'flex-basis:content' in layout.

https://reviewboard.mozilla.org/r/232434/#review238018

::: layout/generic/nsContainerFrame.cpp:858
(Diff revision 1)
> +        (IsFlexItem() &&
> +         nsFlexContainerFrame::IsItemInlineAxisMainAxis(this) &&
> +         pos->mFlexBasis.GetUnit() == eStyleUnit_Enumerated &&
> +         pos->mFlexBasis.GetIntValue() == NS_STYLE_FLEX_BASIS_CONTENT)) {

Looking at IsFlexItem() and IsItemInlineAxisMainAxis (https://hg.mozilla.org/try/rev/180ae618b76817e1a6dfc497631602450c773b21), I think it would be more performant to move the two pos->mFlexBasis conditions first.  I assuming flex-basis:content is rare given its not the intial value.

::: layout/generic/nsFrame.cpp:5640
(Diff revision 1)
> +        (flexMainAxis == eLogicalAxisInline ?
> +         inlineStyleCoord : blockStyleCoord) = &autoStyleCoord;

nit: it might be more readable, and avoids repeating the same code twice, if you declare a reference before the if-else block, like so:
auto& mainAxisCoord =
  flexMainAxis == eLogicalAxisInline ? inlineStyleCoord : blockStyleCoord;

::: layout/generic/nsFrame.cpp:5895
(Diff revision 1)
> +          (flexMainAxis == eLogicalAxisInline ?
> +           inlineStyleCoord : blockStyleCoord) = &autoStyleCoord;

ditto

Comment 12

[Mats Palmgren (inactive)]

Comment on attachment 8963493 [details]
Bug 1105111 part 4: Add reftests for 'flex-basis:content' in row-oriented flex container.

https://reviewboard.mozilla.org/r/232436/#review238030

::: layout/reftests/w3c-css/submitted/flexbox/flexbox-flex-basis-content-001-ref.html:11
(Diff revision 1)
> +<html>
> +<head>
> +  <title>CSS Reftest Reference</title>
> +  <meta charset="utf-8">
> +  <link rel="author" title="Daniel Holbert" href="mailto:[e-mail]">
> +  <link rel="stylesheet" type="text/css" href="support/ahem.css">

FYI, including a local Ahem font like this goes against WPT guidelines so I think we should probably move away from doing this:
http://web-platform-tests.org/writing-tests/general-guidelines.html

Also, Ahem usage guidelines recommends only using multiples of 5px for font-size:
http://web-platform-tests.org/writing-tests/ahem.html

Not a big deal though...

Comment 13

[Mats Palmgren (inactive)]

Comment on attachment 8963494 [details]
Bug 1105111 part 5: Add reftests for 'flex-basis:content' in column-oriented flex container.

https://reviewboard.mozilla.org/r/232438/#review238032

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8963490 [details]
Bug 1105111 part 1: Add support for 'flex-basis:content' in the style system (servo side).

https://reviewboard.mozilla.org/r/232430/#review238010

> This needs to handle `Content` too. I'd recomend implementing `from_gecko_style_coord` on `FlexBasis` directly for that.
> 
> I'm happy to fix it up for you if you want, just let me know.

If you wouldn't mind, that'd be great! Then I can update this patch with your fix and keep you as the author, and not feel bad about potentially attributing my own iffy rust code to you. :D

(Also: I'm not sure any of our tests caught this deficiency -- do you know what sort of test we'd need to exercise this codepath?)

Comment 15

[Daniel Holbert [:dholbert]]

Comment on attachment 8963491 [details]
Bug 1105111 part 2: Add support for 'flex-basis:content' in the style system (gecko / getComputedStyle side).

https://reviewboard.mozilla.org/r/232432/#review237998

> Perhaps we could static_assert that the number of entries in this table is one more than kWidthKTable to enforce that?

This is a great idea! I'll put that static_assert in nsComputedDOMStyle::DoGetFlexBasis() (which is the only place we use this table, in the stylo-only world).

Comment 16

[Mats Palmgren (inactive)]

BTW, do we have tests that checks that 'flex-basis' does NOT affect abs.pos. children?
If not, it might be worth adding here or in a follow-up bug...

Comment 17

[Emilio Cobos Álvarez (:emilio)]

(In reply to Daniel Holbert [:dholbert] from comment #14)
> > This needs to handle `Content` too. I'd recomend implementing `from_gecko_style_coord` on `FlexBasis` directly for that.
> > 
> > I'm happy to fix it up for you if you want, just let me know.
> 
> If you wouldn't mind, that'd be great! Then I can update this patch with
> your fix and keep you as the author, and not feel bad about potentially
> attributing my own iffy rust code to you. :D
> 
> (Also: I'm not sure any of our tests caught this deficiency -- do you know
> what sort of test we'd need to exercise this codepath?)

I think as of right now this is only used for animation stuff. So I'd expect animating between flex-basis: normal and flex-basis: normal or similar would trigger a panic.

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 8963493 [details]
Bug 1105111 part 4: Add reftests for 'flex-basis:content' in row-oriented flex container.

https://reviewboard.mozilla.org/r/232436/#review238030

> FYI, including a local Ahem font like this goes against WPT guidelines so I think we should probably move away from doing this:
> http://web-platform-tests.org/writing-tests/general-guidelines.html
> 
> Also, Ahem usage guidelines recommends only using multiples of 5px for font-size:
> http://web-platform-tests.org/writing-tests/ahem.html
> 
> Not a big deal though...

RE moving away from using @font-face with a local copy of Ahem -- I filed bug 1450095 on this. (I don't know if we currently have a better solution for this that plays nicely with our own reftest harness; we can discuss further over there.)

The 5px thing is a good point - thanks! My font-size choices were arbitrary in this group of tests, and I can easily make them multiples of 5px.

Comment 19

[Emilio Cobos Álvarez (:emilio)]

Attachment: Updated part 1.

Comment 20

[Daniel Holbert [:dholbert]]

With "part 1" and "part 2" applied on top of mozilla-central (no other patches needed, I think), it looks like we fail some non-negative clamping tests for flex-basis CSS transitions:
> failed | length-valued property flex-basis: clamping of negatives - got "-68.3px", expected "0px"
> failed | percent-valued property flex-basis: clamping of negatives - got "-102.452%", expected "0%"

I'm guessing that's due to a bug in part 1 (and I think the initial version of part 1 was probably affected, too, actually, though I hadn't noticed it until now because I hadn't thought to run this transitions test).  emilio, sorry to poke you again -- would you mind taking a look?

(This is from the functions test_length_clamped() and test_percent_clamped(), which we call for 'width', 'flex-basis', and various other length-valued properties to ensure that they are clamped to 0 if we produce an interpolated value that's negative. It seems the current stylo changes in Part 1 might be loosening that restriction on flex-basis, by accident.)

Comment 21

[Emilio Cobos Álvarez (:emilio)]

Attachment: Updated part 1 w/ transition clamping fix.

Comment 22

[Xidorn Quan [:xidorn] UTC+10]

Comment on attachment 8963823 [details] [diff] [review]
Updated part 1 w/ transition clamping fix.

Review of attachment 8963823 [details] [diff] [review]:
-----------------------------------------------------------------

::: servo/components/style/gecko/values.rs
@@ +77,5 @@
> +        if let Some(width) = MozLength::from_gecko_style_coord(coord) {
> +            return Some(FlexBasis::Width(width))
> +        }
> +
> +        if let CoordDataValue::Enumerated(structs::NS_STYLE_FLEX_BASIS_CONTENT) = coord.as_value() {

I don't think you need `let` here, just check equality should work?

::: servo/components/style/values/generics/flex.rs
@@ -10,5 @@
>  #[cfg_attr(feature = "servo", derive(MallocSizeOf))]
> -#[derive(Clone, Copy, Debug, PartialEq, ToComputedValue, ToCss)]
> -pub enum FlexBasis<LengthOrPercentage> {
> -    /// `auto`
> -    Auto,

Change to this probably means you also need to modify Servo's layout code in layout/flex.rs.

::: servo/components/style/values/specified/flex.rs
@@ +10,5 @@
>  use values::generics::flex::FlexBasis as GenericFlexBasis;
> +
> +/// The `width` value type.
> +#[cfg(feature = "servo")]
> +pub use ::values::specified::LengthOrPercentageOrAuto as Width;

Use `pub type` instead of `pub use` like what is done for computed value?

@@ +14,5 @@
> +pub use ::values::specified::LengthOrPercentageOrAuto as Width;
> +
> +/// The `width` value type.
> +#[cfg(feature = "gecko")]
> +pub use ::values::specified::MozLength as Width;

Ditto.

@@ +24,5 @@
>      fn parse<'i, 't>(
>          context: &ParserContext,
> +        input: &mut Parser<'i, 't>,
> +    ) -> Result<Self, ParseError<'i>> {
> +        if let Ok(width) = input.try(|i| Width::parse(context, i)) {

So... it seems `MozLength` only parses non-negative, but `LengthOrPercentageOrAuto` doesn't. Wouldn't this break Servo's parsing?

You probably should add a `parse_non_negative` to `MozLength` as an alias of `parse` and use it here. Preferably also add document to `MozLength` stating that it only accepts non-negative values.

Comment 23

[Emilio Cobos Álvarez (:emilio)]

Attachment: Part 1 w/ servo changes.

Servo didn't animate properly, etc, etc... Simplified a bit the flex stuff while at it.

Comment 24

[Daniel Holbert [:dholbert]]

Comment on attachment 8963490 [details]
Bug 1105111 part 1: Add support for 'flex-basis:content' in the style system (servo side).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232430/diff/1-2/

Comment 25

[Daniel Holbert [:dholbert]]

Comment on attachment 8963491 [details]
Bug 1105111 part 2: Add support for 'flex-basis:content' in the style system (gecko / getComputedStyle side).

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232432/diff/1-2/

Comment 26

[Daniel Holbert [:dholbert]]

Comment on attachment 8963492 [details]
Bug 1105111 part 3: Add support for 'flex-basis:content' in layout.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232434/diff/1-2/

Comment 27

[Daniel Holbert [:dholbert]]

Comment on attachment 8963493 [details]
Bug 1105111 part 4: Add reftests for 'flex-basis:content' in row-oriented flex container.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232436/diff/1-2/

Comment 28

[Daniel Holbert [:dholbert]]

Comment on attachment 8963494 [details]
Bug 1105111 part 5: Add reftests for 'flex-basis:content' in column-oriented flex container.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/232438/diff/1-2/

Comment 29

[Daniel Holbert [:dholbert]]

I posted an updated patch stack that addresses mats' review comments. (and includes emilio's updated patch)

(In reply to Mats Palmgren (:mats) from comment #16)
> BTW, do we have tests that checks that 'flex-basis' does NOT affect abs.pos.
> children?
> If not, it might be worth adding here or in a follow-up bug...

I'm not aware that we have such tests -- I filed bug 1450390 on that (in part).

(And as noted above, I spun off bug 1450095 on seeing if we can move away from using a local copy of Ahem.)

Comment 30

[Daniel Holbert [:dholbert]]

emilio, perhaps you could help me to coordinate the servo & gecko-landing parts here, on Monday?

(I assume "part 1" needs to land in servo & get merged over, and then we need to immediately land the rest on the same integration tree right after that merge, because part 1's gecko.rs changes include a dependency on NS_STYLE_FLEX_BASIS_CONTENT, which is added in part 2.)

Comment 31

[Emilio Cobos Álvarez (:emilio)]

(In reply to Daniel Holbert [:dholbert] from comment #30)
> emilio, perhaps you could help me to coordinate the servo & gecko-landing
> parts here, on Monday?

Sure thing :)

> (I assume "part 1" needs to land in servo & get merged over, and then we
> need to immediately land the rest on the same integration tree right after
> that merge, because part 1's gecko.rs changes include a dependency on
> NS_STYLE_FLEX_BASIS_CONTENT, which is added in part 2.)

Yup, that sounds right.

Comment 32

[Emilio Cobos Álvarez (:emilio)]

Err, that was meant to be a self-ni?

Comment 33

[Emilio Cobos Álvarez (:emilio)]

https://github.com/servo/servo/pull/20500

Comment 34

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/46097b1d0225
part 2: Add support for 'flex-basis:content' in the style system (gecko / getComputedStyle side). r=mats
https://hg.mozilla.org/integration/autoland/rev/4df15097883c
part 3: Add support for 'flex-basis:content' in layout. r=mats
https://hg.mozilla.org/integration/autoland/rev/59abb3d013ef
part 4: Add reftests for 'flex-basis:content' in row-oriented flex container. r=mats
https://hg.mozilla.org/integration/autoland/rev/16eaa1e05dac
part 5: Add reftests for 'flex-basis:content' in column-oriented flex container. r=mats

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3c7e71f5d134
Regenerate the devtools db on a CLOSED TREE. r=me

Comment 36

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/46097b1d0225
https://hg.mozilla.org/mozilla-central/rev/4df15097883c
https://hg.mozilla.org/mozilla-central/rev/59abb3d013ef
https://hg.mozilla.org/mozilla-central/rev/16eaa1e05dac
https://hg.mozilla.org/mozilla-central/rev/3c7e71f5d134

Comment 37

[Daniel Holbert [:dholbert]]

Thanks, Emilio!

Comment 38

[Will Bamberg [:wbamberg]]

I've updated the compat data for this: https://developer.mozilla.org/en-US/docs/Web/CSS/flex-basis#Browser_compatibility

Marking as dev-doc-complete, but please comment if you think we need anything else here.

Comment 39

[Chris Mills [:cmills]]

(In reply to Will Bamberg [:wbamberg] from comment #38)
> I've updated the compat data for this:
> https://developer.mozilla.org/en-US/docs/Web/CSS/flex-
> basis#Browser_compatibility
> 
> Marking as dev-doc-complete, but please comment if you think we need
> anything else here.

rel note added too:

https://developer.mozilla.org/en-US/Firefox/Releases/61#CSS