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

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: tedders1, Assigned: tedders1)

Tracking

(Depends on 1 bug)

unspecified
2.2 S14 (12june)
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Assignee

Description

4 years ago
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.
Assignee

Updated

4 years ago
Summary: The directional cursor should reflect the direction of the text it's in → The directional cursor should point in the opposite direction of what backspace will do
Assignee

Comment 1

4 years ago
Caret. I mean "caret", not cursor.
Assignee

Updated

4 years ago
Summary: The directional cursor should point in the opposite direction of what backspace will do → The directional caret should point in the opposite direction of what backspace will do
Assignee

Comment 2

4 years ago
> 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.
Assignee

Updated

4 years ago
Assignee: nobody → tclancy
Assignee

Comment 3

4 years ago
Posted patch bug-1164693-fix.patch (obsolete) — Splinter Review
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.
Attachment #8614511 - Flags: review?(smontagu)
Assignee

Comment 4

4 years ago
Oops. I mean, it depends on Bug 1067788, not Bug 1167788.
(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.
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.
Assignee

Comment 7

4 years ago
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.
Assignee

Comment 8

4 years ago
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.
Attachment #8614511 - Attachment is obsolete: true
Attachment #8614511 - Flags: review?(smontagu)
Attachment #8616322 - Flags: review?(smontagu)
Assignee

Comment 9

4 years ago
This patch fixes a problem where non-bidi paragraphs ending with a newline (which is most of them) were accidentally considered bidi.
Attachment #8616323 - Flags: review?(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
Attachment #8616322 - Flags: review?(smontagu) → review+
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)
Attachment #8616323 - Flags: review?(smontagu) → review+
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.
Target Milestone: --- → 2.2 S14 (12june)
Assignee

Comment 13

4 years ago
Some assertions were failing during tests, and it's because nsTextFrame::GetInFlowContentLength() makes some assumptions that aren't true for preformatted blocks of text.
Attachment #8625066 - Flags: review?(smontagu)
Attachment #8625066 - Flags: review?(smontagu) → review+

Updated

4 years ago
Depends on: 1177505

Updated

4 years ago
Depends on: 1180417
Depends on: 1207761
Depends on: 1221904
You need to log in before you can comment on or make changes to this bug.