Bug 4672

Summary: Incorrect highlight when selection begins with space and word-spacing>0
Product: WebKit Reporter: mitz
Component: Layout and RenderingAssignee: Dave Hyatt <hyatt>
Status: VERIFIED FIXED    
Severity: Normal    
Priority: P2    
Version: 420+   
Hardware: Mac   
OS: OS X 10.4   
Attachments:
Description Flags
testcase
none
suggested fix
none
add leading word space to the selection rect
darin: review+
revised patch
darin: review-
revised patch darin: review+

Description mitz 2005-08-26 10:20:39 PDT
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.
Comment 1 mitz 2005-08-26 10:21:18 PDT
Created attachment 3586 [details]
testcase
Comment 2 mitz 2005-08-26 11:09:07 PDT
Created attachment 3587 [details]
suggested fix
Comment 3 mitz 2005-08-26 11:11:29 PDT
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 4 mitz 2005-08-26 11:21:21 PDT
Comment on attachment 3587 [details]
suggested fix

Oops. Breaks some layout tests.
Comment 5 Darin Adler 2005-08-26 11:24:44 PDT
Comment on attachment 3587 [details]
suggested fix

Is one preceding character always good enough?
Comment 6 mitz 2005-08-26 11:33:18 PDT
I think so. It's only used by widthForNextCharacter() in WebTextRenderer.m:2241 .
Comment 7 mitz 2005-08-26 13:24:12 PDT
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.
Comment 8 mitz 2005-08-26 14:19:02 PDT
Created attachment 3589 [details]
add leading word space to the selection rect
Comment 9 mitz 2005-08-26 14:22:42 PDT
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.
Comment 10 Darin Adler 2005-08-26 19:35:24 PDT
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.
Comment 11 mitz 2005-08-27 03:11:38 PDT
Created attachment 3606 [details]
revised patch

Made textObject() inline AND added a local variable to avoid calling it
repeatedly from selectionRect().
Comment 12 Oliver Hunt 2005-08-27 04:30:19 PDT
I hate word-spacing :-/

Exists in ToT, as at 27/8/05
Comment 13 Darin Adler 2005-08-27 11:44:20 PDT
Comment on attachment 3606 [details]
revised patch

Once textObject() is inlined, it needs to be in the header.
Comment 14 mitz 2005-08-28 07:23:23 PDT
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).
Comment 15 Darin Adler 2005-08-28 11:08:41 PDT
Comment on attachment 3623 [details]
revised patch

r=me