Summary: | Chromium Linux: fix caret positioning in LTR complex languages | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Adam Langley <agl> | ||||||||
Component: | Layout and Rendering | Assignee: | Evan Martin <evan> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | ap, commit-queue, eric, hbono, mitz | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | PC | ||||||||||
OS: | Linux | ||||||||||
Attachments: |
|
Description
Adam Langley
2009-08-13 15:38:52 PDT
Created attachment 34796 [details]
patch
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.)
(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? 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! :) 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.
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. Created attachment 41697 [details]
patch with better changelog
CCing mitz so he sees this go by. Looks good to me though. Comment on attachment 41697 [details]
patch with better changelog
LGTM.
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
Dear Eric, This code is only used on a platform that isn't ran by run-webkit-tests. 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. (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 That would be great! My demo above could have been made into a layout test. 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 on attachment 41697 [details]
patch with better changelog
Sorry about the commit-queue trouble. Should work fine now.
Comment on attachment 41697 [details] patch with better changelog Clearing flags on attachment: 41697 Committed r49994: <http://trac.webkit.org/changeset/49994> All reviewed patches have been landed. Closing bug. |