Closed Bug 1105111 Opened 10 years ago Closed 7 years ago

Implement "flex-basis: content" keyword for flexbox

Categories

(Core :: Layout, defect)

Product:
Core
Component:
Layout
Platform:
x86_64
Linux
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: dev-doc-complete, DevAdvocacy, Whiteboard: [DevRel:P1])

Attachments

(6 files, 2 obsolete files)

Bug 1105111 part 1: Add support for 'flex-basis:content' in the style system (servo side).
59 bytes, text/x-review-board-request
Details
Bug 1105111 part 2: Add support for 'flex-basis:content' in the style system (gecko / getComputedStyle side).
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1105111 part 3: Add support for 'flex-basis:content' in layout.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1105111 part 4: Add reftests for 'flex-basis:content' in row-oriented flex container.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Bug 1105111 part 5: Add reftests for 'flex-basis:content' in column-oriented flex container.
59 bytes, text/x-review-board-request
MatsPalmgren_bugz
: review+
Details
Part 1 w/ servo changes.
29.54 KB, patch
xidorn
: review+
Details | Diff | Splinter Review
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).
Blocks: flexbox-spec-changes
Depends on: 1093316, 1032922
This property is now stable?
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.)
(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: ? → ---
Blocks: 1374540
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Depends on: 1449838
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 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 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 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:[email protected]"> > + <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 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+
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 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).
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.
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.
Attached 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)
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+
Attachment #8963490 - Attachment is obsolete: false
Blocks: 1450390
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.)
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)
Pushed by [email protected]: 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
Pushed by [email protected]: https://hg.mozilla.org/integration/autoland/rev/3c7e71f5d134 Regenerate the devtools db on a CLOSED TREE. r=me
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.
Keywords: dev-doc-needed → dev-doc-complete
(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.

Attachment

General

Creator:
Created:
Updated:
Size: