| Summary: | REGRESSION(r152313): Links in certain twitter postings don't wrap correctly on page | ||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Petersen <c.petersen87> | ||||||||||||||||
| Component: | Layout and Rendering | Assignee: | zalan <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
Chris Petersen
2013-07-05 15:48:35 PDT
Created attachment 206177 [details]
attaching sample test page of twitter issue
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> </span>
</a>
</p>
</div>
</html>
regressed at http://trac.webkit.org/changeset/152313 empty inlines should not affect line-wrapping: ID 25638 (In reply to comment #3) > regressed at http://trac.webkit.org/changeset/152313 > > empty inlines should not affect line-wrapping: ID 25638 bug 25638 Created attachment 206229 [details]
test reduction
Created attachment 206230 [details]
test reduction
Created attachment 206250 [details]
Patch
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. 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 Any update when this fix is landing ? (In reply to comment #10) > Any update when this fix is landing ? will ping Dave again to take a look at it. Created attachment 206871 [details]
Patch
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)
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.
Comment on attachment 206871 [details]
Patch
r=me
Comment on attachment 206871 [details] Patch Clearing flags on attachment: 206871 Committed r153061: <http://trac.webkit.org/changeset/153061> All reviewed patches have been landed. Closing bug. |