Summary: | REGRESSION(r122501): replaced elements with percent width are wrongly size when inserted inside an auto-table layout | ||
---|---|---|---|
Product: | WebKit | Reporter: | Julien Chaffraix <jchaffraix> |
Component: | Layout and Rendering | Assignee: | Julien Chaffraix <jchaffraix> |
Status: | RESOLVED FIXED | ||
Severity: | Normal | CC: | aestes, eric, esprehn, hyatt, ojan, pravind.2k4, robert, tony, webkit.review.bot |
Priority: | P1 | ||
Version: | 528+ (Nightly build) | ||
Hardware: | All | ||
OS: | All | ||
Attachments: |
Description
Julien Chaffraix
2012-09-05 14:09:52 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. Created attachment 163431 [details]
Proposed fix: Avoid calling computeReplacedLogicalWidth for percent logical width during preferred logical width computation.
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() 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.
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. Created attachment 163666 [details]
Updated change. Fix existing vertical-writing mode bug + took Ojan & Elliot's comments into account.
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. > 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. Created attachment 163737 [details]
Patch for landing
Comment on attachment 163737 [details] Patch for landing Clearing flags on attachment: 163737 Committed r128389: <http://trac.webkit.org/changeset/128389> All reviewed patches have been landed. Closing bug. |