Bug 50315 – Redrawing issue with inserting new inline element between existing inline elements

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[temp01]

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)

[bhavaniab]

Created attachment 136418 [details]
patch for redrawing issue with inserting inline element between existing inline elements

[Adam Barth]

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]

Created attachment 167818 [details]
Patch

[Robert Hogan]

Created attachment 168548 [details]
Patch

[Ken Buchanan]

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]

(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]

Created attachment 170433 [details]
Patch

[Levi Weintraub]

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]

Created attachment 175888 [details]
Patch

[WebKit Review Bot]

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]

Created attachment 176034 [details]
Patch

[Levi Weintraub]

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]

Created attachment 176707 [details]
Patch

[WebKit Review Bot]

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 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]

Created attachment 176818 [details]
Patch

[Robert Hogan]

(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 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 on attachment 176818 [details]
Patch

Clearing flags on attachment: 176818

Committed r136513: <http://trac.webkit.org/changeset/136513>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.