Bug 105264 – Percentage min/max width replaced element may incorrectly rendered

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[KyungTae Kim]

If min-width or max-width of replaced element is percentage, and the containing block try to fit to it,
the computation of preferred width of the containing block can be wrong.

In RenderReplaced::computeMaxPreferredLogicalWidth, the "percentage width" case is handled,
but  the "percentage min/max width" is not handled now.

With bug 102784, the bug that the change of "percentage width" image's intrinsic size is not updated is fixed,
but the "percentage min-width" image is not updated despite re-layout because the not updated preferred width.

[KyungTae Kim]

Created attachment 179904 [details]
Patch

[Tony Chang]

Comment on attachment 179904 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=179904&action=review

Hmm, my comment was lost.

> Source/WebCore/rendering/RenderReplaced.cpp:448
> -    if (logicalWidth.isPercent())
> +    if (logicalWidth.isPercent() || style()->logicalMaxWidth().isPercent() || style()->logicalMinWidth().isPercent())

I don't think this is correct.  If the width is fixed and the min/max width is a percent, the preferred size is the width.  Basically, we want to treat any percent values as auto when computing the preferred width.

> Source/WebCore/rendering/RenderReplaced.cpp:451
>      // FIXME: We shouldn't be calling a logical width computing function in preferred

The correct fix is to handle this FIXME. We shouldn't be calling computeReplacedLogicalWidth, we should be doing our own calculation here (should be similar to the computeReplacedLogicalWidth* methods but have Percent and Calculated fall through to intrinsicLogicalWidth()).  You might even be able to remove the logicalWidth.isPercent() check above.

[KyungTae Kim]

Created attachment 180091 [details]
Patch

[KyungTae Kim]

