Closed Bug 1436881 Opened 4 years ago Closed 4 years ago

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

Categories

(Core :: Layout, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: dholbert, Assigned: dholbert)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Version: 57 Branch → Trunk
Yeah, I think bug 567039 is really about bsize rather than physical height.
[ Triage 2017/02/20: P3 ]
Priority: -- → P3
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.
Assignee: nobody → dholbert
Status: NEW → ASSIGNED
Blocks: 1449838
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
Attachment #8963436 - Flags: review?(mats) → review+
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
Gotcha - thanks for clarifying! I'll tweak that bit of the commit message before landing.
Attachment #8963436 - Attachment is obsolete: true
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.
Attachment #8963722 - Flags: review?(mats)
Attachment #8963436 - Attachment is obsolete: false
Attachment #8963722 - Attachment is obsolete: true
Pushed by dholbert@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/e974a8ab639e
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.