nsLayoutUtils::IntrinsicForContainer needs to work with logical coordinates

RESOLVED FIXED in mozilla37

Status

()

Core
Layout: Block and Inline
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

unspecified
mozilla37
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 1 obsolete attachment)

(Assignee)

Description

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

Comment 1

3 years ago
Created attachment 8531026 [details] [diff] [review]
Convert nsLayoutUtils::IntrinsicForContainer to work with logical coordinates

With this, those buttons size themselves properly.
Attachment #8531026 - Flags: review?(smontagu)
(Assignee)

Updated

3 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
(Assignee)

Comment 2

3 years ago
Created attachment 8531682 [details] [diff] [review]
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.
Attachment #8531682 - Flags: review?(smontagu)
(Assignee)

Updated

3 years ago
Attachment #8531026 - Attachment is obsolete: true
Attachment #8531026 - Flags: review?(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
Attachment #8531682 - Flags: review?(smontagu) → review+
(Assignee)

Comment 4

3 years ago
Created attachment 8531762 [details] [diff] [review]
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.
Attachment #8531762 - Flags: review?(smontagu)
Attachment #8531762 - Flags: review?(smontagu) → review+
https://hg.mozilla.org/mozilla-central/rev/8b7a6e2e4ce7
https://hg.mozilla.org/mozilla-central/rev/22b0680ca880
Status: ASSIGNED → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.