Bug 95892 – REGRESSION(r122501): replaced elements with percent width are wrongly size when inserted inside an auto-table layout

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Julien Chaffraix]

Created attachment 162330 [details]
Reproduction - the iframe's page should fit its content

This is a clear regression from r122501 (tested locally and rolling out the change fixes the issue). In the test case, the iframe's width should be sized after the intrinsic content and not be over-contrained. Switching to an image gives similar results where the sizing is wrongly computed.

The branch added in r122501 is executed during the auto-table layout resolution and confuses our sizing. I haven't yet pinpointed what is wrong.

Taking the bug as I am investigating it.

[Julien Chaffraix]

Here is what I think is wrong: RenderReplaced::computePreferredLogicalWidth calls computeReplacedLogicalWidth(false). computeReplacedLogicalWidth assumes it is called during layout and thus tries to access the containing block's layout information (availableLogicalWidth() for example), which are unset / stale.

r122501 exposed this existing issue but did not cause it.

Still figuring out if there is an easy way to cut this dependency without adding a lot of code duplication.

[Julien Chaffraix]

Created attachment 163431 [details]
Proposed fix: Avoid calling computeReplacedLogicalWidth for percent logical width during preferred logical width computation.

[Elliott Sprehn]

Comment on attachment 163431 [details]
Proposed fix: Avoid calling computeReplacedLogicalWidth for percent logical width during preferred logical width computation.

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

> Source/WebCore/rendering/RenderReplaced.cpp:453
> +    // logical widths computation as the layout information are probably invalid.

is probably

> Source/WebCore/rendering/RenderReplaced.cpp:461
>      LayoutUnit borderAndPadding = borderAndPaddingWidth();

This looks wrong. You're adding the logical width to the physical border and padding. I think this should be borderAndPaddingLogicalWidth()

[Ojan Vafai]

Comment on attachment 163431 [details]
Proposed fix: Avoid calling computeReplacedLogicalWidth for percent logical width during preferred logical width computation.

This new code will do the wrong thing for -webkit-calc I believe. I guess it's not really clear what calc should do here, but elsewhere we send it down the percent codepath.

I can't think of a great way to do this "right" and I can't think of how you would make that FIXME work. Given that, I'd rather this code just special-cased percent and calc to return intrinsicLogicalWidth and computeReplacedLogicalWidth otherwise.

[Julien Chaffraix]

Comment on attachment 163431 [details]
Proposed fix: Avoid calling computeReplacedLogicalWidth for percent logical width during preferred logical width computation.

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

(In reply to comment #4)
> (From update of attachment 163431 [details])
> This new code will do the wrong thing for -webkit-calc I believe. I guess it's not really clear what calc should do here, but elsewhere we send it down the percent codepath.

The code is right for -webkit-calc AFAICT. That's because isPercent() returns true for calc (see Length.h).

> I can't think of a great way to do this "right" and I can't think of how you would make that FIXME work. Given that, I'd rather this code just special-cased percent and calc to return intrinsicLogicalWidth and computeReplacedLogicalWidth otherwise.

Note sure I entirely follow. What you are describing is pretty much what the new code does with the exception of handling fixed and viewport percentage on the spot which are pretty trivial cases (especially since computePreferredLogicalWidths checks max-width).

The only solution to the FIXME I know is to copy computeReplacedLogicalWidth and have the new code not depend on layout information. I haven't found a better answer unfortunately but that doesn't mean the FIXME doesn't hold. This code reuse screams that something is deeply wrong, however for now, it's better to avoid code duplication (at least until we have more evidence that it is harming us).

>> Source/WebCore/rendering/RenderReplaced.cpp:461
>>      LayoutUnit borderAndPadding = borderAndPaddingWidth();
> 
> This looks wrong. You're adding the logical width to the physical border and padding. I think this should be borderAndPaddingLogicalWidth()

True but this is an existing bug. I will see if I can fix it as part of this change.

[Julien Chaffraix]

Created attachment 163666 [details]
Updated change. Fix existing vertical-writing mode bug + took Ojan & Elliot's comments into account.

[Ojan Vafai]

Comment on attachment 163666 [details]
Updated change. Fix existing vertical-writing mode bug + took Ojan & Elliot's comments into account.

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

Please address the test comments before committing. I can show you a check-layout.js example in person.

> LayoutTests/fast/replaced/vertical-writing-mode-max-logical-width-replaced.html:1
> +<!DOCTYPE html>

You should make this a check-layout.js test! You could replace all of your script except the dumpAsText part with data-expected-width=200 data-expected-height=100 attributes on the image.

> LayoutTests/fast/table/bad-replaced-sizing-preferred-logical-widths.html:20
> +        <iframe src="resources/iframe.html" width="100%"></iframe>

Can you just use a data url or srcdoc attribute? Then you don't need to add another file.

> LayoutTests/fast/table/bad-replaced-sizing-preferred-logical-widths.html:43
> +    // 50% is a somewhat arbitrary threshold but it should shield us from the viewport size.

How about instead setting a fixed width on the html or body element and then here you can assert actual pixel sizes? Then this could also just be a check-layout.js test.

[Julien Chaffraix]

> Please address the test comments before committing. I can show you a check-layout.js example in person.

Will upload a patch with all the changes.

>> LayoutTests/fast/table/bad-replaced-sizing-preferred-logical-widths.html:43
>> +    // 50% is a somewhat arbitrary threshold but it should shield us from the viewport size.

> How about instead setting a fixed width on the html or body element and then here you can 
> assert actual pixel sizes? Then this could also just be a check-layout.js test.

I was not very comfortable with this as I thought our results were pretty different from the other browsers. However it turns out OK due to the fixed length on <html> and we are close enough so that the width is expected.

[Julien Chaffraix]

Created attachment 163737 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 163737 [details]
Patch for landing

Clearing flags on attachment: 163737

Committed r128389: <http://trac.webkit.org/changeset/128389>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.