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

RESOLVED FIXED in Firefox 64

Status

()

enhancement
P3
normal
RESOLVED FIXED
10 months ago
9 months ago

People

(Reporter: boris, Assigned: boris)

Tracking

(Blocks 2 bugs)

Trunk
mozilla64
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox64 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

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
Assignee: nobody → boris.chiou
See Also: → 1122253
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
Flags: needinfo?(mats)
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.)
Flags: needinfo?(mats)
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.
Summary: getComputedStyle() of sizing properties on block-size dimension doesn't match spec if we use keywords → Handle max-content/min-content/fit-content/available keywords for block-size dimension properly in style system
(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
(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.
Summary: Handle max-content/min-content/fit-content/available keywords for block-size dimension properly in style system → Handle max-content/min-content/fit-content/available keywords for block-size dimension properly
Blocks: 1322780
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.
Move MozLength and MaxLength into generics, and drop the manual implementation
of ToComputedValue.

Depends on D8290
Attachment #9015387 - Attachment is obsolete: true
Attachment #9015388 - Attachment is obsolete: true
Pushed by bchiou@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/7afc10b1bea1
https://hg.mozilla.org/mozilla-central/rev/963840088024
Status: NEW → RESOLVED
Closed: 9 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Depends on: 1500627
You need to log in before you can comment on or make changes to this bug.