Closed Bug 1354478 Opened 4 years ago Closed 4 years ago

Caret sometimes covers part of the text before it

Categories

(Core :: Layout: Text and Fonts, defect)

52 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: dr.khaled.hosny, Assigned: jfkthame)

Details

Attachments

(3 files, 3 obsolete files)

I’m not sure if this is new, but I have been noticing that when editing Arabic text the cursor often covers part of the previous character making it rather hard to tell which two characters the cursor is currently between.

Now it seems this is a result of some interaction between the text direction and the direction of the textarea. If the direction of the the two is different (i.e. LTR text area and RTL text and vice versa) then the cursor covers the end of the character before it (i.e. the character to the right or to the left based on direction). If the two agree on the direction, then the cursor is placed as expected.

In the attached HTML file, the difference can be seen between the 1st textarea (LTR with LTR text) and the 2nd (RTL with LTR text) as well as between the 3rd (RTL with RTL text) and the 4th (LTR with RTL text).
I think I see what's going on here. I've put together a patch that ought to help; when the try builds at https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c2b55ad6db4eb8bd31e03ea5f8099ec5c348198 are ready, could you please test and confirm whether this fixes the issue you're seeing? Thanks.
Attachment #8855839 - Flags: feedback?(khaledhosny)
Attachment #8855839 - Attachment is obsolete: true
Attachment #8855839 - Flags: feedback?(khaledhosny)
Is there a way to test this without building locally?
If you're on Linux64, the most recent try run (fixing crashes that occurred with the previous one) is at https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7fe3ed750d2197277a225a4612a0eba9c29187d; the actual build is at https://queue.taskcluster.net/v1/task/Es7pgoAgSOKi257c_3Op4Q/runs/0/artifacts/public/build/target.tar.bz2, as found by expanding the taskcluster (tc) job group, clicking on the "B" (build) and looking for the " artifact uploaded: target.tar.bz2 " link in the Job Details panel. Yeah, it's a bit obscure!

So if you download and expand that "target" archive, you should be able to run the Firefox (nightly + patch) build from there and check the behavior.

(And if you need a different platform, let me know and I'll fire off try builds as appropriate...)
Thanks Jonathan, I’m glad I asked :) local build was going to take a while.

