Bug 4672 – Incorrect highlight when selection begins with space and word-spacing>0

• Status: VERIFIED
• Resolution: FIXED
• Component: Layout and Rendering

Summary: when word-spacing is positive and the selection begins with a space, the highlight is not the 
right width (does not take into account the width of the initial space).

To reproduce: open the testcase in Safari, click in the space next to the M and drag from there to the 
right.

Expected: highlight to cover the entire space once the cursor crosses the middle of the space, and to 
extend over the letters one at a time as they are reached.

Actual: once the cursor crosses the middle of the space, a thin highlight appears near the M. As the cursor 
is dragged over the letters, the highlight becomes wider.

Created attachment 3586 [details]
testcase

Created attachment 3587 [details]
suggested fix

Comment on attachment 3587 [details]
suggested fix

Give floatWidth the preceding character so that it can account for word
spacing. (It is never given the entire string for performance reasons, see
font.cpp r1.20).

Comment on attachment 3587 [details]
suggested fix

Oops. Breaks some layout tests.

[Darin Adler]

Comment on attachment 3587 [details]
suggested fix

Is one preceding character always good enough?

I think so. It's only used by widthForNextCharacter() in WebTextRenderer.m:2241 .

Sorry, that's line 2171 in WebTextRenderer.m.

Looks like a lot of the code relies on the current behavior of not passing the preceding char (in a way that I 
don't know how to fix for negative word-spacing), so I'll try a different approach for the fix.

Created attachment 3589 [details]
add leading word space to the selection rect

Comment on attachment 3589 [details]
add leading word space to the selection rect

This uses the same logic as WebTextRenderer's widthForNextCharacter to decide
if word spacing should be added.

[Darin Adler]

Comment on attachment 3589 [details]
add leading word space to the selection rect

Looks OK, although I'd like to see a local variable to avoid the 4 calls to
textObject(). In fact, maybe that function should be made inline.

Created attachment 3606 [details]
revised patch

Made textObject() inline AND added a local variable to avoid calling it
repeatedly from selectionRect().

[Oliver Hunt]

I hate word-spacing :-/

Exists in ToT, as at 27/8/05

[Darin Adler]

Comment on attachment 3606 [details]
revised patch

Once textObject() is inlined, it needs to be in the header.

Created attachment 3623 [details]
revised patch

Moved textObject() to the header (couldn't define it inside the InlineTextBox
definition since it needs RenderText to be defined).

[Darin Adler]

Comment on attachment 3623 [details]
revised patch

r=me