Closed Bug 1496558 Opened 4 years ago Closed 4 years ago
Handle max-content/min-content/fit-content/available keywords for block-size dimension properly
46 bytes, text/x-phabricator-request
|Details | Review|
46 bytes, text/x-phabricator-request
|Details | Review|
We haven't supported block-size dimension for keywords (Bug 567039), but at least we could make the getComputedStyle() return the correct results. In  and , 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 , 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.  https://searchfox.org/mozilla-central/rev/924e3d96d81a40d2f0eec1db5f74fc6594337128/servo/components/style/values/computed/length.rs#958-969  https://searchfox.org/mozilla-central/rev/924e3d96d81a40d2f0eec1db5f74fc6594337128/servo/components/style/values/computed/length.rs#1025,1087  https://drafts.csswg.org/cssom/#resolved-values
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` : 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.  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
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.)
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 , is also needed to be updated because it seems it doesn't really mean we are in the inline axis, I guess.  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
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
Pushed by email@example.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
You need to log in before you can comment on or make changes to this bug.