Summary: | REGRESSION: RTL white-space:pre-wrap text is offset to the right | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | mitz | ||||||||||||||||
Component: | Layout and Rendering | Assignee: | Nobody <webkit-unassigned> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Critical | Keywords: | InRadar, Regression | ||||||||||||||||
Priority: | P1 | ||||||||||||||||||
Version: | 420+ | ||||||||||||||||||
Hardware: | Mac | ||||||||||||||||||
OS: | OS X 10.4 | ||||||||||||||||||
Attachments: |
|
Description
mitz
2006-06-30 16:02:03 PDT
Created attachment 9114 [details]
Test case
Created attachment 9119 [details]
Align RTL white-space:pre text with the right edge of the text box
Note that there is a possible time/space tradeoff here: RenderBlock::computeHorizontalPositionsForLine() knows the offsetForAlignRight() when it sets the InlineTextBox's width, so an alternative fix would involve adding this offset as an InlineTextBox member and having computeHorizontalPositionsForLine() set it.
Yet another tradeoff point is to add just a boolean to InlineTextBox -- to be set by computeHorizontalPositionsForLine() -- that will indicate whether offsetForAlignRight() caclulation is necessary. Maybe m_toAdd (currently 13 bits) can spare a bit. (In reply to comment #2) > Subversion r11042 is Bug 5593. :) While the test case for this bug does not fail in shipping WebKit, I am pretty sure that its root cause is present... Contenteditable RTL text definitely misbehaves in shipping WebKit :-( Comment on attachment 9119 [details]
Align RTL white-space:pre text with the right edge of the text box
offsetForAlignRight is so long it should probably not be all in a single line in InlineTextBox.h.
I don't understand the performance impact here, so I'm not comfortable saying review+, otherwise I would have said r=me.
Comment on attachment 9119 [details]
Align RTL white-space:pre text with the right edge of the text box
I agree with darin's comment about offsetForAlignRight.
I think if there is extra space around in an existing bitfield, it is ok to use one of those bits. However, this should be justified with some kind of measurement of the perf impact. I think since m_reversed is checked first, then at least the LTR case should be optimized already, but perhaps in many RTL cases the other calculations can be skipped.
r- so you can decide what to do here but I think the patch is good to go either way (assuming I am right that LTR should normally be unaffected).
Created attachment 9211 [details]
Reformatted inline function
Only changed the formatting. As Maciej said, the impact on LTR text is limited to the m_reversed check and to adding 0 to the offset. For RTL text that this doesn't apply to, there are another couple of checks against the style. The serious impact is the unnecessary width calculation for RTL text that is indeed white-space:pre-wrap and breaks after white space, but whose box wasn't truncated (and therefore doesn't need the offset).
Created attachment 9214 [details]
Add an m_alignRight flag to InlineTextBox
I think this version performs better since it measures the width only if it's really needed, even for RTL text with the relevant style (whereas the other approach measures the width of all RTL text with that style).
Can m_toAdd be negative? If it can't, then the sign bit can be reclaimed.
Comment on attachment 9214 [details]
Add an m_alignRight flag to InlineTextBox
I think a slicker approach that would work for LTR and RTL (and thus avoid having to do this special case code just for RTL) would be to take the problematic white space that is truncated at the edge of the line and put it in its own separate text box.
Created attachment 9405 [details]
Split overflowing white space into its own box
Did as Hyatt suggested :-)
There is a remaining issue with the trailing white space of right-aligned LTR text (and other conditions involving RTL), but I think it belongs in a separate lower-priority bug.
Created attachment 9406 [details]
Something else that the patch fixes
This test case has been lying in my "to do" folder since January. Turns out the patch for this bug fixes it.
Comment on attachment 9406 [details]
Something else that the patch fixes
...or perhaps just improves it.
Created attachment 9424 [details]
Split overflowing white space into its own box
Moved the totWidth > availableWidth check to the beginning of the condition.
Comment on attachment 9424 [details]
Split overflowing white space into its own box
r=me
Committed revision 15418. |