Bug 28284 – Chromium Linux: fix caret positioning in LTR complex languages

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

[Adam Langley]

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.

[Adam Langley]

Created attachment 34796 [details]
patch

[Eric Seidel (no email)]

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.)

[Hironori Bono]

(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?

[Adam Langley]

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! :)

[Evan Martin]

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.

[Evan Martin]

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.

[Evan Martin]

Created attachment 41697 [details]
patch with better changelog

[Eric Seidel (no email)]

CCing mitz so he sees this go by.  Looks good to me though.

[Eric Seidel (no email)]

Comment on attachment 41697 [details]
patch with better changelog

LGTM.

[WebKit Commit Bot]

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

[Evan Martin]

Dear Eric,

This code is only used on a platform that isn't ran by run-webkit-tests.

[Eric Seidel (no email)]

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.

[Hironori Bono]

(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

[Evan Martin]

That would be great!  My demo above could have been made into a layout test.

[WebKit Commit Bot]

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

[Eric Seidel (no email)]

Comment on attachment 41697 [details]
patch with better changelog

Sorry about the commit-queue trouble.  Should work fine now.

[WebKit Commit Bot]

Comment on attachment 41697 [details]
patch with better changelog

Clearing flags on attachment: 41697

Committed r49994: <http://trac.webkit.org/changeset/49994>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.