Bug 9670 – REGRESSION: RTL white-space:pre-wrap text is offset to the right

• Component: Layout and Rendering
• Status: RESOLVED FIXED

Summary:
When a line of RTL text with white-space: pre-wrap is broken such that its width including the trailing space is greater than the available width, the line is rendered offset to the right.

To Reproduce:
Open the attached test case in Safari.

Expected result:
First line of text to be aligned with the right edge of the yellow rect.

Actual result:
The first line overflows the yellow rect. It is offset to the right by (almost) the width of its trailing space.

Regression:
Searching through the nightly builds, it looks like the problem appeared in r11042.

Additional information:
Pre-wrap text breaks after the space, but the width including the space is allowed to exceed the available width. RenderBlock::computeHorizontalPositionsForLine() then shrinks the text box to the available width. With LTR text, the trailing space actually overflows the "shrunk" box, but being a space, it goes unnoticed. In the RTL case, the text, including the trailing space (now on the left) is aligned with the left edge of the "shrunk" box, and the excess overflows on the right.

This is bug is critical, since it affects virtually all editable HTML, including Mail and native text fields.

The only solution that I have come up with so far is, under those circumstances (RTL && autoWrap && breakOnlyAfterWhiteSpace) to align text with the right edge of the InlineTextBox. This involves checking for those conditions in a few places in InlineTextBox, and if they hold, measuring the text and subtracting the difference between the box's width and the text width from the x offset passed to drawText and other drawing/hit testing functions.

Created attachment 9114 [details]
Test case

[David Kilzer (:ddkilzer)]

Subversion r11042 is Bug 5593. :)

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 :-( 

[Darin Adler]

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.

[Maciej Stachowiak]

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).

[Alice Liu]

<rdar://problem/4613682>

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.

[Dave Hyatt]

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.

[Dave Hyatt]

Comment on attachment 9424 [details]
Split overflowing white space into its own box

r=me

[David Kilzer (:ddkilzer)]

Committed revision 15418.