Bug 65356

Summary: positionForPoint returns wrong VisiblePosition at bidi boundaries
Product: WebKit Reporter: Ryosuke Niwa <rniwa>
Component: UI EventsAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, enrica, mitz, playmobil, webkit.review.bot, xji
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 57340    
Attachments:
Description Flags
test cases
none
work in progress
none
fixes the bug
none
fixed qt build failure hyatt: review+

Ryosuke Niwa
Reported 2011-07-28 19:42:42 PDT
Reproduction steps: 1. Paste "‎לשנת12" into a LTR block; e.g. http://simple-rte.rniwa.com/?editor=%3Cspan%20style%3D%22font-size%3A%205em%3Bpadding%3A%2020px%3B%22%3E%u200E%u05DC%u05E9%u05E0%u05EA12%3C/span%3E&dir=ltr&designmode=false&script=document.getElementById%28%27editor%27%29.focus%28%29%3B 2. Click on the right of ל. 3. Observe that the caret is placed on the left of 1. Expected result: Caret is placed on the right of ל.
Attachments
test cases (2.04 KB, text/html)
2011-07-28 19:44 PDT, Ryosuke Niwa 		no flags
Details
work in progress (9.19 KB, patch)
2011-07-28 19:44 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
fixes the bug (17.82 KB, patch)
2011-08-01 18:32 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
fixed qt build failure (17.79 KB, patch)
2011-08-01 18:56 PDT, Ryosuke Niwa 		hyatt: review+
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Ryosuke Niwa
Comment 1 2011-07-28 19:44:09 PDT
Created attachment 102328 [details] test cases Here's some test cases (offset values are obtained on ToT WebKit with my work-in-progress patch).
Ryosuke Niwa
Comment 2 2011-07-28 19:44:49 PDT
Created attachment 102329 [details] work in progress
WebKit Review Bot
Comment 3 2011-07-28 19:47:14 PDT
Attachment 102329 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebCore/rendering/InlineTextBox.cpp..." exit_code: 1 Source/WebCore/rendering/RenderText.cpp:498: This { should be at the end of the previous line [whitespace/braces] [4] Source/WebCore/rendering/RenderText.cpp:526: This { should be at the end of the previous line [whitespace/braces] [4] Total errors found: 2 in 2 files If any of these errors are false positives, please file a bug against check-webkit-style.
Early Warning System Bot
Comment 4 2011-07-28 19:53:01 PDT
Comment on attachment 102329 [details] work in progress Attachment 102329 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9262408
WebKit Review Bot
Comment 5 2011-07-28 20:18:30 PDT
Comment on attachment 102329 [details] work in progress Attachment 102329 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/9266371
WebKit Review Bot
Comment 6 2011-07-28 20:32:35 PDT
Comment on attachment 102329 [details] work in progress Attachment 102329 [details] did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/9266377
WebKit Review Bot
Comment 7 2011-07-28 20:49:39 PDT
Comment on attachment 102329 [details] work in progress Attachment 102329 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9267354
WebKit Review Bot
Comment 8 2011-07-28 21:37:37 PDT
Comment on attachment 102329 [details] work in progress Attachment 102329 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9265372
Ryosuke Niwa
Comment 9 2011-08-01 18:32:37 PDT
Created attachment 102608 [details] fixes the bug
Early Warning System Bot
Comment 10 2011-08-01 18:41:59 PDT
Comment on attachment 102608 [details] fixes the bug Attachment 102608 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/9289500
Ryosuke Niwa
Comment 11 2011-08-01 18:56:42 PDT
Created attachment 102610 [details] fixed qt build failure
Jeremy Moskovich
Comment 12 2011-08-02 04:27:41 PDT
With the patch applied, when running the attached testcase when I click between the "ת" and the 2 in לשנת12 then the caret is placed to the right of the ל instead of between the ת and the 2 like I'd expect.
Ryosuke Niwa
Comment 13 2011-08-02 09:12:34 PDT
(In reply to comment #12) > With the patch applied, when running the attached testcase when I click between the "ת" and the 2 in לשנת12 then the caret is placed to the right of the ל instead of between the ת and the 2 like I'd expect. Right. That's because there's no caret position between ת and 2 in WebKit. Offsets are assigned as: (0)(4)1(5)2D(3)C(2)B(1)A(6)
Jeremy Moskovich
Comment 14 2011-08-02 21:53:34 PDT
In TextEdit and FF I can place the caret between the ת and the 2 .
Ryosuke Niwa
Comment 15 2011-08-02 23:35:29 PDT
(In reply to comment #14) > In TextEdit and FF I can place the caret between the ת and the 2 . You need to set the writing direction to LTR (Format > Text > Writing Direction) first.
Ryosuke Niwa
Comment 16 2011-08-16 16:07:15 PDT
Ping reviewers.
Dave Hyatt
Comment 17 2011-08-18 11:55:30 PDT
Comment on attachment 102610 [details] fixed qt build failure r=me
Ryosuke Niwa
Comment 18 2011-08-18 12:04:33 PDT
(In reply to comment #17) > (From update of attachment 102610 [details]) > r=me Thanks for the review! I'm very excited to land this patch.
Ryosuke Niwa
Comment 19 2011-08-18 16:07:19 PDT
Committed r93369: <http://trac.webkit.org/changeset/93369>
Note You need to log in before you can comment on or make changes to this bug.