Bug 17906 - white-space: pre-wrap or -webkit-line-break: after-white-space text can overlap float at end of line
Summary: white-space: pre-wrap or -webkit-line-break: after-white-space text can overl...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: mitz
URL:
Keywords: HasReduction, InRadar
Depends on:
Blocks:
 
Reported: 2008-03-17 20:50 PDT by mitz
Modified: 2008-07-25 13:22 PDT (History)
1 user (show)

See Also:


Attachments
Test case (791 bytes, text/html)
2008-03-17 20:50 PDT, mitz
no flags Details
Fix for floats added in trailing whitespace for an already full linebox (2.08 KB, patch)
2008-07-15 13:17 PDT, Jacob Refstrup
mitz: review-
Details | Formatted Diff | Diff
Final patch with layout test added and feedback from mitz incorporated (8.15 KB, patch)
2008-07-21 09:44 PDT, Jacob Refstrup
mitz: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description mitz 2008-03-17 20:50:12 PDT
A float occurring in the trailing whitespace of a line with white-space: pre-wrap or -webkit-line-break: after-white-space (editable HTML uses the latter by default) is not pushed down to the next line if it does not fit.

See attached test case.
Comment 1 mitz 2008-03-17 20:50:29 PDT
Created attachment 19857 [details]
Test case
Comment 2 mitz 2008-03-18 13:32:06 PDT
<rdar://problem/5805741>
Comment 3 Jacob Refstrup 2008-07-15 13:17:09 PDT
Created attachment 22285 [details]
Fix for floats added in trailing whitespace for an already full linebox

The root cause seems to be that the call to skipWhitespace(InlineIterator&) when the line box is already full causes floats to be added (correct) but also positioned (incorrect); the positioning should be deferred until newLine() is called.

I've regressed this on gtk without issues (but then again gtk doesn't seem to dump render trees) but will need a volunteer to check it on MacOSX.
Comment 4 mitz 2008-07-19 11:49:43 PDT
Comment on attachment 22285 [details]
Fix for floats added in trailing whitespace for an already full linebox

Looks good, but I think it would help, instead of the comments, to distinguish between the two skipWhitespace functions by renaming them to skipTrailingWhitespace and skipLeadingWhitespace. The former would also not need to return the available width.
Comment 5 mitz 2008-07-20 14:26:23 PDT
Comment on attachment 22285 [details]
Fix for floats added in trailing whitespace for an already full linebox

r- for now because I think it's important to distinguish between the functions (see previous comment).
Comment 6 Jacob Refstrup 2008-07-20 19:28:52 PDT
(In reply to comment #5)
> (From update of attachment 22285 [details] [edit])
> r- for now because I think it's important to distinguish between the functions
> (see previous comment).
> 

Thx for the feedback -- I'll update the patch tomorrow when time allows.
- Jacob
Comment 7 Jacob Refstrup 2008-07-21 09:44:40 PDT
Created attachment 22406 [details]
Final patch with layout test added and feedback from mitz incorporated

Added test case (editable-text-overlapping-float.html) and expected output (for Qt)
Renamed functions per mitz's suggestions.
Removed return value from skipTrailingWhitespace as it's not used.
Comment 8 mitz 2008-07-21 11:09:27 PDT
Comment on attachment 22406 [details]
Final patch with layout test added and feedback from mitz incorporated

Looks good, but cannot be landed as-is: the css2.1 directory contains a standard test suite; the new test should go in fast/block/float. Expected results (render tree and bitmap) will for Mac OS X will need to be generated too. I am going to take care of those things when landing the patch.
Comment 9 mitz 2008-07-25 13:22:04 PDT
Landed in <http://trac.webkit.org/changeset/35354>.