WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
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
Details
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
Details
Formatted Diff
Diff
Patch
(5.56 KB, patch)
2012-10-09 12:29 PDT
,
Robert Hogan
no flags
Details
Formatted Diff
Diff
Patch
(5.27 KB, patch)
2012-10-13 01:35 PDT
,
Robert Hogan
no flags
Details
Formatted Diff
Diff
Patch
(5.43 KB, patch)
2012-10-24 11:12 PDT
,
Robert Hogan
no flags
Details
Formatted Diff
Diff
Patch
(5.49 KB, patch)
2012-11-25 12:50 PST
,
Robert Hogan
no flags
Details
Formatted Diff
Diff
Patch
(5.49 KB, patch)
2012-11-26 10:50 PST
,
Robert Hogan
no flags
Details
Formatted Diff
Diff
Patch
(5.47 KB, patch)
2012-11-29 04:58 PST
,
Robert Hogan
no flags
Details
Formatted Diff
Diff
Patch
(5.74 KB, patch)
2012-11-29 14:29 PST
,
Robert Hogan
no flags
Details
Formatted Diff
Diff
Show Obsolete
(7)
View All
Add attachment
proposed patch, testcase, etc.
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
Created
attachment 167818
[details]
Patch
Robert Hogan
Comment 4
2012-10-13 01:35:23 PDT
Created
attachment 168548
[details]
Patch
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
Created
attachment 170433
[details]
Patch
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
Created
attachment 175888
[details]
Patch
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
Created
attachment 176034
[details]
Patch
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
Created
attachment 176707
[details]
Patch
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
Created
attachment 176818
[details]
Patch
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.
Top of Page
Format For Printing
XML
Clone This Bug