1436881 - Honor enumerated values for "flex-basis" (-moz-min-content, etc) more coherently

Status: RESOLVED FIXED

(Core :: Layout, enhancement, P3)

Description

[Daniel Holbert [:dholbert]]

nsFrame::ComputeSize[WithIntrinsicDimensions] has some special cases for flex-basis: -moz-min-content that don't necessarily make sense anymore, now that we (sort of) support these keywords for 'height' per bug 1122253.

Filing this bug to rethink how this should work.  We might be able to get rid of the special cases entirely, or we might need to keep them around but just make them specific to the logical block axis rather than the vertical axis.

Comment 1

[Daniel Holbert [:dholbert]]

I'm talking specifically about these clauses:
https://searchfox.org/mozilla-central/rev/a5abf843f8fac8530aa5de6fb40e16547bb4a47a/layout/generic/nsFrame.cpp#5654-5666
https://searchfox.org/mozilla-central/rev/a5abf843f8fac8530aa5de6fb40e16547bb4a47a/layout/generic/nsFrame.cpp#5908-5920

(...which are getting rewritten slightly in bug 1267462 but still do basically the same thing).

Comment 2

[Mats Palmgren (inactive)]

Yeah, I think bug 567039 is really about bsize rather than physical height.

Comment 3

[Jet Villegas (inactive)]

[ Triage 2017/02/20: P3 ]

Comment 4

[Daniel Holbert [:dholbert]]

Actually, I think the special cases can just be removed from nsFrame::ComputeSize[WithIntrinsicDimensions].

I originally added these special cases on the theory that:
 - This code swaps in the computed "flex-basis" for "height" in some scenarios.
 - Therefore, this code can effectively make us behave as if we had "height:-moz-min-content".
 - "height:-moz-min-content" had been forever forbidden at the parser level, so it seems reasonable to be concerned that we might not be set up to handle it correctly here.
 - Therefore, to be on the safe side, we had this special case to preemptively try to substitute in "auto" instead in this case.

However: I just tested what happens if we remove these cases, and it looks like everything works out fine. Specifically, we *still* end up satisfying the "IsAutoBSize()" condition, which means we skip the ComputeBSizeValue(...) codepath and leave in-place the NS_UNCONSTRAINEDSIZE value that we got from  ComputeAutoSize() at the top of the method.

So I think we should just simplify this code and remove these special cases.

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: Bug 1436881: Remove redundant special-case code for treating flex-basis enum values as 'auto' in vertical axis.

This patch should not affect behavior.

Logic-wise: the idea behind this patch is to behave as if the
'usingFlexBasisForHeight' variable were always false, which in turn lets us
remove an "if (!usingFlexBasisForHeight || ...)" check, because it trivially
passes when that bool is false.

Background on this special case & why we can remove it:
=======================================================
In the original flexbox implementation, we had some special-case code to be
sure we didn't end up swapping in e.g. "flex-basis:-moz-min-content" for
"height" in these ComputeSize functions, because that was a scenario that
previously would've been prevented at the parser level (height:-moz-min-content
is rejected for now), and hence may not have ended up being handled robustly.

However, nowadays it'll be handled just fine in these functions, and will
produce the same result as our special-case exception tries to achieve.

In particular, for a nsFrame that is a flex item in a flex container with a
vertical main axis (the scenario that these special cases are catching):

 - If the (vertical) main axis is this nsFrame's inline axis (i.e. if this
   nsFrame has a vertical writing-mode), then Stylo actually converts
   enumerated flex-basis values like "-moz-min-content" to "auto" at the parser
   level.  So it's not actually possible to get a computed "flex-basis" of
   -moz-min-content, in that scenario.

 - Otherwise, i.e. if the (vertical) main axis is this nsFrame's block axis,
   then these ComputeSize functions will now end up getting an enumerated
   "blockStyleCoord" (really pointing to flexBasis), but that'll still end up
   being treated like 'auto'. This happens by virtue of ComputeSize's calls to
   ComputeAutoSize (which initializes the tentative bsize value to
   NS_UNCONSTRAINEDSIZE) and to nsLayoutUtils::IsAutoBSize (which returns
   "true" for eStyleUnit_Enumerated values and then makes us leave the
   ComputeAutoSize result unperturbed).

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

Comment 6

[Mats Palmgren (inactive)]

