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>
Created attachment 179435 [details] Patch
Created attachment 179439 [details] Patch 2 better summary
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
(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*)
Created attachment 179480 [details] Patch 2
ping reviewers
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.
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.
(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?
(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.
(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.
Created attachment 183129 [details] Patch 4
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
Created attachment 183142 [details] Patch 5 a test fix
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.
Created attachment 183382 [details] Patch 6 Handle box-sizing change too
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.
Comment on attachment 183382 [details] Patch 6 Thanks for fixing this!
Comment on attachment 183382 [details] Patch 6 Thank you for the advices.
Comment on attachment 183382 [details] Patch 6 Clearing flags on attachment: 183382 Committed r140290: <http://trac.webkit.org/changeset/140290>
All reviewed patches have been landed. Closing bug.
Re-opened since this is blocked by bug 107412
Created attachment 183708 [details] Patch for landing Fix assertion failures.
Comment on attachment 183708 [details] Patch for landing Clearing flags on attachment: 183708 Committed r140296: <http://trac.webkit.org/changeset/140296>
This change breaks the web: https://bugs.webkit.org/show_bug.cgi?id=107824
I think this also caused bug 107474. I'm going to roll it out.
Re-opened since this is blocked by bug 107857
Another downstream regression that is caused with this change: http://code.google.com/p/chromium/issues/detail?id=171917
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.
Created attachment 184595 [details] Patch
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)?
(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?
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.
Created attachment 184778 [details] Patch for landing
Comment on attachment 184778 [details] Patch for landing Clearing flags on attachment: 184778 Committed r140854: <http://trac.webkit.org/changeset/140854>