Bug 28284 - Chromium Linux: fix caret positioning in LTR complex languages
Summary: Chromium Linux: fix caret positioning in LTR complex languages
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Evan Martin
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-08-13 15:38 PDT by Adam Langley
Modified: 2009-10-23 13:26 PDT (History)
5 users (show)

See Also:


Attachments
patch (1.33 KB, patch)
2009-08-13 15:41 PDT, Adam Langley
eric: review-
Details | Formatted Diff | Diff
demo of the problem (not a layout test) (573 bytes, text/html)
2009-10-22 16:45 PDT, Evan Martin
no flags Details
patch with better changelog (1.36 KB, patch)
2009-10-22 16:50 PDT, Evan Martin
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Langley 2009-08-13 15:38:52 PDT
As pointed out in http://code.google.com/p/chromium/issues/detail?id=16279, the caret position for complex LTR languages (like Thai) is wrong: we shouldn't be adding the advance width to the position.
Comment 1 Adam Langley 2009-08-13 15:41:22 PDT
Created attachment 34796 [details]
patch
Comment 2 Eric Seidel (no email) 2009-08-13 20:18:59 PDT
Comment on attachment 34796 [details]
patch

We can test selection rects.  Why doesn't this affect tests?

(Also, I don't think you want to mark me as the requestee.)
Comment 3 Hironori Bono 2009-09-01 02:50:43 PDT
(In reply to comment #2)
> (From update of attachment 34796 [details])
> We can test selection rects.  Why doesn't this affect tests?

The answer for this question is simple: there are not any tests in LayoutTests that tests the cursor position of a complex text, i.e. there are not any tests that calls this selectionRectForComplexText(). Thus, changing this function doesn't affect any existing tests. (This is also the reason why we ware not able to be aware of <http://crbug.com/16279> until a user reported it.)

Shall I write a new test that verifies this issue?
Comment 4 Adam Langley 2009-09-01 10:30:53 PDT
Writing the test isn't too hard: but I'm afraid that generating all the needed baselines pushed this bug below my threshold. If you care enough to do all that work, please go right ahead! :)
Comment 5 Evan Martin 2009-10-22 16:45:13 PDT
Created attachment 41696 [details]
demo of the problem (not a layout test)

Here was the test page I was playing with in trying to make a test.

It describes what goes wrong.
Comment 6 Evan Martin 2009-10-22 16:46:15 PDT
agl, eseidel, and I spent some time looking at this trying to come up with a way to test it.  Unfortunately the bug is only in how the cursor is drawn, not in selections, and dumpEditingCallbacks doesn't help either.
Comment 7 Evan Martin 2009-10-22 16:50:01 PDT
Created attachment 41697 [details]
patch with better changelog
Comment 8 Eric Seidel (no email) 2009-10-22 16:53:44 PDT
CCing mitz so he sees this go by.  Looks good to me though.
Comment 9 Eric Seidel (no email) 2009-10-22 16:53:58 PDT
Comment on attachment 41697 [details]
patch with better changelog

LGTM.
Comment 10 WebKit Commit Bot 2009-10-22 17:24:28 PDT
Comment on attachment 41697 [details]
patch with better changelog

Rejecting patch 41697 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11484 test cases.
fast/media/mq-transform-02.html -> failed

Exiting early after 1 failures. 7441 tests run.
135.06s total testing time

7440 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
3 test cases (<1%) had stderr output
Comment 11 Evan Martin 2009-10-22 17:57:41 PDT
Dear Eric,

This code is only used on a platform that isn't ran by run-webkit-tests.
Comment 12 Eric Seidel (no email) 2009-10-22 18:04:34 PDT
Comment on attachment 41697 [details]
patch with better changelog

See bug 30700.  I worry the commit-queue could be horked and I may need to disable it for this evening until I can fix the QuickTime version check in DumpRenderTree.
Comment 13 Hironori Bono 2009-10-22 18:44:42 PDT
(In reply to comment #6)
> agl, eseidel, and I spent some time looking at this trying to come up with a
> way to test it.  Unfortunately the bug is only in how the cursor is drawn, not
> in selections, and dumpEditingCallbacks doesn't help either.

To write an automated test for this case, I have been writing a WebKit patch that adds a function to retrieve the pixel-level cursor position into TextInputController.
Even though I'm wondering if it is still useful for you, is it acceptable to file another bug and attach my patch?

Regards,

Hironori Bono
Comment 14 Evan Martin 2009-10-22 18:46:47 PDT
That would be great!  My demo above could have been made into a layout test.
Comment 15 WebKit Commit Bot 2009-10-22 21:35:42 PDT
Comment on attachment 41697 [details]
patch with better changelog

Rejecting patch 41697 from commit-queue.

Failed to run "['WebKitTools/Scripts/run-webkit-tests', '--no-launch-safari', '--quiet', '--exit-after-n-failures=1']" exit_code: 1
Running build-dumprendertree
Running tests from /Users/eseidel/Projects/CommitQueue/LayoutTests
Testing 11484 test cases.
fast/media/mq-transform-02.html -> failed

Exiting early after 1 failures. 7441 tests run.
130.30s total testing time

7440 test cases (99%) succeeded
1 test case (<1%) had incorrect layout
3 test cases (<1%) had stderr output
Comment 16 Eric Seidel (no email) 2009-10-23 12:04:52 PDT
Comment on attachment 41697 [details]
patch with better changelog

Sorry about the commit-queue trouble.  Should work fine now.
Comment 17 WebKit Commit Bot 2009-10-23 13:26:17 PDT
Comment on attachment 41697 [details]
patch with better changelog

Clearing flags on attachment: 41697

Committed r49994: <http://trac.webkit.org/changeset/49994>
Comment 18 WebKit Commit Bot 2009-10-23 13:26:22 PDT
All reviewed patches have been landed.  Closing bug.