Bug 104997

Summary:

Re-layout child blocks when border/padding of the box-sizing:border-box parent is updated

Product:

WebKit

Reporter:

Kent Tamura <tkent>

Component:

Layout and Rendering

Assignee:

Kent Tamura <tkent>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

buildbot, dglazkov, eric, jchaffraix, mifenton, ojan.autocc, ojan, pfeldman, rniwa, simon.fraser, tony, webkit.review.bot

Priority:

Version:

528+ (Nightly build)

Hardware:

All

OS:

All

Bug Depends on:

107412, 107857

Bug Blocks:

Attachments:

Description	Flags
Patch	none
Patch 2	none
Patch 2	none
Patch 4	none
Patch 5	none
Patch 6	none
Patch for landing	none
regression test case	none
Patch	none
Patch for landing	none

Kent Tamura

Reported 2012-12-14 00:07:52 PST

http://code.google.com/p/chromium/issues/detail?id=140001 Repro: <body> <input id=text1 style="width:100px; text-align:right;" value="Hello world"> <script> window.onload = function() { setTimeout(function() { document.getElementById('text1').style.paddingLeft = '40px'; }, 0); }; </script> </body>

Attachments
Patch (3.83 KB, patch) 2012-12-14 00:17 PST, Kent Tamura	no flags	Details Formatted Diff Diff
Patch 2 (3.85 KB, patch) 2012-12-14 00:26 PST, Kent Tamura	no flags	Details Formatted Diff Diff
Patch 2 (4.77 KB, patch) 2012-12-14 07:46 PST, Kent Tamura	no flags	Details Formatted Diff Diff
Patch 4 (6.44 KB, patch) 2013-01-16 23:35 PST, Kent Tamura	no flags	Details Formatted Diff Diff
Patch 5 (6.42 KB, patch) 2013-01-17 01:22 PST, Kent Tamura	no flags	Details Formatted Diff Diff
Patch 6 (7.01 KB, patch) 2013-01-17 23:08 PST, Kent Tamura	no flags	Details Formatted Diff Diff
Patch for landing (6.97 KB, patch) 2013-01-20 21:47 PST, Kent Tamura	no flags	Details Formatted Diff Diff
regression test case (424 bytes, text/html) 2013-01-24 15:12 PST, Tony Chang	no flags	Details
Patch (9.75 KB, patch) 2013-01-24 15:32 PST, Tony Chang	no flags	Details Formatted Diff Diff
Patch for landing (9.79 KB, patch) 2013-01-25 11:19 PST, Tony Chang	no flags	Details Formatted Diff Diff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Kent Tamura

Comment 1 2012-12-14 00:17:19 PST

Created attachment 179435 [details] Patch

Kent Tamura

Comment 2 2012-12-14 00:26:52 PST

Created attachment 179439 [details] Patch 2 better summary

Build Bot

Comment 3 2012-12-14 03:50:10 PST

Comment on attachment 179439 [details] Patch 2 Attachment 179439 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15310673 New failing tests: fast/forms/drag-into-textarea.html editing/pasteboard/drag-drop-input-textarea.html editing/pasteboard/drop-text-events.html editing/pasteboard/drop-inputtext-acquires-style.html editing/pasteboard/drag-drop-url-text.html

Kent Tamura

Comment 4 2012-12-14 06:32:51 PST

