Summary: | Clicking on the first or the last letter of LTR/RTL text in a RTL/LTR block puts caret on the opposite side | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Ryosuke Niwa <rniwa> | ||||||||||
Component: | HTML Editing | Assignee: | Ryosuke Niwa <rniwa> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, eric, jamesr, mitz, webkit.review.bot, xji | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | All | ||||||||||||
OS: | All | ||||||||||||
Attachments: |
|
Description
Ryosuke Niwa
2010-12-13 15:59:14 PST
Can you give an example of what you mean? (In reply to comment #1) > Can you give an example of what you mean? Open caret-ltr-2.html or caret-rtl-2.html and click on the first or the last letter of text. The caret is placed on the opposite side. (In reply to comment #2) > (In reply to comment #1) > > Can you give an example of what you mean? > > Open caret-ltr-2.html or caret-rtl-2.html and click on the first or the last letter of text. The caret is placed on the opposite side. I see correct behavior: in caret-ltr-2.html, if I click in the left half of the W, the insertion point appears to the right of “2”. If I click in the right half of the “2”, the insertion point appears to the left of “W”. Never mind. I wasn’t testing the right build. However, it now appears that in r73922, clicking inside a letter behaves correctly, but clicking on either side of the text behaves incorrectly. I am no longer certain http://trac.webkit.org/changeset/73548 was right. One last comment (for now): this bug is valid. It suggest to me that <http://trac.webkit.org/changeset/73548> was not the right way to fix bug 33503. Created attachment 76468 [details]
Patch
Comment on attachment 76468 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=76468&action=review > WebCore/rendering/InlineTextBox.cpp:1116 > + if ((offset == 0 || offset == m_len) && renderer()->containingBlock()->style()->isLeftToRightDirection() != isLeftToRightDirection()) I know I should be doing !offset but that looks odd in this context. Attachment 76468 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/editing/selection/caret-bidi-first-and-last-letters-expected.txt', u'LayoutTests/editing/selection/caret-bidi-first-and-last-letters.html', u'WebCore/ChangeLog', u'WebCore/rendering/InlineTextBox.cpp']" exit_code: 1
WebCore/rendering/InlineTextBox.cpp:1116: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
Total errors found: 1 in 5 files
If any of these errors are false positives, please file a bug against check-webkit-style.
(In reply to comment #5) > Never mind. I wasn’t testing the right build. However, it now appears that in r73922, clicking inside a letter behaves correctly, but clicking on either side of the text behaves incorrectly. By "behaves incorrectly", what do you exactly mean? As far as I checked, when I click on the left of text, the caret is placed on the left, and when I click on the right of text, it's placed on the right as expected regardless of direction of text or the block. Or do you expect a different behavior? (In reply to comment #11) > (In reply to comment #5) > > Never mind. I wasn’t testing the right build. However, it now appears that in r73922, clicking inside a letter behaves correctly, but clicking on either side of the text behaves incorrectly. > > By "behaves incorrectly", what do you exactly mean? As far as I checked, when I click on the left of text, the caret is placed on the left, and when I click on the right of text, it's placed on the right as expected regardless of direction of text or the block. > > Or do you expect a different behavior? No, that is the expected behavior. (In reply to comment #12) > > Or do you expect a different behavior? > > No, that is the expected behavior. Ok. Then I think http://trac.webkit.org/changeset/73548 was an improvement but I didn't take into account the case where Font::positionForPoint returns 0 or m_len, and this patch addresses those two cases. Created attachment 76475 [details]
Modified caret-ltr* caret-rtl* per Xiaomei's request
The Layout test result is confusing. Given caret-ltr-2.html as example. If I open the page in browser, When I click the left empty space of "WebKit2", the caret is placed at the left of 'W', which is correct. But the test printed out FAIL: wrong offset 7, expected 0". If I click the left half of 'W', the caret is places at very right, which is wrong. But the test printed out "PASS". The test printed out correct information if I click the right empty space of "WebKit2" or the right half of '2'. The test also prints out incorrect message when clicking inside the text. Maybe the test need to be changed to at least not printing wrong message. Thanks for changing the test. Attachment 76475 [details] did not pass style-queue:
Failed to run "['WebKitTools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/editing/selection/caret-bidi-first-and-last-letters-expected.txt', u'LayoutTests/editing/selection/caret-bidi-first-and-last-letters.html', u'LayoutTests/editing/selection/resources/caret-edge-shared.js', u'WebCore/ChangeLog', u'WebCore/rendering/InlineTextBox.cpp']" exit_code: 1
WebCore/rendering/InlineTextBox.cpp:1116: Tests for true/false, null/non-null, and zero/non-zero should all be done without equality comparisons. [readability/comparison_to_zero] [5]
Total errors found: 1 in 6 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 76476 [details]
Refactored some code
Created attachment 76558 [details]
edge cases
seems not working for some edge cases.
(In reply to comment #18) > Created an attachment (id=76558) [details] > edge cases > > seems not working for some edge cases. Yeah, we need to overhaul most of positionForPoint code for this case to work properly. However, neither of my changes regress for this edge case. So as you said, we can probably improve the behavior incrementally. Comment on attachment 76476 [details] Refactored some code Clearing flags on attachment: 76476 Committed r74971: <http://trac.webkit.org/changeset/74971> All reviewed patches have been landed. Closing bug. Thanks for the review, Dan. http://trac.webkit.org/changeset/74971 might have broken Qt Linux Release The following tests are not passing: editing/selection/caret-bidi-first-and-last-letters.html |