Created attachment 75257 [details] redrawing issue testcase See attached testcase. Click the "link" (2nd span) and notice how the new cloned span doesn't appear until you do something to force a redraw (e.g. resizing window or zooming in/out). Reproduces in Chromium with webkit r72952 and Safari 5 on both Windows and Mac. (original reported on jquery bug tracker - http://bugs.jquery.com/ticket/7611)
Created attachment 136418 [details] patch for redrawing issue with inserting inline element between existing inline elements
Comment on attachment 136418 [details] patch for redrawing issue with inserting inline element between existing inline elements This patch is causing many tests to fail: Regressions: Unexpected image mismatch : (23) fast/forms/listbox-clip.html = IMAGE fast/forms/placeholder-position.html = IMAGE fast/multicol/cell-shrinkback.html = IMAGE fast/repaint/bugzilla-50315.html = IMAGE fast/repaint/gradients-em-stops-repaint.html = IMAGE fast/repaint/layout-state-relative.html = IMAGE fast/repaint/line-flow-with-floats-1.html = IMAGE fast/repaint/line-flow-with-floats-10.html = IMAGE fast/repaint/line-flow-with-floats-3.html = IMAGE fast/repaint/line-flow-with-floats-4.html = IMAGE fast/repaint/line-flow-with-floats-5.html = IMAGE fast/repaint/line-flow-with-floats-6.html = IMAGE fast/repaint/line-flow-with-floats-7.html = IMAGE fast/repaint/line-in-scrolled-clipped-block.html = IMAGE fast/repaint/line-overflow.html = IMAGE fast/repaint/lines-with-layout-delta.html = IMAGE fast/repaint/overflow-delete-line.html = IMAGE fast/repaint/overflow-scroll-delete.html = IMAGE fast/repaint/reflection-repaint-test.html = IMAGE fast/repaint/text-append-dirty-lines.html = IMAGE fast/repaint/transform-layout-repaint.html = IMAGE fast/text/international/thai-line-breaks.html = IMAGE platform/chromium-linux/fast/text/international/complex-joining-using-gpos.html = IMAGE
Created attachment 167818 [details] Patch
Created attachment 168548 [details] Patch
You should get a reviewer to comment here, but might there be a way to select the correct linebox for |child| to begin with, rather than going too far and trying to accommodate by dirtying the next linebox? If this is really the best way to fix this problem then it would warrant a comment since it is not at all obvious what that additional clause achieves.
(In reply to comment #5) > You should get a reviewer to comment here, but might there be a way to select the correct linebox for |child| to begin with, rather than going too far and trying to accommodate by dirtying the next linebox? I couldn't see any way of doing this. Even if I checked the RootInlineBox of the inserted child's nextSibling() against the RootInlineBox of the latest previousSibling() that has one and found they were different I still couldn't be sure which line the inserted child is on - really you have to dirty both and let line layout decide. > > If this is really the best way to fix this problem then it would warrant a comment since it is not at all obvious what that additional clause achieves. Yes, fair point.
Created attachment 170433 [details] Patch
Comment on attachment 170433 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=170433&action=review Looks good with a few nits. > Source/WebCore/rendering/RenderLineBoxList.cpp:354 > + // If |child| has been inserted before the first real element in the linebox, but after collapsed leading I'm not sure what exactly "real element" means in this context. Can we do better? > LayoutTests/fast/inline/layout-after-inserting-nested-br-expected.html:4 > + <meta http-equiv="content-type" content="text/html; charset=UTF-8"/> Is this needed? > LayoutTests/fast/inline/layout-after-inserting-nested-br.html:4 > + <meta http-equiv="content-type" content="text/html; charset=UTF-8"/> Ditto. > LayoutTests/fast/inline/layout-after-inserting-nested-br.html:5 > + <title>Redrawing issue when inserting new element</title> I'd make the title the same as the bug instead of truncating. > LayoutTests/fast/inline/layout-after-inserting-nested-br.html:8 > +<body> > +<span id="span">abc<br></span> Please add a description and a bug link to make cross-referencing easier :) > LayoutTests/fast/inline/layout-after-inserting-nested-br.html:30 > + var testEl = document.getElementById("link"); > + var x = testEl.offsetLeft; > + var y = testEl.offsetTop + testEl.offsetHeight / 2; > + if (window.eventSender) { > + eventSender.mouseMoveTo(x,y); > + eventSender.mouseDown(); > + eventSender.mouseUp(); > + } > + setTimeout(testRunner.notifyDone(), 0); > + }, 0); You have some funny indentation going on here... please fix it.
Created attachment 175888 [details] Patch
Comment on attachment 175888 [details] Patch Attachment 175888 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/14989487 New failing tests: WebFrameTest.ChromePageJavascript fast/dom/shadow/shadow-dom-event-dispatching.html WebFrameTest.DispatchMessageEventWithOriginCheck
Created attachment 176034 [details] Patch
Comment on attachment 176034 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176034&action=review > Source/WebCore/rendering/RenderLineBoxList.cpp:354 > + // If |child| has been inserted before the first real element in the linebox, but after collapsed leading Taking this out of the review queue. This comment needs an update as we discussed.
Created attachment 176707 [details] Patch
Comment on attachment 176707 [details] Patch Attachment 176707 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15027930 New failing tests: WebFrameTest.ChromePageJavascript WebFrameTest.DispatchMessageEventWithOriginCheck
Comment on attachment 176707 [details] Patch Attachment 176707 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/15057005 New failing tests: WebFrameTest.ChromePageJavascript WebFrameTest.DispatchMessageEventWithOriginCheck
Created attachment 176818 [details] Patch
(In reply to comment #15) > (From update of attachment 176707 [details]) > Attachment 176707 [details] did not pass chromium-ews (chromium-xvfb): > Output: http://queues.webkit.org/results/15057005 > > New failing tests: > WebFrameTest.ChromePageJavascript > WebFrameTest.DispatchMessageEventWithOriginCheck These failures were genuine and prompted me to revert to expanding the current logic for handling cases where we dirty too far back up the linebox stack.
Comment on attachment 176818 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=176818&action=review > Source/WebCore/rendering/RenderLineBoxList.cpp:393 > - if (adjacentBox && (adjacentBox->lineBreakObj() == child || child->isBR() || (curr && curr->isBR()))) > + // If |child| has been inserted before the first element in the linebox, but after collapsed leading > + // space, the search for |child|'s linebox will go past the leading space to the previous linebox and select that > + // one as |box|. If we hit that situation here, dirty the |box| actually containing the child too. > + bool insertedAfterLeadingSpace = box->lineBreakObj() == child->previousSibling(); > + if (adjacentBox && (adjacentBox->lineBreakObj() == child || child->isBR() || (curr && curr->isBR()) || insertedAfterLeadingSpace)) This makes more sense.
Comment on attachment 176818 [details] Patch Clearing flags on attachment: 176818 Committed r136513: <http://trac.webkit.org/changeset/136513>
All reviewed patches have been landed. Closing bug.