(In reply to comment #3) > (From update of attachment 179439 [details]) > Attachment 179439 [details] did not pass mac-ews (mac): > Output: http://queues.webkit.org/results/15310673 > > New failing tests: > fast/forms/drag-into-textarea.html > editing/pasteboard/drag-drop-input-textarea.html > editing/pasteboard/drop-text-events.html > editing/pasteboard/drop-inputtext-acquires-style.html > editing/pasteboard/drag-drop-url-text.html It looks a real regression. They crash. ASSERTION FAILED: Uncaught exception - Cannot lock focus on image <NSImage 0x7ff98ca06a00 Size={0, 0} Reps=( )>, because it is size zero. 0 /Users/kent/Webkit/Source/WebCore/platform/mac/BlockExceptions.mm(36) : void ReportBlockedObjCException(NSException *) 1 0x10f16c7ad ReportBlockedObjCException(NSException*) 2 0x10f6a91df WebCore::EventHandler::mouseDragged(NSEvent*)

Kent Tamura

Comment 5 2012-12-14 07:46:16 PST

Created attachment 179480 [details] Patch 2

Kent Tamura

Comment 6 2013-01-07 17:49:14 PST

ping reviewers

Tony Chang

Comment 7 2013-01-08 11:07:00 PST

Comment on attachment 179480 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=179480&action=review I did some digging and found out that the bug is more general. It happens to all box-sizing: border-box cases. Here's an example: http://jsfiddle.net/TCFCf/ I think the fix is simple. In RenderBlock::updateLogicalWidthAndColumnWidth, we compare the logicalWidth before and after. I think we should be comparing the logicalContentWidth instead. Hmm, we'll have to test to see if that causes other unintended side effects. > LayoutTests/fast/forms/text/text-padding-dynamic-change.html:11 > +if (window.testRunner) > + testRunner.waitUntilDone(); > +window.onload = function() { > + setTimeout(function() { You shouldn't need to use waitUntilDone or setTimeout. In the <script>, force a layout using document.body.offsetHeight, then change the padding. You don't even need to wait for onload. You could also make this a check-layout.js test (it has slightly nicer output with PASS/FAIL), but a ref-test is also OK.

Kent Tamura

Comment 8 2013-01-08 21:01:15 PST

Thank you for the review. (In reply to comment #7) > I did some digging and found out that the bug is more general. It happens to all box-sizing: border-box cases. Here's an example: http://jsfiddle.net/TCFCf/ > > I think the fix is simple. In RenderBlock::updateLogicalWidthAndColumnWidth, we compare the logicalWidth before and after. I think we should be comparing the logicalContentWidth instead. Hmm, we'll have to test to see if that causes other unintended side effects. Hmm, the replacement in updateLogicalWidthAndColumnWidth didn't fix http://jsfiddle.net/TCFCf/. We need more investigation.

Kent Tamura

Comment 9 2013-01-08 22:48:52 PST

(In reply to comment #8) > > I think the fix is simple. In RenderBlock::updateLogicalWidthAndColumnWidth, we compare the logicalWidth before and after. I think we should be comparing the logicalContentWidth instead. Hmm, we'll have to test to see if that causes other unintended side effects. > > Hmm, the replacement in updateLogicalWidthAndColumnWidth didn't fix http://jsfiddle.net/TCFCf/. We need more investigation. LayoutUnit contentWidth() const { return clientWidth() - paddingLeft() - paddingRight(); } LayoutUnit contentHeight() const { return clientHeight() - paddingTop() - paddingBottom(); } LayoutUnit contentLogicalWidth() const { return style()->isHorizontalWritingMode() ? contentWidth() : contentHeight(); } In updateLogicalWidthAndColumnWidth, paddingLeft() already has a new value, and contentLogicalWidth() before and after updateLogicalWidth() are same. I have no idea to detect padding changes in RenderBlock::layoutBlock(). Should we set a flag in styleDidChange() if box-sizing==border-box and paddings are changed?

Tony Chang

Comment 10 2013-01-09 09:59:33 PST

(In reply to comment #9) > In updateLogicalWidthAndColumnWidth, paddingLeft() already has a new value, and contentLogicalWidth() before and after updateLogicalWidth() are same. Ah right, sorry, that doesn't work. > I have no idea to detect padding changes in RenderBlock::layoutBlock(). Should we set a flag in styleDidChange() if box-sizing==border-box and paddings are changed? Yes, I think that's the right idea. In RenderBlock::styleDidChange(), if either the old style or the new style is border-box and the padding or border changed (specifically, in the inline direction), we want to mark all children as needing layout (setChildNeedsLayout(true, MarkOnlyThis)). Since it's possible to reproduce the bug by setting the border instead of the padding, we want a test case for border as well.

Kent Tamura

Comment 11 2013-01-16 23:23:15 PST

(In reply to comment #10) > Yes, I think that's the right idea. In RenderBlock::styleDidChange(), if either the old style or the new style is border-box and the padding or border changed (specifically, in the inline direction), we want to mark all children as needing layout (setChildNeedsLayout(true, MarkOnlyThis)). setChildNeedsLayout(true, MarkOnlyThis) didn't resolve the bug. It seems we have to apply setChildNeedsLayout to each child. RenderBlock::layoutBlockChildren applies setChildNeedsLayout to each child when relayoutChildren==true.

Kent Tamura

Comment 12 2013-01-16 23:35:57 PST

Created attachment 183129 [details] Patch 4

Build Bot

Comment 13 2013-01-17 01:02:46 PST

Comment on attachment 183129 [details] Patch 4 Attachment 183129 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15906769 New failing tests: fast/forms/text/text-padding-dynamic-change.html

Kent Tamura

Comment 14 2013-01-17 01:22:51 PST

Created attachment 183142 [details] Patch 5 a test fix

Tony Chang

Comment 15 2013-01-17 10:40:22 PST

Comment on attachment 183142 [details] Patch 5 View in context: https://bugs.webkit.org/attachment.cgi?id=183142&action=review > Source/WebCore/rendering/RenderBox.cpp:295 > + if (newStyle->boxSizing() == BORDER_BOX && oldStyle && oldStyle->boxSizing() == BORDER_BOX > + && (newStyle->paddingBox() != oldStyle->paddingBox() || newStyle->border() != oldStyle->border())) { I think it's possible that we need to relayout children when changing boxSizing. Here's an example: http://plexode.com/u/#AVcolor2Udiv2SentERwidth66P%3D*QOdocumS.2Nbox2M%3E2K%3CEJborder2H%0A2G%3Bj9px!solid!blackj8G!N-sizing66%3A!A4style2*%222!%20~http://plexode.com/eval3/#ht=K4MH%23x!%7BH!!!!background-V6cyanG!H%7DHK%2F4MHKU!idPx*!4PR100px8%3AJ-NG*MKU!4PJ619*MThis!N!should!not!overflow!the!cyan!Ved!N.K%2FUM&jt=Obody.offsetLeftGHOgetElemSById(*x*).setAttribute(*4*%2C!!*R50pxG!J-left650986contS-NG*) I think the right condition is: (newStyle->boxSizing() == BORDER_BOX || (oldStyle && oldStyle->boxSizing() == BORDER_BOX)) && (newStyle->paddingBox() != oldStyle->paddingBox() || newStyle->border() != oldStyle->border()) You could make the padding and border change more specific to test only left/right in horizontal writing mode and top/bottom in vertical writing mode, but maybe that's not worth the code complexity.

Kent Tamura

Comment 16 2013-01-17 23:08:14 PST

Created attachment 183382 [details] Patch 6 Handle box-sizing change too

Kent Tamura

Comment 17 2013-01-17 23:09:18 PST

Comment on attachment 183142 [details] Patch 5 View in context: https://bugs.webkit.org/attachment.cgi?id=183142&action=review >> Source/WebCore/rendering/RenderBox.cpp:295 >> + && (newStyle->paddingBox() != oldStyle->paddingBox() || newStyle->border() != oldStyle->border())) { > > I think it's possible that we need to relayout children when changing boxSizing. Here's an example: > http://plexode.com/u/#AVcolor2Udiv2SentERwidth66P%3D*QOdocumS.2Nbox2M%3E2K%3CEJborder2H%0A2G%3Bj9px!solid!blackj8G!N-sizing66%3A!A4style2*%222!%20~http://plexode.com/eval3/#ht=K4MH%23x!%7BH!!!!background-V6cyanG!H%7DHK%2F4MHKU!idPx*!4PR100px8%3AJ-NG*MKU!4PJ619*MThis!N!should!not!overflow!the!cyan!Ved!N.K%2FUM&jt=Obody.offsetLeftGHOgetElemSById(*x*).setAttribute(*4*%2C!!*R50pxG!J-left650986contS-NG*) > > I think the right condition is: (newStyle->boxSizing() == BORDER_BOX || (oldStyle && oldStyle->boxSizing() == BORDER_BOX)) && (newStyle->paddingBox() != oldStyle->paddingBox() || newStyle->border() != oldStyle->border()) > > You could make the padding and border change more specific to test only left/right in horizontal writing mode and top/bottom in vertical writing mode, but maybe that's not worth the code complexity. Indeed. I update the condition.

Tony Chang

Comment 18 2013-01-18 10:38:11 PST

Comment on attachment 183382 [details] Patch 6 Thanks for fixing this!

Kent Tamura

Comment 19 2013-01-20 18:19:09 PST

Comment on attachment 183382 [details] Patch 6 Thank you for the advices.

WebKit Review Bot

Comment 20 2013-01-20 18:41:30 PST

Comment on attachment 183382 [details] Patch 6 Clearing flags on attachment: 183382 Committed r140290: <http://trac.webkit.org/changeset/140290>

WebKit Review Bot

Comment 21 2013-01-20 18:41:35 PST

All reviewed patches have been landed. Closing bug.

WebKit Review Bot

Comment 22 2013-01-20 21:21:12 PST

Re-opened since this is blocked by bug 107412

Kent Tamura

Comment 23 2013-01-20 21:47:31 PST

Created attachment 183708 [details] Patch for landing Fix assertion failures.

WebKit Review Bot

Comment 24 2013-01-20 22:11:51 PST

Comment on attachment 183708 [details] Patch for landing Clearing flags on attachment: 183708 Committed r140296: <http://trac.webkit.org/changeset/140296>

WebKit Review Bot

Comment 25 2013-01-20 22:11:56 PST

All reviewed patches have been landed. Closing bug.

Pavel Feldman

Comment 26 2013-01-24 07:09:11 PST

This change breaks the web: https://bugs.webkit.org/show_bug.cgi?id=107824

Simon Fraser (smfr)

Comment 27 2013-01-24 13:24:43 PST

I think this also caused bug 107474. I'm going to roll it out.

WebKit Review Bot

Comment 28 2013-01-24 13:26:30 PST

Re-opened since this is blocked by bug 107857

Pavel Feldman

Comment 29 2013-01-24 13:35:26 PST

Another downstream regression that is caused with this change: http://code.google.com/p/chromium/issues/detail?id=171917

Tony Chang

Comment 30 2013-01-24 15:12:23 PST

Created attachment 184590 [details] regression test case The problem is that we're setting the children as needing layout when the input itself doesn't need layout. This can happen if only the border color changes, which would cause newStyle->border() != oldStyle->border() to be true. Probably the right fix here is to compare each border width piece separately.

Tony Chang

Comment 31 2013-01-24 15:32:40 PST

Created attachment 184595 [details] Patch

Simon Fraser (smfr)

Comment 32 2013-01-24 16:28:53 PST

Comment on attachment 184595 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184595&action=review > Source/WebCore/rendering/RenderBox.cpp:321 > + if (oldStyle && (newStyle->boxSizing() == BORDER_BOX || oldStyle->boxSizing() == BORDER_BOX) > + && (newStyle->paddingBox() != oldStyle->paddingBox() || borderWidthChanged(oldStyle, newStyle))) { > + for (RenderObject* child = firstChild(); child; child = child->nextSibling()) > + child->setChildNeedsLayout(true, MarkOnlyThis); > + } Won't 'diff' tell you if the diff is a layout diff (border width or something else changed) or a repaint diff (border color changed)?

Tony Chang

Comment 33 2013-01-24 16:31:13 PST

(In reply to comment #32) > (From update of attachment 184595 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=184595&action=review > > > Source/WebCore/rendering/RenderBox.cpp:321 > > + if (oldStyle && (newStyle->boxSizing() == BORDER_BOX || oldStyle->boxSizing() == BORDER_BOX) > > + && (newStyle->paddingBox() != oldStyle->paddingBox() || borderWidthChanged(oldStyle, newStyle))) { > > + for (RenderObject* child = firstChild(); child; child = child->nextSibling()) > > + child->setChildNeedsLayout(true, MarkOnlyThis); > > + } > > Won't 'diff' tell you if the diff is a layout diff (border width or something else changed) or a repaint diff (border color changed)? Yes, we could add a check to make sure that diff is a StyleDifferenceLayout, but that would be redundant with the existing checks. Would you like me to ASSERT this condition?

Kent Tamura

Comment 34 2013-01-24 19:38:47 PST

Comment on attachment 184595 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=184595&action=review I'm sorry for the disaster, and thank you for the fix. >>> Source/WebCore/rendering/RenderBox.cpp:321 >>> + } >> >> Won't 'diff' tell you if the diff is a layout diff (border width or something else changed) or a repaint diff (border color changed)? > > Yes, we could add a check to make sure that diff is a StyleDifferenceLayout, but that would be redundant with the existing checks. Would you like me to ASSERT this condition? I think adding "diff==StyleDifferenceLayout&&" is reasonable because it would be an early exit path for casess of no padding/broder changes.

Tony Chang

Comment 35 2013-01-25 11:19:35 PST

Created attachment 184778 [details] Patch for landing

WebKit Review Bot

Comment 36 2013-01-25 11:53:27 PST

Comment on attachment 184778 [details] Patch for landing Clearing flags on attachment: 184778 Committed r140854: <http://trac.webkit.org/changeset/140854>

WebKit Review Bot

Comment 37 2013-01-25 11:53:33 PST

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.