Bug 118435

Summary: REGRESSION(r152313): Links in certain twitter postings don't wrap correctly on page
Product: WebKit Reporter: Chris Petersen <c.petersen87>
Component: Layout and RenderingAssignee: alan <zalan>
Status: RESOLVED FIXED    
Severity: Major CC: beidson, commit-queue, eric, esprehn+autocc, glenn, hyatt, leviw, robert, sam, simon.fraser, webkit-bug-importer, zalan
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Mac (Intel)   
OS: OS X 10.8   
URL: http://twitter.com
Attachments:
Description Flags
screen shot of problem. Link test doesn't wrap.
none
attaching sample test page of twitter issue
none
test reduction
none
test reduction
none
Patch
none
Patch
none
linebreaks none

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, alan 		no flags
Details
test reduction (177 bytes, text/html)
2013-07-08 04:07 PDT, alan 		no flags
Details
Patch (4.18 KB, patch)
2013-07-08 09:19 PDT, alan 		no flags
DetailsFormatted DiffDiff
Patch (4.13 KB, patch)
2013-07-17 02:38 PDT, alan 		no flags
DetailsFormatted DiffDiff
linebreaks (95.85 KB, image/jpeg)
2013-07-17 02:50 PDT, alan 		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>
alan
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
alan
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
alan
Comment 5 2013-07-08 04:04:45 PDT
Created attachment 206229 [details] test reduction
alan
Comment 6 2013-07-08 04:07:55 PDT
Created attachment 206230 [details] test reduction
alan
Comment 7 2013-07-08 09:19:56 PDT
Created attachment 206250 [details] Patch
alan
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.
alan
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 ?
alan
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.
alan
Comment 12 2013-07-17 02:38:34 PDT
Created attachment 206871 [details] Patch
alan
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)
alan
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.