WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
95892
REGRESSION(
r122501
): replaced elements with percent width are wrongly size when inserted inside an auto-table layout
https://bugs.webkit.org/show_bug.cgi?id=95892
Summary
REGRESSION(r122501): replaced elements with percent width are wrongly size wh...
Julien Chaffraix
Reported
2012-09-05 14:09:52 PDT
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.
Attachments
Reproduction - the iframe's page should fit its content
(589 bytes, text/html)
2012-09-05 14:09 PDT
,
Julien Chaffraix
no flags
Details
Proposed fix: Avoid calling computeReplacedLogicalWidth for percent logical width during preferred logical width computation.
(8.01 KB, patch)
2012-09-11 14:05 PDT
,
Julien Chaffraix
no flags
Details
Formatted Diff
Diff
Updated change. Fix existing vertical-writing mode bug + took Ojan & Elliot's comments into account.
(10.59 KB, patch)
2012-09-12 11:53 PDT
,
Julien Chaffraix
no flags
Details
Formatted Diff
Diff
Patch for landing
(8.62 KB, patch)
2012-09-12 16:51 PDT
,
Julien Chaffraix
no flags
Details
Formatted Diff
Diff
Show Obsolete
(2)
View All
Add attachment
proposed patch, testcase, etc.
Julien Chaffraix
Comment 1
2012-09-07 14:15:46 PDT
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
Comment 2
2012-09-11 14:05:42 PDT
Created
attachment 163431
[details]
Proposed fix: Avoid calling computeReplacedLogicalWidth for percent logical width during preferred logical width computation.
Elliott Sprehn
Comment 3
2012-09-11 14:17:45 PDT
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 4
2012-09-11 15:22:31 PDT
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 5
2012-09-11 16:00:12 PDT
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
Comment 6
2012-09-12 11:53:36 PDT
Created
attachment 163666
[details]
Updated change. Fix existing vertical-writing mode bug + took Ojan & Elliot's comments into account.
Ojan Vafai
Comment 7
2012-09-12 12:07:42 PDT
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
Comment 8
2012-09-12 16:51:08 PDT
> 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
Comment 9
2012-09-12 16:51:53 PDT
Created
attachment 163737
[details]
Patch for landing
WebKit Review Bot
Comment 10
2012-09-12 17:29:45 PDT
Comment on
attachment 163737
[details]
Patch for landing Clearing flags on attachment: 163737 Committed
r128389
: <
http://trac.webkit.org/changeset/128389
>
WebKit Review Bot
Comment 11
2012-09-12 17:29:49 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug