1106669 - nsLayoutUtils::IntrinsicForContainer needs to work with logical coordinates

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[Jonathan Kew [:jfkthame]]

nsLayoutUtils::IntrinsicForContainer is used when sizing a <button> to fit around an <img>, and this is why the buttons that contain images in bug 1106667's testcase don't size themselves properly.

Comment 1

[Jonathan Kew [:jfkthame]]

Attachment: Convert nsLayoutUtils::IntrinsicForContainer to work with logical coordinates

With this, those buttons size themselves properly.

Comment 2

[Jonathan Kew [:jfkthame]]

Attachment: Convert nsLayoutUtils::IntrinsicForContainer to work with logical coordinates

Actually, IntrinsicForContainer is primarily interested in the parent's writing mode, as it is an inline size in that WM that we're computing. If aFrame is orthogonal to its parent, it'd be its block-size that we need, but that probably isn't known yet... so we lose. But this is probably close enough for a start.

Comment 3

[Simon Montagu :smontagu]

Comment on attachment 8531682 [details] [diff] [review]
Convert nsLayoutUtils::IntrinsicForContainer to work with logical coordinates

Review of attachment 8531682 [details] [diff] [review]:
-----------------------------------------------------------------

Tests?

::: layout/base/nsLayoutUtils.cpp
@@ +4101,5 @@
> +    isVertical ? stylePos->mHeight : stylePos->mWidth;
> +  const nsStyleCoord &styleMinISize =
> +    isVertical ? stylePos->mMinHeight : stylePos->mMinWidth;
> +  const nsStyleCoord &styleMaxISize =
> +    isVertical ? stylePos->mMaxHeight : stylePos->mMaxWidth;

I think it's better to make this one if/else rather than three ternaries in a row with the same condition.

@@ +4185,5 @@
> +      isVertical ? stylePos->mWidth : stylePos->mHeight;
> +    const nsStyleCoord &styleMinBSize =
> +      isVertical ? stylePos->mMinWidth : stylePos->mMinHeight;
> +    const nsStyleCoord &styleMaxBSize =
> +      isVertical ? stylePos->mMaxWidth : stylePos->mMaxHeight;

Same here

@@ +4188,5 @@
> +    const nsStyleCoord &styleMaxBSize =
> +      isVertical ? stylePos->mMaxWidth : stylePos->mMaxHeight;
> +
> +    if (styleBSize.GetUnit() != eStyleUnit_Auto ||
> +        !(styleMinBSize.GetUnit() == eStyleUnit_Auto || 

Nit: remove trailing whitespace

Comment 4

[Jonathan Kew [:jfkthame]]

Attachment: Reftest for IntrinsicForContainer writing-mode handling

This seems to work OK as a testcase; it uses the sizing of a button to check whether it is using the intrinsic size of its contents properly.

Comment 5

[Jonathan Kew [:jfkthame]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d588126b6d92
https://hg.mozilla.org/integration/mozilla-inbound/rev/52a6bc20f60d

Comment 6

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8b7a6e2e4ce7
https://hg.mozilla.org/mozilla-central/rev/22b0680ca880