RESOLVED FIXED 118435
REGRESSION(r152313): Links in certain twitter postings don't wrap correctly on page 
https://bugs.webkit.org/show_bug.cgi?id=118435
Summary REGRESSION(r152313): Links in certain twitter postings don't wrap correctly o...
Chris Petersen
Reported 2013-07-05 15:48:35 PDT
Created attachment 206173 [details] screen shot of problem. Link test doesn't wrap. REGRESSION: Links in certain twitter postings don't warp correctly on page. I'm seeing this on several of twitter posting such as Walt Mossberg and Fox News. This first occurs with WebKit r152405 but this worked on previous Webkit NB r152225. I have a reduced test case I'm working one now. 1) Launch WebKit r152405 and signing to your Twitter account 2) Scroll down list of twitter posting and look for "Walt Mossberg - FCC Approves SoftBank Takeover of Sprint, Clearwire ". Notice the link doesn't wrap down.
Attachments
screen shot of problem. Link test doesn't wrap. (94.36 KB, image/png)
2013-07-05 15:48 PDT, Chris Petersen 		no flags
Details
attaching sample test page of twitter issue (399 bytes, text/html)
2013-07-05 16:24 PDT, Chris Petersen 		no flags
Details
test reduction (166 bytes, text/html)
2013-07-08 04:04 PDT, zalan 		no flags
Details
test reduction (177 bytes, text/html)
2013-07-08 04:07 PDT, zalan 		no flags
Details
Patch (4.18 KB, patch)
2013-07-08 09:19 PDT, zalan 		no flags
DetailsFormatted DiffDiff
Patch (4.13 KB, patch)
2013-07-17 02:38 PDT, zalan 		no flags
DetailsFormatted DiffDiff
linebreaks (95.85 KB, image/jpeg)
2013-07-17 02:50 PDT, zalan 		no flags
Details
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Chris Petersen
Comment 1 2013-07-05 16:24:07 PDT
Created attachment 206177 [details] attaching sample test page of twitter issue
Chris Petersen
Comment 2 2013-07-05 16:25:21 PDT
Here is what the HTML structure of the test case: <!DOCTYPE html> <head> <title>Twitter</title> <style type="text/css"> .content-main{float:right;width:522px} </style> </head> <body> <div class="content-main"> <div> </div> <div> <p>FCC Approves SoftBank Takeover of Sprint Clearwire <a href="/DelRey">@DelRey</a> <a href="http://t.co/B9okl3g4kk"> dthin.gs/13q6ZYi<span></span><span>&nbsp;</span> </a> </p> </div> </html>
zalan
Comment 3 2013-07-08 02:33:22 PDT
regressed at http://trac.webkit.org/changeset/152313 empty inlines should not affect line-wrapping: ID 25638
zalan
Comment 4 2013-07-08 02:35:24 PDT
(In reply to comment #3) > regressed at http://trac.webkit.org/changeset/152313 > > empty inlines should not affect line-wrapping: ID 25638 bug 25638
zalan
Comment 5 2013-07-08 04:04:45 PDT
Created attachment 206229 [details] test reduction
zalan
Comment 6 2013-07-08 04:07:55 PDT
Created attachment 206230 [details] test reduction
zalan
Comment 7 2013-07-08 09:19:56 PDT
Created attachment 206250 [details] Patch
zalan
Comment 8 2013-07-08 09:24:55 PDT
zalan: rhogan: do you have a test case for this? -> http://trac.webkit.org/browser/trunk/Source/WebCore/rendering/RenderBlockLineLayout.cpp#L2910 zalan: rhogan: wondering as it introduced regression and the testcase that came with the commit passes even when this line is removed. ------ // If we are an empty inline in the middle of a word and don't fit on the line then clear any line break we have and find // one in the following text instead. if (!canPlaceOnLine && !canBreakHere && isEmptyInline(current.m_obj)) lBreak.clear(); ---------- It would be great to see a test case for this^ behaviour. Sadly, the comment does not say why it is done this way. As a side note, even with this regression, the line should break at a latter position. I'll figure it out why it does not.
zalan
Comment 9 2013-07-10 23:10:36 PDT
rhogan: zalan: your findings match mine - when I wrote the patch that line was required to pass the test unless I was careless, which is also possible rhogan: zalan: so yes, that line is superfluous and can be removed - I must have gotten confused while testing and convinced myself it was required
Chris Petersen
Comment 10 2013-07-11 10:42:52 PDT
Any update when this fix is landing ?
zalan
Comment 11 2013-07-11 10:53:53 PDT
(In reply to comment #10) > Any update when this fix is landing ? will ping Dave again to take a look at it.
zalan
Comment 12 2013-07-17 02:38:34 PDT
Created attachment 206871 [details] Patch
zalan
Comment 13 2013-07-17 02:42:20 PDT
alternatively, If we prefer breaking the line after the empty inline, we should fix it like this: if (!canPlaceOnLine && !canBreakHere && isEmptyInline(current.m_obj)) - lBreak.clear(); + lBreak.m_pos = 0; else if (canPlaceOnLine && canBreakHere) so that we can still break on the line. (as right now, with lBreak.clear(), we not only clear the undesired position, but also the m_obj which confuses the breaking logic at this point)
zalan
Comment 14 2013-07-17 02:50:05 PDT
Created attachment 206872 [details] linebreaks 1. line break before the empty inline (the current patch does this -match FF and Opera) 2, line break after the empty inline (with the alternative lBreak.m_pos = 0; change) 3, trunk behaviour, no break at all.
Dave Hyatt
Comment 15 2013-07-23 12:13:46 PDT
Comment on attachment 206871 [details] Patch r=me
Radar WebKit Bug Importer
Comment 16 2013-07-23 12:18:50 PDT
<rdar://problem/14524323>
WebKit Commit Bot
Comment 17 2013-07-23 12:46:17 PDT
Comment on attachment 206871 [details] Patch Clearing flags on attachment: 206871 Committed r153061: <http://trac.webkit.org/changeset/153061>
WebKit Commit Bot
Comment 18 2013-07-23 12:46:20 PDT
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.