1496558 - Handle max-content/min-content/fit-content/available keywords for block-size dimension properly

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement, P3)

Description

[Boris Chiou [:boris]]

We haven't supported block-size dimension for keywords (Bug 567039), but at least we could make the getComputedStyle() return the correct results. In [1] and [2], we make the computed value of sizing properties on block-axis _always_ be auto, so getComputedStyle doesn't return the correct keywords if |display:none|. (Note: According to spec [3], width and height should return computed values if |display:none|.)

This bug may update the style system and make sure that layout handles the keywords as default value (e.g. 0px or auto or none) on block-size dimension. We could leave the implementation in Bug 567039.

There is a comparison between Firefox and Chrome in Bug 1322780 Comment 22.

[1] https://searchfox.org/mozilla-central/rev/924e3d96d81a40d2f0eec1db5f74fc6594337128/servo/components/style/values/computed/length.rs#958-969
[2] https://searchfox.org/mozilla-central/rev/924e3d96d81a40d2f0eec1db5f74fc6594337128/servo/components/style/values/computed/length.rs#1025,1087
[3] https://drafts.csswg.org/cssom/#resolved-values

Comment 1

[Boris Chiou [:boris]]

Attachment: Bug 1496558 - Part 1: Store the correct computed values for keywords for sizing properties

In order to get the correct computed value of these keywords, we have to
make sure we store the correct computed values in sizing properties,
even for block size dimension. However, we haven't implement these
keywords for block size, so we have to make sure layout handle them
correctly.

Comment 2

[Boris Chiou [:boris]]

Attachment: Bug 1496558 - Part 2: Move MozLength and MaxLength into generics

Move MozLength and MaxLength into generics, and drop the manual implementation
of ToComputedValue.

Depends on D8043

Comment 3

[Boris Chiou [:boris]]

The part 1 is buggy because I didn't cover all necessary checks for the handle of keywords on block size dimension. I got two reftest failures related to `grid` [1]:
a) https://searchfox.org/mozilla-central/source/layout/reftests/css-grid/grid-auto-min-sizing-min-content-min-size-004.html
b) https://searchfox.org/mozilla-central/source/layout/reftests/css-grid/grid-item-auto-min-size-clamp-007.html

In (a) and (b), we use the unsupported keywords on `height` or `min-height`, and their results should look like `auto`. However, the part 1 tries to store the correct computed value of keywords for block size properties, and it seems there must be some places that I missed and we should handle the keywords correctly. I tried to search the potential places by `eStyleUnit_Auto`, but it seems this is not enough.

Hi Mats,

Sorry, I'm still not familiar with grid, and may need your hints or any possible entry point for fixing these two cases after applying the part 1. For these two failures, it seems we use an incorrect height/min-height if an element is in a grid element.


[1] https://hg.mozilla.org/mozilla-central/raw-file/tip/layout/tools/reftest/reftest-analyzer.xhtml#logurl=https://queue.taskcluster.net/v1/task/dgj3QkbySXKd9_pdhlMQEA/runs/0/artifacts/public/logs/live_backing.log&only_show_unexpected=1

Comment 4

[Mats Palmgren (inactive)]

I haven't reviewed the patch in detail, but this seems wrong
(in ShouldApplyAutoMinSize):
>const bool isInlineAxis = aContainerAxis == eLogicalAxisInline;
...
>	​(!isInlineAxis && size.GetUnit() == eStyleUnit_Enumerated);

The child (grid item) could be orthogonal to the grid container,
So what you want to test is if the given axis is the same as
the *child's* block-axis, something like:
isInlineAxis == aContainerWM.IsOrthogonalTo(mFrame->GetWritingMode()) && ...

That's probably not the cause of the Grid test failures though.
I think that problem is likely in nsLayoutUtils::IntrinsicForAxis:
https://searchfox.org/mozilla-central/rev/80ac71c1c54af788b32e851192dfd2de2ec18e18/layout/base/nsLayoutUtils.cpp#5311-5313
It assumes enumerated values never occur in the block-axis.

We could fix that by making 'styleISize' a local instance (rather
than a reference) and just explicitly set it to 'auto' (SetAutoValue())
if it's an "enum in the block-axis" value. (ditto for styleMinISize and
styleMaxISize I guess).  (Note that the "I" in the variable names here
are a bit confusing since they are not necessarily in the inline-axis.)

Comment 5

[Mats Palmgren (inactive)]

IIUC, the patch aims to implement the -moz-min/max-content values
correctly for all three properties in the block-axis, is that correct?
If so, then please update the summary of this bug since it's a more
general fix than what it currently says.

Comment 6

[Boris Chiou [:boris]]

(In reply to Mats Palmgren (:mats) from comment #4)
> I think that problem is likely in nsLayoutUtils::IntrinsicForAxis:
> https://searchfox.org/mozilla-central/rev/
> 80ac71c1c54af788b32e851192dfd2de2ec18e18/layout/base/nsLayoutUtils.cpp#5311-
> 5313
> It assumes enumerated values never occur in the block-axis.

Thanks for these hints. I could fix the test failures by the update you mentioned above.
Besides, `nsLayoutUtils::MinSizeContributionForAxis()`, which is used by compute the min-size
contribution for a grid item [1], is also needed to be updated because it seems it doesn't really
mean we are in the inline axis, I guess.

[1] https://searchfox.org/mozilla-central/rev/29aea2a2a3bd0f5e25ce0b60a76053fb25ba5149/layout/generic/nsGridContainerFrame.cpp#3755-3815

Comment 7

[Boris Chiou [:boris]]

(In reply to Mats Palmgren (:mats) from comment #5)
> IIUC, the patch aims to implement the -moz-min/max-content values
> correctly for all three properties in the block-axis, is that correct?
> If so, then please update the summary of this bug since it's a more
> general fix than what it currently says.

My plan in this bug is kind of "preparation" for bug 567039.
1. Store the correct computed values for sizing properties. (i.e. Keywords),
   so we could have the correct getComputedStyle().
2. Go through all the places we should update if we want to support keywords on block-size, and
   leave the comments there.

Looks like we have to update a lot of test cases after finishing bug 567039.
For now, I am trying to make sure we have the same behavior as the initial values on these properties.

Please feel free to update the title. Really thanks for these suggestions.

Comment 8

[Boris Chiou [:boris]]

Attachment: Bug 1496558 - Part 1: Store the correct computed values for keywords for sizing properties

In order to get the correct computed value of these keywords, we have to
make sure we store the correct computed values in sizing properties,
even for block size dimension. However, we haven't implement these
keywords for block size, so we have to make sure layout handle them
correctly.

Comment 9

[Boris Chiou [:boris]]

Attachment: Bug 1496558 - Part 2: Move MozLength and MaxLength into generics

Move MozLength and MaxLength into generics, and drop the manual implementation
of ToComputedValue.

Depends on D8290

Comment 10

[Boris Chiou [:boris]]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=766268dc491c4961f7e2746bb4b668689ab671cb

Comment 11

[Boris Chiou [:boris]]

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d8c80f6b4a40d2dda3049c65db723ac5b61df930

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7afc10b1bea1
Part 1: Store the correct computed values for keywords for sizing properties r=mats
https://hg.mozilla.org/integration/autoland/rev/963840088024
Part 2: Move MozLength and MaxLength into generics r=emilio

Comment 13

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/7afc10b1bea1
https://hg.mozilla.org/mozilla-central/rev/963840088024