Comment on attachment 8963436 [details]
Bug 1436881: Remove redundant special-case code for treating flex-basis enum values as 'auto' in vertical axis.

https://reviewboard.mozilla.org/r/232342/#review237994

Comment 7

[Emilio Cobos Álvarez (:emilio)]

Comment on attachment 8963436 [details]
Bug 1436881: Remove redundant special-case code for treating flex-basis enum values as 'auto' in vertical axis.

https://reviewboard.mozilla.org/r/232342/#review237996

::: commit-message-6aa3b:26
(Diff revision 1)
> +In particular, for a nsFrame that is a flex item in a flex container with a
> +vertical main axis (the scenario that these special cases are catching):
> +
> + - If the (vertical) main axis is this nsFrame's inline axis (i.e. if this
> +   nsFrame has a vertical writing-mode), then Stylo actually converts
> +   enumerated flex-basis values like "-moz-min-content" to "auto" at the parser

Minor correction: We can't really do this at parse time, because we don't know the writing mode beforehand. This is done at computed value time in:

https://searchfox.org/mozilla-central/rev/7e663b9fa578d425684ce2560e5fa2464f504b34/servo/components/style/values/computed/length.rs#988

Comment 8

[Daniel Holbert [:dholbert]]

Gotcha - thanks for clarifying! I'll tweak that bit of the commit message before landing.

Comment 9

[Daniel Holbert [:dholbert]]

Attachment: Bug 1436881: Remove redundant special-case code for treating flex-basis enum values as 'auto' in vertical axis.

This patch should not affect behavior.

Logic-wise: the idea behind this patch is to behave as if the
'usingFlexBasisForHeight' variable were always false, which in turn lets us
remove an "if (!usingFlexBasisForHeight || ...)" check, because it trivially
passes when that bool is false.

Background on this special case & why we can remove it:
=======================================================
In the original flexbox implementation, we had some special-case code to be
sure we didn't end up swapping in e.g. "flex-basis:-moz-min-content" for
"height" in these ComputeSize functions, because that was a scenario that
previously would've been prevented at the parser level (height:-moz-min-content
is rejected for now), and hence may not have ended up being handled robustly.

However, nowadays it'll be handled just fine in these functions, and will
produce the same result as our special-case exception tries to achieve.

In particular, for a nsFrame that is a flex item in a flex container with a
vertical main axis (the scenario that these special cases are catching):

 - If the (vertical) main axis is this nsFrame's inline axis (i.e. if this
   nsFrame has a vertical writing-mode), then Stylo actually converts
   enumerated flex-basis values like "-moz-min-content" to "auto" when
   producing the computed values that layout sees.  So it's not actually
   possible for layout to see a computed "flex-basis" of -moz-min-content, in
   that scenario.

 - Otherwise, i.e. if the (vertical) main axis is this nsFrame's block axis,
   then these ComputeSize functions will now end up getting an enumerated
   "blockStyleCoord" (really pointing to flexBasis), but that'll still end up
   being treated like 'auto'. This happens by virtue of ComputeSize's calls to
   ComputeAutoSize (which initializes the tentative bsize value to
   NS_UNCONSTRAINEDSIZE) and to nsLayoutUtils::IsAutoBSize (which returns
   "true" for eStyleUnit_Enumerated values and then makes us leave the
   ComputeAutoSize result unperturbed).

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

Comment 10

[Daniel Holbert [:dholbert]]

Gah, I'm not sure why mozreview obsoleted the old patch rather than treating this as an update + interdiff.  The only change was this, to the commit message, to address comment 7:
======
  - If the (vertical) main axis is this nsFrame's inline axis (i.e. if this
    nsFrame has a vertical writing-mode), then Stylo actually converts
-   enumerated flex-basis values like "-moz-min-content" to "auto" at the parser
-   level.  So it's not actually possible to get a computed "flex-basis" of
-   -moz-min-content, in that scenario.
+   enumerated flex-basis values like "-moz-min-content" to "auto" when
+   producing the computed values that layout sees.  So it's not actually
+   possible for layout to see a computed "flex-basis" of -moz-min-content, in
+   that scenario.
======

I'll just land on inbound and not bother trying to make MozReview happy.

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e974a8ab639e
Remove redundant special-case code for treating flex-basis enum values as 'auto' in vertical axis. r=mats

Comment 12

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/e974a8ab639e