(In reply to comment #2)
> (From update of attachment 179904 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=179904&action=review
> 
> Hmm, my comment was lost.
> 
> > Source/WebCore/rendering/RenderReplaced.cpp:448
> > -    if (logicalWidth.isPercent())
> > +    if (logicalWidth.isPercent() || style()->logicalMaxWidth().isPercent() || style()->logicalMinWidth().isPercent())
> 
> I don't think this is correct.  If the width is fixed and the min/max width is a percent, the preferred size is the width.  Basically, we want to treat any percent values as auto when computing the preferred width.
> 
> > Source/WebCore/rendering/RenderReplaced.cpp:451
> >      // FIXME: We shouldn't be calling a logical width computing function in preferred
> 
> The correct fix is to handle this FIXME. We shouldn't be calling computeReplacedLogicalWidth, we should be doing our own calculation here (should be similar to the computeReplacedLogicalWidth* methods but have Percent and Calculated fall through to intrinsicLogicalWidth()).  You might even be able to remove the logicalWidth.isPercent() check above.

Because what we want is not to use "percent min width" when calculating preferred width,
I added a parameter for that in computeReplacedLogicalWidth and functions called by it.

[Build Bot]

Comment on attachment 180091 [details]
Patch

Attachment 180091 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15399589

[KyungTae Kim]

Created attachment 180106 [details]
Patch

[KyungTae Kim]

Created attachment 180107 [details]
Patch

[Tony Chang]

Comment on attachment 180107 [details]
Patch

I was curious about includeMaxWidth, so I tracked this down to http://trac.webkit.org/changeset/35828 .  It was added for the same reason includeMinWidth is being added.

I think having 2 args is confusing.  Instead, we could have a single enum: ComputePreferred, ComputeActual (default to ComputeActual).  In computeReplacedLogicalWidthRespectingMinMaxWidth, if the enum is ComputePreferred, we can ignore percent min or max widths.

Even with that change, I think RenderReplaced::computeMaxPreferredLogicalWidth is still incorrect.  If the width is a percent but the min-width is greater than the intrinsic logical width, the preferred size should be the min-width.  I guess that's a separate bug.

[KyungTae Kim]

Comment on attachment 180107 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180107&action=review

(In reply to comment #8)
> (From update of attachment 180107 [details])
> I was curious about includeMaxWidth, so I tracked this down to http://trac.webkit.org/changeset/35828 .  It was added for the same reason includeMinWidth is being added.
> 
> I think having 2 args is confusing.  Instead, we could have a single enum: ComputePreferred, ComputeActual (default to ComputeActual).  In computeReplacedLogicalWidthRespectingMinMaxWidth, if the enum is ComputePreferred, we can ignore percent min or max widths.
Is separated control really not needed? Some callers use includeMaxWidth differently now.

> Source/WebCore/rendering/RenderReplaced.cpp:-368
> -                return computeReplacedLogicalWidthRespectingMinMaxWidth(roundToInt(round(logicalHeight * intrinsicRatio)));

In here, the includeMaxWidth is always true now.

> Source/WebCore/rendering/RenderReplaced.cpp:-379
> -                    logicalWidth = blockWithWidth->computeReplacedLogicalWidthRespectingMinMaxWidth(blockWithWidth->computeReplacedLogicalWidthUsing(MainOrPreferredSize, blockWithWidth->style()->logicalWidth()), false);

In here, the includeMaxWidth is always false now.

[Tony Chang]

(In reply to comment #9)
> Is separated control really not needed? Some callers use includeMaxWidth differently now.

I'm not sure if they need to be separate or if we just have poor test coverage.  Do you know which tests cover these cases?

[KyungTae Kim]

(In reply to comment #10)
> (In reply to comment #9)
> > Is separated control really not needed? Some callers use includeMaxWidth differently now.
> 
> I'm not sure if they need to be separate or if we just have poor test coverage.  Do you know which tests cover these cases?
As I searched, the code originated from https://bugs.webkit.org/show_bug.cgi?id=15849
And the test cases for that are: 
css2.1/20110323/replaced-intrinsic-ratio-001.htm
css2.1/20110323/replaced-intrinsic-003.htm
css2.1/20110323/block-replaced-height-007.htm
css2.1/20110323/float-replaced-height-007.htm
css2.1/20110323/inline-block-replaced-height-007.htm
css2.1/20110323/inline-replaced-height-007.htm

[Tony Chang]

(In reply to comment #11)
> (In reply to comment #10)
> > (In reply to comment #9)
> > > Is separated control really not needed? Some callers use includeMaxWidth differently now.
> > 
> > I'm not sure if they need to be separate or if we just have poor test coverage.  Do you know which tests cover these cases?
> As I searched, the code originated from https://bugs.webkit.org/show_bug.cgi?id=15849

I don't see any calls to computeReplacedLogicalWidthRespectingMinMaxWidth with 'false' passed in for includeMaxWidth.

There are plenty of calls in computeReplacedLogicalWidth to computeReplacedLogicalWidthRespectingMinMaxWidth that pass in the default value, but those look like bugs.  It seems like you would always want to have the includeMaxWidth arg.

> And the test cases for that are: 
> css2.1/20110323/replaced-intrinsic-ratio-001.htm
> css2.1/20110323/replaced-intrinsic-003.htm
> css2.1/20110323/block-replaced-height-007.htm
> css2.1/20110323/float-replaced-height-007.htm
> css2.1/20110323/inline-block-replaced-height-007.htm
> css2.1/20110323/inline-replaced-height-007.htm

Do these tests start failing if you pass includeMaxWidth through?  grepping those files for max-width returns no results.

[KyungTae Kim]

Created attachment 180256 [details]
Patch

[KyungTae Kim]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #10)
> > > (In reply to comment #9)
> > > > Is separated control really not needed? Some callers use includeMaxWidth differently now.
> > > 
> > > I'm not sure if they need to be separate or if we just have poor test coverage.  Do you know which tests cover these cases?
> > As I searched, the code originated from https://bugs.webkit.org/show_bug.cgi?id=15849
> 
> I don't see any calls to computeReplacedLogicalWidthRespectingMinMaxWidth with 'false' passed in for includeMaxWidth.
Actually the call computeReplacedLogicalWidthRespectingMinMaxWidth with 'false' is introduced by bug 88437 ,
And I guess he got the 'false' from computeIntrinsicLogicalWidth(containingBlock, false) that introduced by bug 15849
Sorry for insufficient information.

> There are plenty of calls in computeReplacedLogicalWidth to computeReplacedLogicalWidthRespectingMinMaxWidth that pass in the default value, but those look like bugs.  It seems like you would always want to have the includeMaxWidth arg.
OK. I just afraid of regression by my patch.

Anyway, I uploaded new patch, I didn't use enum because I think it can passed by bool,
if enum is better, I'll modify.

[Tony Chang]

Comment on attachment 180256 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180256&action=review

Please use an enum. See http://www.webkit.org/coding/coding-style.html#names-enum-to-bool

> Source/WebCore/rendering/RenderReplaced.cpp:452
>      // FIXME: We shouldn't be calling a logical width computing function in preferred
>      // logical widths computation as the layout information is probably invalid.

I think this fixes this FIXME. You can remove this comment.

> Source/WebCore/rendering/RenderReplaced.h:35
> +    virtual LayoutUnit computeReplacedLogicalWidth(bool isComputePreferred = false) const;

OVERRIDE

> Source/WebCore/rendering/RenderVideo.h:73
> +    virtual LayoutUnit computeReplacedLogicalWidth(bool isComputePreferred = false) const;

OVERRIDE

[KyungTae Kim]

Created attachment 180424 [details]
Patch

[Tony Chang]

Comment on attachment 180424 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=180424&action=review

> Source/WebCore/rendering/RenderBox.cpp:2418
> +LayoutUnit RenderBox::computeReplacedLogicalWidth(ComputeType computeType) const

computeType is very vague.  Please use something like shouldComputePreferred for the variable name.

> Source/WebCore/rendering/RenderBox.h:43
> +enum ComputeType { ComputeActual, ComputePreferred  };

I would probably name it something like: enum ComputePreferred { ShouldComputeActual, ShouldComputePreferred };

> Source/WebCore/rendering/RenderReplaced.h:35
> +    virtual LayoutUnit computeReplacedLogicalWidth(ComputeType  = ComputePreferred) const;

OVERRIDE

[Tony Chang]

Comment on attachment 180424 [details]
Patch

Sorry, I mean, please make the changes and you can use "webkit-patch land-safely" to upload the final version without need for another review.

[KyungTae Kim]

Created attachment 180438 [details]
Patch

[KyungTae Kim]

Comment on attachment 180438 [details]
Patch

Modify the enum type to enum ShouldComputePreferred { ComputeActual, ComputePreferred };

[WebKit Review Bot]

Comment on attachment 180438 [details]
Patch

Clearing flags on attachment: 180438

Committed r138332: <http://trac.webkit.org/changeset/138332>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.