logical vs physical confusion when handling CSS min- and max-width and -height properties

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

(Blocks 1 bug)

unspecified
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

Assignee

Description

5 years ago
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).
Assignee

Comment 1

5 years ago
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.
Assignee

Comment 2

5 years ago
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.
Attachment #8529097 - Flags: review?(smontagu)
Assignee

Updated

5 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
Assignee

Comment 3

5 years ago
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...
Assignee

Comment 5

5 years ago
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
Attachment #8529097 - Flags: review?(smontagu) → review+
Assignee

Comment 7

5 years ago
Here are a couple of reftests based on the testcases above.
Attachment #8529733 - Flags: review?(smontagu)
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
Attachment #8529111 - Flags: review?(smontagu) → review+
Assignee

Comment 9

5 years ago
(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.
Attachment #8529733 - Flags: review?(smontagu) → review+
You need to log in before you can comment on or make changes to this bug.