|Summary:||Incorrect highlight when selection begins with space and word-spacing>0|
|Component:||Layout and Rendering||Assignee:||Dave Hyatt <hyatt>|
|OS:||OS X 10.4|
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 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).