With this build the caret position seems to be right in the four cases.
Attachment #8855914 - Flags: feedback?(khaledhosny) → feedback+
Attachment #8855914 - Attachment is obsolete: true
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
(In reply to Khaled Hosny from comment #0)
> In the attached HTML file, the difference can be seen between the 1st
> textarea (LTR with LTR text) and the 2nd (RTL with LTR text) as well as
> between the 3rd (RTL with RTL text) and the 4th (LTR with RTL text).

I didn't initially see the difference here, but now I do -- it's very subtle! (to my eye & on my OS at least, Ubuntu 16.04)

Here are specific Steps To Reproduce, for anyone else testing this down the line:
 0. Load https://bugzilla.mozilla.org/attachment.cgi?id=8855705
 1. Click between the first two "a" characters in the first box.
 2. Click between the first two "a" characters in the second box.
 3. (optional) press Shift+Tab to jump back to first box, and then Tab to jump to second box again, to compare the cursor position.
 4. Compare the position of the cursor relative to the first "a" in each of those boxes.

EXPECTED RESULTS:
 Consistent position.

ACTUAL RESULTS:
 The cursor is further to the left in the second box, as compared to the first.
We should be able to add a reftest based directly on this testcase, so that if we were to accidentally regress this in future, we'd know about it. I'll put up a patch to add such a test.
Comment on attachment 8856390 [details] [diff] [review]
Adjust caret rect to paint on the opposite side of the nominal caret position in "upstream" text, for more visually consistent placement

Review of attachment 8856390 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/nsCaret.cpp
@@ +350,5 @@
> +    // frame's inline-dir (or in sideways-lr mode where "normal" inline dir
> +    // is reversed from physical) we want the caret to be painted before rather
> +    // than after its nominal inline position, so we offset by its width.
> +    if (textRun && (wm.IsInlineReversed() != textRun->IsInlineReversed())
> +                     != textRun->IsSidewaysLeft()) {

Three things:
(1) I find this condition very hard to reason about.  Could you break down this condition a bit, so it's easier to grok?

e.g. I think it could be refactored into the following:
  if (textRun) {
    bool textRunDirIsReverseOfFrame =
      wm.IsInlineReversed() != textRun->IsInlineReversed();
    if (textRunDirIsReverseOfFrame != textRun->IsSidewaysLeft()) {

(2) Even with that ^^, I don't immediately grok the "!= IsSidewaysLeft" part -- it's not clear to me how that maps onto what the comment says about sideways-left.  (The comment phrases it as "or in sideways-lr mode ...", which I can't immediately map onto this inequality.)  Could you adjust the logic and/or clarify the comment so that the connection is clearer?

(3) It doesn't look like this patch's test-tweaks would exercise the sideways-left logic here. Could you add a test for that?
Flags: needinfo?(jfkthame)
I hope this is a little easier to understand (functionally equivalent). BTW, it may make more sense if you realize that our caret position in sideways-lr mode is currently off by one pixel -except- in the upstream-textrun case (i.e. it suffers from the inverse of this bug), which is why here, we give sideways-lr the inverse treatment to all the other ('normal') modes.
Attachment #8857188 - Flags: review?(dholbert)
Attachment #8856390 - Attachment is obsolete: true
Attachment #8856390 - Flags: review?(dholbert)
Summary: Cursor sometimes covers part of the text before it → Caret sometimes covers part of the text before it
Comment on attachment 8857188 [details] [diff] [review]
Adjust caret rect to paint on the opposite side of the nominal caret position in "upstream" text, for more visually consistent placement

Review of attachment 8857188 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks - this is indeed easier to reason about.

I can confirm that the caret behavior seems opposite of normal in sideways-lr mode right now (i.e. it's "before"-hugging), after viewing this testcase and zooming in and clicking between two characters:
 data:text/html,<textarea style="writing-mode: sideways-lr; direction: rtl">aaaaa
...though it sounds like this patch is fixing that (hooray)!

::: layout/base/tests/bug646382-1-ref.html
@@ +10,5 @@
>        var textarea = document.querySelector("textarea");
>        function start() {
>          textarea.focus();
> +        textarea.selectionStart = 1;
> +        textarea.selectionEnd = 1;

Could you add a comment explaining what you're doing here? (It's a bit obscure, since this is "selectionStart/End" but you're not actually testing selections.)

E.g. if this is correct, maybe the following:
  // Place caret after the character.

(Same goes for the corresponding reference case, since you're making the same change there.)
Attachment #8857188 - Flags: review?(dholbert) → review+
Comment on attachment 8857189 [details] [diff] [review]
Reftests for caret drawing in text that is reversed relative to the element's inline direction

Review of attachment 8857189 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/base/tests/bug1354478-1-ref.html
@@ +19,5 @@
> +    <script>
> +      function start() {
> +        var textarea = document.querySelector("textarea");
> +        textarea.selectionStart = 1;
> +        textarea.selectionEnd = 1;

As in part 1, a comment in each of the files here might be helpful, to explain the selectionStart/selectionEnd tweak (e.g. "Place caret between the characters", if that's accurate)

r=me either way though.
Attachment #8857189 - Flags: review?(dholbert) → review+
Pushed by jkew@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba2ed132a710
Adjust caret rect to paint on the opposite side of the nominal caret position in "upstream" text, for more visually consistent placement. r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a67191bf3fb
Reftests for caret drawing in text that is reversed relative to the element's inline direction. r=dholbert
https://hg.mozilla.org/mozilla-central/rev/ba2ed132a710
https://hg.mozilla.org/mozilla-central/rev/2a67191bf3fb
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Flags: needinfo?(jfkthame)
You need to log in before you can comment on or make changes to this bug.