Bug 102784

Summary:

Percentage width replaced element incorrectly rendered when intrinsic size changed

Product:

WebKit

Reporter:

KyungTae Kim <ktf.kim>

Component:

Layout and Rendering

Assignee:

KyungTae Kim <ktf.kim>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

dglazkov, eric, koivisto, ojan.autocc, ojan, tony, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none
Patch	none

KyungTae Kim

Reported 2012-11-20 02:27:50 PST

Percentage width replaced element incorrectly rendered when intrinsic size changed. The issue scenario is like below : 1) <image> with percentage width is included in parent element like <table> or "absolute" positioned. 2) parent element's width is set from the <image>'s width 3) <image>'s src is changed to larger image 4) because the <image>'s width is percentage, it refers the parent element's width 5) <image>'s width is not updated In RenderBox::computeReplacedLogicalWidthUsing, there already 2 FIXME exist, but none of those are same with this case. https://bugs.webkit.org/show_bug.cgi?id=46496 https://bugs.webkit.org/show_bug.cgi?id=91071

Attachments
Patch (4.75 KB, patch) 2012-11-27 00:26 PST, KyungTae Kim	no flags	Details Formatted Diff Diff
Patch (5.64 KB, patch) 2012-11-27 20:50 PST, KyungTae Kim	no flags	Details Formatted Diff Diff
Patch (6.84 KB, patch) 2012-12-09 17:11 PST, KyungTae Kim	no flags	Details Formatted Diff Diff
Patch (6.01 KB, patch) 2012-12-12 15:56 PST, KyungTae Kim	no flags	Details Formatted Diff Diff
Patch (8.74 KB, patch) 2012-12-13 16:48 PST, KyungTae Kim	no flags	Details Formatted Diff Diff
Patch (7.98 KB, patch) 2012-12-14 01:25 PST, KyungTae Kim	no flags	Details Formatted Diff Diff
Patch (6.17 KB, patch) 2012-12-15 22:24 PST, KyungTae Kim	no flags	Details Formatted Diff Diff
Patch (5.99 KB, patch) 2012-12-17 15:46 PST, KyungTae Kim	no flags	Details Formatted Diff Diff
Show Obsolete (7) View All Add attachment proposed patch, testcase, etc.

KyungTae Kim

Comment 1 2012-11-27 00:26:00 PST

Created attachment 176190 [details] Patch

KyungTae Kim

Comment 2 2012-11-27 00:39:30 PST

In existing code, in the below callstack's case, the 'logical width' is not properly updated: RenderImage::imageDimensionsChanged RenderBox::computeLogicalWidthInRegion RenderReplaced::computeReplacedLogicalWidth RenderReplaced::computeAspectRatioInformationForRenderBox RenderBox::computeReplacedLogicalWidth RenderBox::computeReplacedLogicalWidthRespectingMinMaxWidth RenderBox::computeReplacedLogicalWidthUsing (In case 'Percent', the updated width is not properly updated) It's the case that the width of the container of the replaced element was calculated from the width of the replaced element, but the replaced element try to refer it after because the width type is 'percent'. I'm trying to find a method to handle that, but if there's no optimal solution, I think we need to re-layout whenever the intrinsic size is updated.

WebKit Review Bot

Comment 3 2012-11-27 01:48:55 PST

Comment on attachment 176190 [details] Patch Attachment 176190 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14989942 New failing tests: fast/canvas/canvas-as-image-incremental-repaint.html platform/chromium/virtual/gpu/fast/canvas/canvas-as-image-incremental-repaint.html

Tony Chang

Comment 4 2012-11-27 11:37:59 PST

Comment on attachment 176190 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176190&action=review We don't want to throw away this optimization. Instead of always forcing a layout, can't we check to see if there's a percent logical width, min-width, or max-width and only layout when needed? Also, this seems like it's only a problem in shrink-to-fit objects (sizesLogicalWidthToFitContent returns true). We could maybe detect that as well. > LayoutTests/ChangeLog:11 > + * fast/css/percent-width-img-src-change-expected.html: Added. > + * fast/css/percent-width-img-src-change.html: Added. I would make this a dumpAsText/js-test-pre.js test that checks the size of the image and makes sure it is 64. The color is confusing since red boxes normally mean fail. > LayoutTests/fast/css/percent-width-img-src-change.html:1 > +<html> Missing <!DOCTYPE html> > LayoutTests/fast/css/percent-width-img-src-change.html:6 > + htmlImageElements = document.getElementsByClassName("imgForChange"); > + for (i = 0; i < htmlImageElements.length; i++) Nit: 'var' for local variables, ++i instead of i++.

