Implement "flex-basis: content" keyword for flexbox

RESOLVED FIXED in Firefox 61

Status

()

defect
RESOLVED FIXED
5 years ago
Last year

People

(Reporter: dholbert, Assigned: dholbert)

Tracking

(Blocks 1 bug, {dev-doc-complete, DevAdvocacy})

Trunk
mozilla61
x86_64
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

(Whiteboard: [DevRel:P1], )

Attachments

(6 attachments, 2 obsolete attachments)

Assignee

Description

5 years ago
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).
Assignee

Updated

5 years ago

Comment 1

4 years ago
This property is now stable?
Assignee

Comment 2

4 years ago
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

3 years ago
(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.
Whiteboard: [DevRel:P1]
Flags: platform-rel?
platform-rel: --- → ?
platform-rel: ? → ---
Assignee

Updated

Last year
Blocks: 1374540
Assignee

Updated

Last year
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Assignee

Updated

Last year
Depends on: 1449838
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 9

Last year
mozreview-review
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?
Attachment #8963491 - Flags: review?(mats) → review+

Comment 10

Last year
mozreview-review
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

Last year
mozreview-review
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
Attachment #8963492 - Flags: review?(mats) → review+

Comment 12

Last year
mozreview-review
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:dholbert@mozilla.com">
> +  <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...
Attachment #8963493 - Flags: review?(mats) → review+

Comment 13

Last year
mozreview-review
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
Attachment #8963494 - Flags: review?(mats) → review+
Assignee

Comment 14

Last year
mozreview-review-reply
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?)
Assignee

Comment 15

Last year
mozreview-review-reply
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

Last year
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...
(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.
Assignee

Comment 18

Last year
mozreview-review-reply
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.
Posted patch Updated part 1. (obsolete) — Splinter Review
Attachment #8963490 - Attachment is obsolete: true
Attachment #8963490 - Flags: review?(xidorn+moz)
Attachment #8963761 - Flags: review?(xidorn+moz)
Assignee

Comment 20

Last year
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.)
Flags: needinfo?(emilio)
Flags: needinfo?(emilio)
Attachment #8963823 - Flags: review?(xidorn+moz)
Attachment #8963761 - Attachment is obsolete: true
Attachment #8963761 - Flags: review?(xidorn+moz)
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.
Attachment #8963823 - Flags: review?(xidorn+moz)
Servo didn't animate properly, etc, etc... Simplified a bit the flex stuff while at it.
Attachment #8963823 - Attachment is obsolete: true
Attachment #8963841 - Flags: review?(xidorn+moz)
Attachment #8963841 - Flags: review?(xidorn+moz) → review+
Assignee

Updated

Last year
Attachment #8963490 - Attachment is obsolete: false
Assignee

Updated

Last year
Blocks: 1450390
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Assignee

Comment 29

Last year
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.)
Assignee

Comment 30

Last year
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.)
(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.
Flags: needinfo?(dholbert)
Err, that was meant to be a self-ni?
Flags: needinfo?(dholbert) → needinfo?(emilio)

Comment 34

Last year
Pushed by ecoal95@gmail.com:
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

Last year
Pushed by ecoal95@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/3c7e71f5d134
Regenerate the devtools db on a CLOSED TREE. r=me
Assignee

Comment 37

Last year
Thanks, Emilio!
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.
(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
You need to log in before you can comment on or make changes to this bug.