Currently, while CSS 'width' and 'height' work physically (as expected), their min- and max- counterparts are confused: in vertical writing modes, they seem to get applied to both the inline and block dimensions of the frame. I think this is primarily a result of bug 1046950, which changed ComputeSize and friends to work with logical coordinates, but still calls the physical ComputeWidthValue, etc., from nsLayoutUtils. Given that these properties are *supposed* to remain entirely physical, I'm somewhat in two minds whether it's best to try and push the logicalization further down into the layoutUtils methods, or back away from that and do more of the ComputeSize calculations physically (while maintaining its overall logical API).

Note how in vertical mode, the examples with 'width' or 'height' specified for the inner <div> work fine; but 'max-width' gets applied to both width and height, and while 'max-height' correctly limits the height of the frame, the line-wrap within it does not respect this.

This turned out to be a lot less painful that I feared. I've changed nsLayoutUtils::ComputeWidthValue (which handles the min-/max- properties) to ComputeISizeValue, and similarly for other related methods; but I've left physically-named "wrappers" that simply call the new names, as there are a bunch of other users of these methods (particularly table layout code) that haven't been converted yet. Such code will of course not do the right thing in vertical mode until we logicalize it, too. When we do, we can remove the physical names altogether.

It looks like the same fix is needed in nsLayoutUtils::ComputeSizeWithIntrinsicDimensions, which is used for elements such as images; here's a simple testcase to demonstrate. This should not change at all when the writing mode is switched, but it does...

And here's the corresponding patch.

Comment on attachment 8529097 [details] [diff] [review] Fix the logical/physical confusion with min- and max- dimensions in nsFrame::ComputeSize() Review of attachment 8529097 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsFrame.cpp @@ +4236,2 @@ > if (result.BSize(aWM) != NS_UNCONSTRAINEDSIZE) { > + if (!nsLayoutUtils::IsAutoBSize(stylePos->mMaxHeight, aCBSize.BSize(aWM)) && Oops, missed one: s/stylePos->mMaxHeight/blockMax/ here.

Comment on attachment 8529097 [details] [diff] [review] Fix the logical/physical confusion with min- and max- dimensions in nsFrame::ComputeSize() Review of attachment 8529097 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/base/nsLayoutUtils.cpp @@ +4330,1 @@ > "have unconstrained width; this should only result from " Change "width" to "inline-size" in this line too. ::: layout/generic/nsFrame.cpp @@ +4183,5 @@ > *inlineStyleCoord); > } > > + const nsStyleCoord& inlineMax = > + isVertical ? stylePos->mMaxHeight : stylePos->mMaxWidth; Nit: the names {inline/block}{Max/Min} are inconsistent with most of our naming patterns and IMO less clear. I would prefer {min/max}{I/B}Size

Here are a couple of reftests based on the testcases above.

Comment on attachment 8529111 [details] [diff] [review] part 2 - Fix logical/physical confusion with min- and max- dimensions in nsLayoutUtils::ComputeSizeWithIntrinsicDimensions() Review of attachment 8529111 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the same naming nit as the previous patch

(In reply to Simon Montagu :smontagu from comment #6) > > + const nsStyleCoord& inlineMax = > > + isVertical ? stylePos->mMaxHeight : stylePos->mMaxWidth; > > Nit: the names {inline/block}{Max/Min} are inconsistent with most of our > naming patterns and IMO less clear. I would prefer {min/max}{I/B}Size I've made them {min/max}{I/B}SizeCoord, to clarify that these are nsStyleCoord values, not simple nscoord sizes. (And we already have at least one place where {min/max}{I/B}Size variables exist that are plain nscoord sizes.) Hope that's OK with you.

https://hg.mozilla.org/mozilla-central/rev/40e2635971ea https://hg.mozilla.org/mozilla-central/rev/92fa13bb42fa https://hg.mozilla.org/mozilla-central/rev/e1bf61449609