KyungTae Kim

Comment 5 2012-11-27 20:50:21 PST

Created attachment 176390 [details] Patch

Tony Chang

Comment 6 2012-11-28 10:05:35 PST

Comment on attachment 176390 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176390&action=review Upon further discussion and inspection, I think the bug is in RenderBox::computeReplacedLogicalWidthUsing. Specifically, the if (cw > 0 ||) check seems fishy. I'm not entirely sure what the check is trying to do, but it seems like an accident that the first time cw is 0 (because we haven't done a layout yet) and the second time we use the old cw value. I think we want the code to fall through to intrinsicLogicalWidth() on relayout. > LayoutTests/fast/css/percent-width-img-src-change-expected.html:7 > + > + This should be a dumpAsText test. You can use check-layout.js or js-test-pre.js. The benefits are that the test output will be clearer (it will say PASS or FAIL with a useful message) and it will run faster.

KyungTae Kim

Comment 7 2012-11-28 18:18:26 PST

(In reply to comment #6) > Upon further discussion and inspection, I think the bug is in RenderBox::computeReplacedLogicalWidthUsing. Specifically, the if (cw > 0 ||) check seems fishy. I'm not entirely sure what the check is trying to do, but it seems like an accident that the first time cw is 0 (because we haven't done a layout yet) and the second time we use the old cw value. I think we want the code to fall through to intrinsicLogicalWidth() on relayout. If make it to fall through to intrinsicLogicalWidth(), the percentage width calculation could be wrong in other cases. Anyway, as you said, the problem is that the cw(container's width) is not updated value. Normally, the cw is updated before the Image's computeReplacedLogicalWidthUsing called during layouting as the below callstack. [c] RenderBlock::layout [c] RenderBlock::layoutBlock [c] RenderBlock::updateLogicalWidthAndColumnWidth [c] RenderBox::updateLogicalWidth [c] RenderBox::setLogicalWidth [c] RenderBox::setWidth => Update container's width [c] RenderBlock::layoutInlineChildren [i] RenderObject::layoutIfNeeded [i] RenderImage::layout [i] ... [i] RenderBox::computeReplacedLogicalWidthUsing ([c]: for Container object that contains the image, [i]: for the Image object) We can update the cw in computeReplacedLogicalWidthUsing, but it is duplicated operation in most cases. So, I think relayouting on imageDimensionsChanged seems better solution because it's more specific case.

KyungTae Kim

Comment 8 2012-12-09 17:11:32 PST

Created attachment 178457 [details] Patch

Build Bot

Comment 9 2012-12-09 18:38:45 PST

Comment on attachment 178457 [details] Patch Attachment 178457 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15222506 New failing tests: fast/css/percent-width-img-src-change.html

WebKit Review Bot

Comment 10 2012-12-09 18:56:11 PST

Comment on attachment 178457 [details] Patch Attachment 178457 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15239221 New failing tests: fast/css/percent-width-img-src-change.html

Tony Chang

Comment 11 2012-12-10 10:23:13 PST

Comment on attachment 178457 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178457&action=review (In reply to comment #7) > (In reply to comment #6) > > Upon further discussion and inspection, I think the bug is in RenderBox::computeReplacedLogicalWidthUsing. Specifically, the if (cw > 0 ||) check seems fishy. I'm not entirely sure what the check is trying to do, but it seems like an accident that the first time cw is 0 (because we haven't done a layout yet) and the second time we use the old cw value. I think we want the code to fall through to intrinsicLogicalWidth() on relayout. > > If make it to fall through to intrinsicLogicalWidth(), the percentage width calculation could be wrong in other cases. Can you describe the other cases where the percentage width calculations would be wrong? > LayoutTests/fast/css/percent-width-img-src-change-expected.txt:3 > +layer at (0,0) size 800x600 > + RenderView at (0,0) size 800x600 > +layer at (0,0) size 800x234 This should not be a pixel test. It should be a dumpAsText test.

KyungTae Kim

Comment 12 2012-12-11 05:12:54 PST

(In reply to comment #11) > (From update of attachment 178457 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=178457&action=review > > (In reply to comment #7) > > (In reply to comment #6) > > > Upon further discussion and inspection, I think the bug is in RenderBox::computeReplacedLogicalWidthUsing. Specifically, the if (cw > 0 ||) check seems fishy. I'm not entirely sure what the check is trying to do, but it seems like an accident that the first time cw is 0 (because we haven't done a layout yet) and the second time we use the old cw value. I think we want the code to fall through to intrinsicLogicalWidth() on relayout. > > > > If make it to fall through to intrinsicLogicalWidth(), the percentage width calculation could be wrong in other cases. > > Can you describe the other cases where the percentage width calculations would be wrong? As you said, the cw is 0 before layout, but as I tested, during layout, the cw is updated before calculating the image's width, and the image refer the updated cw as I mentioned in (Comment #7). If make it to fall through to intrinsicLogicalWidth(), the case (Container width) < (intrinsicLogicalWidth) could be wrong. One of that cases is LayoutTests/fast/css/percent-width-img-inside-zero-percent-and-fixed-container.html. I guess the "if (cw > 0 ||)" became fishy because of this case. > > LayoutTests/fast/css/percent-width-img-src-change-expected.txt:3 > > +layer at (0,0) size 800x600 > > + RenderView at (0,0) size 800x600 > > +layer at (0,0) size 800x234 > > This should not be a pixel test. It should be a dumpAsText test. I tried the dumpAsText test, but that didn't work properly because the image's 'width' was not immediately updated after the 'src'. I think I can use a timer to wait the image loading before check, but I think it seems tricky. Could you please guide me if you know a better solution for that?

Tony Chang

Comment 13 2012-12-11 12:25:55 PST

Comment on attachment 178457 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178457&action=review > Source/WebCore/rendering/RenderImage.cpp:232 > + if (imageSizeChanged || width() != newWidth || height() != newHeight || style()->logicalWidth().type() == Percent || style()->logicalMaxWidth().type() == Percent) { Ok, after some discussion with Ojan and spending some time in an debugger, I think what we want is: if (imageSizeChanged || width() != newWidth || height() != newHeight || ((style()->logicalWidth().type() == Percent && containingBlock()->sizesLogicalWidthToFitContent(MainOrPreferredSize)) || (style()->logicalMaxWidth().type() == Percent && containingBlock()->sizesLogicalWidthToFitContent(MaxSize))) { I would move the new conditions into a helper function. Maybe call it: containingBlockNeedsToRecomputePreferredSize()? >>> LayoutTests/fast/css/percent-width-img-src-change-expected.txt:3 >>> +layer at (0,0) size 800x234 >> >> This should not be a pixel test. It should be a dumpAsText test. > > I tried the dumpAsText test, but that didn't work properly because the image's 'width' was not immediately updated after the 'src'. > I think I can use a timer to wait the image loading before check, but I think it seems tricky. > Could you please guide me if you know a better solution for that? See LayoutTests/fast/replaced/image-resize-width.html for an almost identical test. Change it to use JS to verify the sizes rather than using a pixel test.

Ojan Vafai

Comment 14 2012-12-11 12:29:01 PST

Comment on attachment 178457 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=178457&action=review >> Source/WebCore/rendering/RenderImage.cpp:232 >> + if (imageSizeChanged || width() != newWidth || height() != newHeight || style()->logicalWidth().type() == Percent || style()->logicalMaxWidth().type() == Percent) { > > Ok, after some discussion with Ojan and spending some time in an debugger, I think what we want is: > if (imageSizeChanged || width() != newWidth || height() != newHeight || ((style()->logicalWidth().type() == Percent && containingBlock()->sizesLogicalWidthToFitContent(MainOrPreferredSize)) || (style()->logicalMaxWidth().type() == Percent && containingBlock()->sizesLogicalWidthToFitContent(MaxSize))) { > > I would move the new conditions into a helper function. Maybe call it: > containingBlockNeedsToRecomputePreferredSize()? Nit: also need to check min-width in the case where the image gets smaller.

KyungTae Kim

Comment 15 2012-12-12 15:56:42 PST

Created attachment 179140 [details] Patch

Tony Chang

Comment 16 2012-12-12 16:09:58 PST

Comment on attachment 179140 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179140&action=review Close! Just some minor style nits. > Source/WebCore/rendering/RenderImage.cpp:232 > + // If image's logical width type is percent and the containing block fits to contents, > + // relayout is needed because the 'newWidth' can be wrong because it was calculated from the 'old containing block width' I would remove this command and put this text in the ChangeLog instead. > LayoutTests/fast/css/percent-width-img-src-change.html:11 > + if (image.src.substr(image.src.length - 12, 12) == "greenbox.png") The hardcoded 12 is unfortunate. Maybe use a regular expression like image.src.match("greenbox[.]png$"). > LayoutTests/fast/css/percent-width-img-src-change.html:16 > + if (image.width != 100) > + ++failures; It would be more helpful to output which test case failed. The number of failures isn't that useful. > LayoutTests/fast/css/percent-width-img-src-change.html:52 > +<div style="position:absolute;top:340px;left:11px"> > +<img class="imgForChange" style="max-width:100%;" src="resources/greenbox.png" onload="imageLoaded(this)"> Please add a test for min-width. I think if the second image is smaller than the first image and there's a percent min-width, we won't resize properly.

Tony Chang

Comment 17 2012-12-12 16:10:27 PST

Comment on attachment 179140 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179140&action=review >> Source/WebCore/rendering/RenderImage.cpp:232 >> + // relayout is needed because the 'newWidth' can be wrong because it was calculated from the 'old containing block width' > > I would remove this command and put this text in the ChangeLog instead. s/command/comment/

KyungTae Kim

Comment 18 2012-12-13 16:48:39 PST

Created attachment 179376 [details] Patch

Tony Chang

Comment 19 2012-12-13 16:50:45 PST

Comment on attachment 179376 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179376&action=review > LayoutTests/fast/css/percent-width-img-src-change.html:32 > + Nit: You could uncomment this text now that it's a dumpAsText test.

WebKit Review Bot

Comment 20 2012-12-13 18:06:41 PST

Comment on attachment 179376 [details] Patch Attachment 179376 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15314504 New failing tests: fast/css/percent-min-width-img-src-change.html fast/css/percent-width-img-src-change.html

KyungTae Kim

Comment 21 2012-12-13 18:10:09 PST

Comment on attachment 179376 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179376&action=review > Source/WebCore/rendering/RenderImage.cpp:234 > + || (style()->logicalMinWidth().type() == Percent && containingBlock()->sizesLogicalWidthToFitContent(MinSize)); Currently, containingBlock()->sizesLogicalWidthToFitContent does not work as we expect. This code fail the tests and the below code pass the tests. bool containingBlockNeedsToRecomputePreferredSize = style()->logicalWidth().type() == Percent || style()->logicalMaxWidth().type() == Percent || style()->logicalMinWidth().type() == Percent; I think I need to find another condition that can be used instead of containingBlock()->sizesLogicalWidthToFitContent.

KyungTae Kim

Comment 22 2012-12-14 01:25:23 PST

Created attachment 179446 [details] Patch

KyungTae Kim

Comment 23 2012-12-14 01:42:22 PST

(In reply to comment #21) > (From update of attachment 179376 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=179376&action=review > > > Source/WebCore/rendering/RenderImage.cpp:234 > > + || (style()->logicalMinWidth().type() == Percent && containingBlock()->sizesLogicalWidthToFitContent(MinSize)); > > Currently, containingBlock()->sizesLogicalWidthToFitContent does not work as we expect. > This code fail the tests and the below code pass the tests. > bool containingBlockNeedsToRecomputePreferredSize = style()->logicalWidth().type() == Percent || style()->logicalMaxWidth().type() == Percent || style()->logicalMinWidth().type() == Percent; > > I think I need to find another condition that can be used instead of containingBlock()->sizesLogicalWidthToFitContent. For this, I checked whether containingBlock's logicalWidth is Auto or FitContent. One another weird thing is, in min-width case, even though let it re-layout, the containing block's width is not updated (It seems another bug). It was updated normally on another local source based on r135083. As I checked, in latest version, m_maxPreferredLogicalWidth used in computePositionedLogicalWidthUsing was not updated properly (I'll check further).

WebKit Review Bot

Comment 24 2012-12-14 03:04:07 PST

Comment on attachment 179446 [details] Patch Attachment 179446 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15317621 New failing tests: fast/css/percent-min-width-img-src-change.html

Build Bot

Comment 25 2012-12-14 07:50:52 PST

Comment on attachment 179446 [details] Patch Attachment 179446 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15314678 New failing tests: fast/css/percent-min-width-img-src-change.html

Tony Chang

Comment 26 2012-12-14 12:58:29 PST

(In reply to comment #21) > (From update of attachment 179376 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=179376&action=review > > > Source/WebCore/rendering/RenderImage.cpp:234 > > + || (style()->logicalMinWidth().type() == Percent && containingBlock()->sizesLogicalWidthToFitContent(MinSize)); > > Currently, containingBlock()->sizesLogicalWidthToFitContent does not work as we expect. > This code fail the tests and the below code pass the tests. Hmm, you're right. It looks like position:absolute and table cells don't return true for that function even though they fit content (in RenderBox::computePositionedLogicalWidthUsing, the shrink-to-fit logic is applied). I would probably do something like: // FIXME: We only need to recompute the containing block's preferred size if the containing block's size depends on the image's size (i.e., the container uses shrink-to-fit sizing). There's no way easy way to detect that shrink-to-fit is needed, always force a layout. bool containingBlockNeedsToRecomputePreferredSize = style()->logicalWidth().type() == Percent || style()->logicalMaxWidth().type() == Percent || style()->logicalMinWidth().type() == Percent; Sorry for going in circles on this.

KyungTae Kim

Comment 27 2012-12-15 22:24:38 PST

Created attachment 179640 [details] Patch

KyungTae Kim

Comment 28 2012-12-15 22:51:06 PST

(In reply to comment #26) > (In reply to comment #21) > > (From update of attachment 179376 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=179376&action=review > > > > > Source/WebCore/rendering/RenderImage.cpp:234 > > > + || (style()->logicalMinWidth().type() == Percent && containingBlock()->sizesLogicalWidthToFitContent(MinSize)); > > > > Currently, containingBlock()->sizesLogicalWidthToFitContent does not work as we expect. > > This code fail the tests and the below code pass the tests. > > Hmm, you're right. It looks like position:absolute and table cells don't return true for that function even though they fit content (in RenderBox::computePositionedLogicalWidthUsing, the shrink-to-fit logic is applied). I would probably do something like: > > // FIXME: We only need to recompute the containing block's preferred size if the containing block's size depends on the image's size (i.e., the container uses shrink-to-fit sizing). There's no way easy way to detect that shrink-to-fit is needed, always force a layout. > bool containingBlockNeedsToRecomputePreferredSize = > style()->logicalWidth().type() == Percent > || style()->logicalMaxWidth().type() == Percent > || style()->logicalMinWidth().type() == Percent; > > Sorry for going in circles on this. I think (In reply to comment #23) > (In reply to comment #21) > > (From update of attachment 179376 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=179376&action=review > > > > > Source/WebCore/rendering/RenderImage.cpp:234 > > > + || (style()->logicalMinWidth().type() == Percent && containingBlock()->sizesLogicalWidthToFitContent(MinSize)); > > > > Currently, containingBlock()->sizesLogicalWidthToFitContent does not work as we expect. > > This code fail the tests and the below code pass the tests. > > bool containingBlockNeedsToRecomputePreferredSize = style()->logicalWidth().type() == Percent || style()->logicalMaxWidth().type() == Percent || style()->logicalMinWidth().type() == Percent; > > > > I think I need to find another condition that can be used instead of containingBlock()->sizesLogicalWidthToFitContent. > > For this, I checked whether containingBlock's logicalWidth is Auto or FitContent. > > One another weird thing is, in min-width case, even though let it re-layout, the containing block's width is not updated (It seems another bug). > It was updated normally on another local source based on r135083. > As I checked, in latest version, m_maxPreferredLogicalWidth used in computePositionedLogicalWidthUsing was not updated properly (I'll check further). I removed the LayoutTest for min-width because in min-width case, even though let it re-layout, the containing block's width is not updated now. I'll try to fix it on another new bug.

Tony Chang

Comment 29 2012-12-17 09:14:53 PST

Comment on attachment 179640 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=179640&action=review > Source/WebCore/rendering/RenderImage.cpp:235 > + (containingBlock()->style()->logicalWidth().type() == Auto || containingBlock()->style()->logicalWidth().type() == FitContent) I don't think this part of the condition is correct. I would remove it. For example, try putting the image inside 2 nested floats.

KyungTae Kim

Comment 30 2012-12-17 15:46:10 PST

Created attachment 179817 [details] Patch

Tony Chang

Comment 31 2012-12-17 16:38:55 PST

Comment on attachment 179817 [details] Patch Thanks! Please file a new bug for the min-width case.

WebKit Review Bot

Comment 32 2012-12-17 17:00:31 PST

Comment on attachment 179817 [details] Patch Clearing flags on attachment: 179817 Committed r137960: <http://trac.webkit.org/changeset/137960>

WebKit Review Bot

Comment 33 2012-12-17 17:00:37 PST

All reviewed patches have been landed. Closing bug.

KyungTae Kim

Comment 34 2012-12-18 01:42:02 PST

(In reply to comment #31) > (From update of attachment 179817 [details]) > Thanks! Please file a new bug for the min-width case. I made bug 105264 for the min-width case.

Note You need to log in before you can comment on or make changes to this bug.