1164693 - The directional caret should point in the opposite direction of what backspace will do

Status: RESOLVED FIXED

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

Description

[Ted Clancy [:tedders1]]

When the pref |bidi.browser.ui| is true, the caret is replaced with a "directional" caret.

Right now, this caret seems to reflect the directionality of the current paragraph, which isn't terribly useful.

What would be better is if the caret reflected the directionality of the current text it's in. And if the caret is at a bidi boundary, the caret should always point in the OPPOSITE direction of the movement that would happen when backspace is pressed. That is, backspace should always move the cursor backwards relative to the direction it's pointing in.

This way, the user never needs to be confused about what hitting "backspace" will do.

Comment 1

[Ted Clancy [:tedders1]]

Caret. I mean "caret", not cursor.

Comment 2

[Ted Clancy [:tedders1]]

> Right now, this caret seems to reflect the directionality of the current paragraph, which isn't terribly useful.

My apologies. It seems that the caret currently reflects the current input language.

Comment 3

[Ted Clancy [:tedders1]]

Attachment: bug-1164693-fix.patch

This patch:

1) Shows the bidi caret whenever: (a) the document is bidi; (b) the user is using an RTL keyboard; or (c) The bidi.browser.ui pref is set to true.

2) Makes the hook on the caret point reflect the caret bidi level. (I shall call this the "caret bidi direction".)

3) Did you know that when you have a bidi boundary (e.g. ABCאבג) and you click on the boundary (e.g. right between the C and the gimmel), the caret bidi direction depends on whether you clicked slightly to the right of the boundary or slightly to the left of the boundary? I didn't. And since you couldn't see the caret direction, it was really confusing. But now that you can see the caret direction, it's pretty nifty.


Make sure you update to a recent version of mozilla-central before testing this. It relies on Bug 1167788, which just recently landed on mozilla-central.

Comment 4

[Ted Clancy [:tedders1]]

Oops. I mean, it depends on Bug 1067788, not Bug 1167788.

Comment 5

[Simon Montagu :smontagu]

(In reply to Ted Clancy [:tedders1] from comment #3)
> This patch:
> 
> 1) Shows the bidi caret whenever: (a) the document is bidi;

We used to do that but people complained -- see bug 418513. The problem is that a lot of documents happen to be bidi because they have an RTL character tucked away somewhere -- a good example is any Wikipedia page that has an RTL language in the "this page in other languages" list.

Comment 6

[Jonathan Kew [:jfkthame]]

From a UX point of view, maybe the ideal thing to do would be to show the bidi caret automatically at bidi boundaries, but not otherwise. But I'm not sure offhand how tricky this might be to implement.

Comment 7

[Ted Clancy [:tedders1]]

How about we show the bidi caret automatically in bidi paragraphs?

(Mind you, I don't know how I'd detect that the caret is in a bidi paragraph. But I'll figure something out.)

> maybe the ideal thing to do would be to show the bidi caret automatically at bidi boundaries, but not
> otherwise. But I'm not sure offhand how tricky this might be to implement.

That's not hard.

Comment 8

[Ted Clancy [:tedders1]]

Attachment: bug-1164693-fix-part-1.patch

I modified the patch so that it only shows the bidi caret in paragraphs that are bidi. It does this by checking for NS_FRAME_IS_BIDI, which is set on frames that are in bidi paragraphs.

Otherwise, it shows the bidi caret if the keyboard language is RTL, or if bidi.browser.ui is set, as before.

Comment 9

[Ted Clancy [:tedders1]]

Attachment: bug-1164693-fix-part-2.patch

This patch fixes a problem where non-bidi paragraphs ending with a newline (which is most of them) were accidentally considered bidi.

Comment 10

[Simon Montagu :smontagu]

Comment on attachment 8616322 [details] [diff] [review]
bug-1164693-fix-part-1.patch

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

::: layout/base/nsCaret.cpp
@@ +934,5 @@
> +    int caretBidiLevel = selection->GetFrameSelection()->GetCaretBidiLevel();
> +    if (caretBidiLevel & BIDI_LEVEL_UNDEFINED) {
> +      caretBidiLevel = NS_GET_EMBEDDING_LEVEL(aFrame);
> +    }
> +    bool isCaretRTL = caretBidiLevel % 2;

Please make caretBidiLevel an nsBidiLevel, and use IS_LEVEL_RTL(caretBidiLevel) here

Comment 11

[Simon Montagu :smontagu]

Comment on attachment 8616323 [details] [diff] [review]
bug-1164693-fix-part-2.patch

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

::: layout/base/nsBidiPresUtils.cpp
@@ +733,5 @@
>  #endif
>  #endif
>  #endif
>  
> +  bool isNonBidi = false;

Nit: Double negatives are confusing. I think it's clearer to call this isBidi (and reverse the true/false values, of course)

Comment 12

[Simon Montagu :smontagu]

A thought for future enhancement: maybe it would be useful to add a new frame property to cache the result of GetDirection: NSBIDI_LTR, NSBIDI_RTL, or NSBIDI_MIXED? I think using that would be more accurate than using NS_FRAME_IS_BIDI -- I think I've been inclined to set NS_FRAME_IS_BIDI on a "can't do any harm" basis in the past.

Comment 13

[Ted Clancy [:tedders1]]

Attachment: bug-1164693-fix-part-3.patch

Some assertions were failing during tests, and it's because nsTextFrame::GetInFlowContentLength() makes some assumptions that aren't true for preformatted blocks of text.

Comment 14

[Ted Clancy [:tedders1]]

Treeherder run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2a4bd9caceb3

Comment 15

[Ted Clancy [:tedders1]]

Newer treeherder run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6ee4649cf002

Comment 16

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/b355b20f6123
https://hg.mozilla.org/integration/mozilla-inbound/rev/2a383ebb99d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/b232c2a5af6d

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/b355b20f6123
https://hg.mozilla.org/mozilla-central/rev/2a383ebb99d5
https://hg.mozilla.org/mozilla-central/rev/b232c2a5af6d