RESOLVED FIXED 50315
Redrawing issue with inserting new inline element between existing inline elements
https://bugs.webkit.org/show_bug.cgi?id=50315
Summary Redrawing issue with inserting new inline element between existing inline ele...
temp01
Reported 2010-11-30 23:17:57 PST
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)
Attachments
redrawing issue testcase (499 bytes, text/html)
2010-11-30 23:17 PST, temp01
no flags
patch for redrawing issue with inserting inline element between existing inline elements (5.08 KB, patch)
2012-04-10 03:23 PDT, bhavaniab
no flags
Patch (5.56 KB, patch)
2012-10-09 12:29 PDT, Robert Hogan
no flags
Patch (5.27 KB, patch)
2012-10-13 01:35 PDT, Robert Hogan
no flags
Patch (5.43 KB, patch)
2012-10-24 11:12 PDT, Robert Hogan
no flags
Patch (5.49 KB, patch)
2012-11-25 12:50 PST, Robert Hogan
no flags
Patch (5.49 KB, patch)
2012-11-26 10:50 PST, Robert Hogan
no flags
Patch (5.47 KB, patch)
2012-11-29 04:58 PST, Robert Hogan
no flags
Patch (5.74 KB, patch)
2012-11-29 14:29 PST, Robert Hogan
no flags
bhavaniab
Comment 1 2012-04-10 03:23:44 PDT
Created attachment 136418 [details] patch for redrawing issue with inserting inline element between existing inline elements
Adam Barth
Comment 2 2012-04-10 15:10:11 PDT
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
Robert Hogan
Comment 3 2012-10-09 12:29:02 PDT
Robert Hogan
Comment 4 2012-10-13 01:35:23 PDT
Ken Buchanan
Comment 5 2012-10-18 13:36:26 PDT
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.
Robert Hogan
Comment 6 2012-10-18 14:11:05 PDT
(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.
Robert Hogan
Comment 7 2012-10-24 11:12:41 PDT
Levi Weintraub
Comment 8 2012-11-05 10:47:57 PST
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.
Robert Hogan
Comment 9 2012-11-25 12:50:50 PST
WebKit Review Bot
Comment 10 2012-11-26 00:34:17 PST
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
Robert Hogan
Comment 11 2012-11-26 10:50:43 PST
Levi Weintraub
Comment 12 2012-11-28 11:26:42 PST
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.
Robert Hogan
Comment 13 2012-11-29 04:58:28 PST
WebKit Review Bot
Comment 14 2012-11-29 12:07:44 PST
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
WebKit Review Bot
Comment 15 2012-11-29 12:54:36 PST
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
Robert Hogan
Comment 16 2012-11-29 14:29:31 PST
Robert Hogan
Comment 17 2012-12-03 11:31:58 PST
(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.
Levi Weintraub
Comment 18 2012-12-03 13:33:25 PST
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.
WebKit Review Bot
Comment 19 2012-12-04 08:59:09 PST
Comment on attachment 176818 [details] Patch Clearing flags on attachment: 176818 Committed r136513: <http://trac.webkit.org/changeset/136513>
WebKit Review Bot
Comment 20 2012-12-04 08:59:14 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.