1207761 - Regression: RTL caret-indicator in wrong direction

Status: RESOLVED WORKSFORME

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

Description

[Yaron]

User Agent: Mozilla/5.0 (Windows NT 6.1; rv:41.0) Gecko/20100101 Firefox/41.0
Build ID: 20150917150946

Steps to reproduce:

FF RTL (Hebrew).

Set the input language in Windows Task Bar to English.
Start Firefox.


Actual results:

The caret-indicator (hook) in text fields (Search Bar, Find Bar etc. ;- page content as well) turns to the *left* and indicates RTL direction (wrong, the lang is En).
http://s30.postimg.org/5j63zamsd/Hook.png

If the caret is on the left side and the hook turns to the *right* (direction = ltr), and then you press Ctrl+Shift+X, the caret moves to the right but the hook changes direction to left (wrong).

Type anything and the hook turns to the right (as it should be).

***
Another problem:
Type anything in a text field.
Change the input language in Windows Task Bar to Hebrew.

- The hook changes direction to left (correct).
Press left or right key - the hook changes direction to right (wrong). 

***
Both issues are new in FF 41.

Comment 1

[Yaron]

Hi Ehsan,

Could you please have a look?

Comment 2

[Tomer Cohen :tomer]

It sounds to me as it is related to dir=auto, and not directly to the language chooser, but as far as I remember, the switch direction item actually disables dir=auto.

Comment 3

[Yaron]

Hi Gijs,

This is the worst bug I've encountered.
Any idea?

Can anyone refer me to the possible relevant files (if they're included in omni.ja)?

Thanks.

Comment 4

[Ted Clancy [:tedders1]]

So, paragraphs have a direction that is independent of the text in the paragraph. An LTR paragraph is still an LTR paragraph even if it only contains RTL text. An LTR paragraph is still an LTR paragraph even if it's empty.

Normally the caret takes its direction from the text it's adjacent to. (If it's between an LTR character and an RTL character, it can be either). However, there are cases where the caret takes the paragraph's direction.

1) In an empty paragraph, the caret takes the paragraph's direction.

2) At the logical start of a paragraph, the caret can have either the paragraph's direction or the direction of its first character.

3) At the logical end of a paragraph, the caret can have either the paragraph's direction or the direction of its last character.

I'm realizing now that this is confusing for users.

What's happening in this bug is that the caret is in an empty paragraph, and it's indicating the direction of the paragraph, which isn't what the user's expect it to be.

I'd like to make to following changes:

1) In an empty paragraph, the caret shouldn't indicate *either* direction. It should just look like a non-directional caret.

2) At the start of a paragraph, the caret should only ever have the directionality of the first character.

3) At the end of a paragraph, the caret should only ever have the directionality of the last character.

Simon, does that sound good with you? Can I go ahead with this?

Comment 5

[:Gijs (he/him)]

(In reply to Yaron from comment #3)
> Hi Gijs,
> 
> This is the worst bug I've encountered.
> Any idea?

Ted seems to be on top of this. :-)

Comment 6

[Yaron]

(In reply to :Gijs Kruitbosch from comment #5)
> (In reply to Yaron from comment #3)
> > Hi Gijs,
> > 
> > This is the worst bug I've encountered.
> > Any idea?
> 
> Ted seems to be on top of this. :-)

Thanks Gijs.
I'm glad Ted is working on it.

Comment 7

[Yaron]

(In reply to Ted Clancy [:tedders1] from comment #4)

> I'd like to make to following changes:
> 
> 1) In an empty paragraph, the caret shouldn't indicate *either* direction.
> It should just look like a non-directional caret.
>
> 2) At the start of a paragraph, the caret should only ever have the
> directionality of the first character.
> 
> 3) At the end of a paragraph, the caret should only ever have the
> directionality of the last character.

All applications supporting RTL always display the indicator.
Think about the hook as a badge displaying either "En" or "He".

The rule is clear and simple: if the current input language is LTR (e.g. English), the hook turns to the right; if the current input language is RTL (e.g. Hebrew), the hook turns to the left.

Many users (when quickly wanting to know the current lang) don't look at the Windows Language Bar, but rather at the current direction of the caret.  

IMO, we should revert to the old behavior; - the conventional behavior in ALL other applications.
Any other behavior would be highly confusing.

I hope Simon agrees with me.

Thank you Ted for working on it.

Comment 8

[Ted Clancy [:tedders1]]

> IMO, we should revert to the old behavior; - the conventional behavior in ALL other applications.
> Any other behavior would be highly confusing.

Well, there are advantages to the new behaviour too:

* When you click near a bidi boundary (say, between Hebrew letters and numerals), you can tell which side of the boundary you clicked on.

* You can predict which way the backspace key will work (at a bidi boundary) by looking at the caret.

* As Firefox expands into non-desktop platforms (TVs, phones, IoT) I can imagine newer platforms where "current keyboard input language" is a vague concept (for example, handwriting input, or speech recognition, or simply being on a platform where we can't query the keyboard layout). So indicating the current input language would be impossible.

Also, I was under the impression that the bidi caret was invented to reduce the confusion that comes from editing bidi text, and that's what the new behaviour aims to do. 

The confusion of editing bidi text doesn't come from forgetting the current input language, so there's no point in using the caret to indicate the current input language. The confusion comes from surprising caret movements at bidi boundaries, so the new behaviour attempts to make those movements more predictable.

If it really were the caret's job to indicate the current input language, we'd have different carets for Greek and English, for those people who need to switch between Greek and English. But we don't.

And on phones (which are becoming more and more important for Firefox), the on-screen keyboard is a much better indicator of the current input language than the caret could ever be. 

However, the corner cases I mentioned above (empty paragraph, or start/end of paragraph) do need to be addressed. (And I will.)

And after seeing the response here, I'm starting to realize that maybe I should have been put the new behaviour behind a pref. That would have allowed us to play with the new behaviour and iron out the corner cases before making it the default. It would also allow us to have different defaults on different platforms.

I'll implement a pref.

Comment 9

[Ted Clancy [:tedders1]]

And of course, having a pref would allow users to use the old behaviour if they prefer.

Comment 10

[Yaron]

(In reply to Ted Clancy [:tedders1] from comment #8)

Hi Ted,

First, I'd like to thank you for working on this issue.
I really appreciate it.

> Well, there are advantages to the new behaviour too:
> 
> * When you click near a bidi boundary (say, between Hebrew letters and
> numerals), you can tell which side of the boundary you clicked on.
> 
> * You can predict which way the backspace key will work (at a bidi boundary)
> by looking at the caret.

I see your point.
However, combining LTR and RTL (and even numerals) has never been smooth and perfect.
In some more complex cases it can turn into a nightmare. 
So, personally I'd rather deal with "good old" quirks and whims than new ones. :)

> * As Firefox expands into non-desktop platforms (TVs, phones, IoT) I can
> imagine newer platforms where "current keyboard input language" is a vague
> concept (for example, handwriting input, or speech recognition, or simply
> being on a platform where we can't query the keyboard layout). So indicating
> the current input language would be impossible.

That's a solid argument.

> Also, I was under the impression that the bidi caret was invented to reduce
> the confusion that comes from editing bidi text, and that's what the new
> behaviour aims to do. 

I don't know why the bidi caret was originally introduced.
But being around for years, many users do rely on the indicator to determine which is the current language.  
Or in more accurate words: even if users do not *rely* on the indicator, they most certainly associate the hook's direction with a specific language. 

> If it really were the caret's job to indicate the current input language,
> we'd have different carets for Greek and English, for those people who need
> to switch between Greek and English. But we don't.

Another good point.
But RTL users (unlike Greek users) have become accustomed to relying on the indicator.

> However, the corner cases I mentioned above (empty paragraph, or start/end
> of paragraph) do need to be addressed. (And I will.)
> 
> And after seeing the response here, I'm starting to realize that maybe I
> should have been put the new behaviour behind a pref. That would have
> allowed us to play with the new behaviour and iron out the corner cases
> before making it the default. It would also allow us to have different
> defaults on different platforms.
> 
> I'll implement a pref.

That's a great solution.

***
May I refer you to some related issues - Bug 265070 (no idea how closely related)?

Thanks again.

Comment 11

[Liz Henry (:lizzard) (relman/hg->git project)]

Tracking for 43+. 

Ted, what was the bug where we changed this behavior in 41?

Comment 12

[Ted Clancy [:tedders1]]

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #11)
> Ted, what was the bug where we changed this behavior in 41?

Bug 1164693.

Comment 13

[Sylvestre Ledru [:Sylvestre]]

Important regression for rtl users, tracking.

Comment 14

[Yaron]

Thank you Sylvestre.

Comment 15

[Kohei]

Hmm, now I don't think this requires a site compatibility doc.

Comment 16

[Simon Montagu :smontagu]

Do you still need info from me here? Implementing the new behaviour behind a pref sounds like a plan.

Comment 17

[Yaron]

Ted,
Do you have time to work on this?

Simon,
If Ted can't complete the work soon, could you please re-merge the old code (
Bug 1164693)?

Would it be possible to do that for 42? It's *highly important*. 

Thanks.

Comment 18

[Sylvestre Ledru [:Sylvestre]]

Ted, Gregor, we are considering the backout of bug 1164693 to fix this bug as it caused some important regressions. Is that OK with you? Thanks

Comment 19

[Yaron]

Thanks Sylvestre. I appreciate it.

Comment 20

[Sylvestre Ledru [:Sylvestre]]

Simon, as you reviewed the patches in bug 1164693, are you ok with the backout? Thanks

Comment 21

[Simon Montagu :smontagu]

Yes, I'm on board with that as the next step (and as I said before, we should then go ahead and work out new behaviour behind a pref)

Comment 22

[Yaron]

That's great.
Thank you Sylvestre and Simon. I appreciate your work.

Comment 23

[Ted Clancy [:tedders1]]

I think I can get this done by the end of the week, if you're willing to wait.

Comment 24

[Sylvestre Ledru [:Sylvestre]]

Thanks Ted, this would be appreciated. However, we are so far in the cycle that I think we will ship 42 with this bug.

Comment 25

[Sylvestre Ledru [:Sylvestre]]

Ted, have you been able to work on this?
Too late for 42 but we will take a patch for 43.

Comment 26

[Yaron]

Ted,

Are you getting along with this patch?
Please update.

Thanks.

Comment 27

[Ted Clancy [:tedders1]]

The good news is that it's my highest priority right now.

Comment 28

[Yaron]

Thank you and best of luck. :)
I appreciate your work.

Comment 29

[Ted Clancy [:tedders1]]

Attachment: bug-1207761-part-1.patch

Comment 30

[Ted Clancy [:tedders1]]

Attachment: bug-1207761-part-2.patch

Comment 31

[Ted Clancy [:tedders1]]

Attachment: bug-1207761-part-2.patch

Comment 32

[Ted Clancy [:tedders1]]

Attachment: bug-1207761-part-2.patch

Comment 33

[Ted Clancy [:tedders1]]

Attachment: bug-1207761-part-3.patch

Comment 34

[Ted Clancy [:tedders1]]

I apologize for flip-flopping on the design of the bidi caret handling. I didn't realize that users expect the on-screen caret direction to reflect the current input language.

Changes in these patches:

1) As requested, the on-screen caret direction reflects the current input language. This means I'm reverting some of my earlier code. (I'm not going to bother with a pref after all.) Since the directional caret doesn't convey any information that isn't already known to the user, I don't show the directional caret unless bidi.ui.browser is set (which was the old behaviour).

2) The code no longer changes CaretBidiLevel when arrowing over a character or deleting a character. Instead, the CaretBidiLevel of the caret now always matches the direction of the on-screen caret (as set by the input language), which allows for predictable caret behaviour. The CaretBidiLevel is now only changed when the input language changes.

3) When positioning the cursor visually (that is, using the cursor keys or clicking the mouse) at a bidi boundary, the logical position of the caret will now depend on the input language. So if you move the caret to be between some English text and Hebrew text, the caret will be logically adjacent to the English text if your keyboard is set to English, or logically adjacent to the Hebrew text if your keyboard is set to Hebrew. This is more intuitive and reduces how much the caret jumps around.

Overall, I think this new behaviour is an improvement. (Backspacing sometimes happens "at a distance", but predictably so.) Please try it out.

Comment 35

[Yaron]

Great! Many thanks Ted.

Comment 36

[Jonathan Kew [:jfkthame]]

(In reply to Ted Clancy [:tedders1] from comment #34)
> 3) When positioning the cursor visually (that is, using the cursor keys or
> clicking the mouse) at a bidi boundary, the logical position of the caret
> will now depend on the input language. So if you move the caret to be
> between some English text and Hebrew text, the caret will be logically
> adjacent to the English text if your keyboard is set to English, or
> logically adjacent to the Hebrew text if your keyboard is set to Hebrew.
> This is more intuitive and reduces how much the caret jumps around.

I haven't tried to build & test with these patches yet, but I'm curious what this means for situations where there is no well-defined concept of "input language" or keyboard direction. For an extreme example, consider the Unicode Hex Input keyboard layout on OS X, which can enter any desired 16-bit code unit.... there's no way to know whether the user intends to type LTR or RTL characters with it.

More generally, any Unicode-based keyboard may provide a mixture of LTR, RTL and neutral characters. The idea of a "current keyboard layout direction" only really made sense in a world where keyboard layouts were associated with legacy codepages, which in turn targeted particular languages with a more uniform repertoire of characters.

Comment 38

[katzelad]

Having this bug too (hebrew), definitely started in FF 41.

Comment 39

[Yaron]

Would it be possible to include Ted's patch in 43?
This issue causes numerous annoying mistakes on a daily basis. 

Thanks.

Comment 40

[avi golan]

as a developer I will not abandon firefox so easy, but this bug is a nightmare and I am using explorer as my temporary browser.

Comment 41

[Sylvestre Ledru [:Sylvestre]]

Ted, do you have news on this? Thanks

Comment 42

[Simon Montagu :smontagu]

I went ahead with the backout in bug 1216096, because neither jfkthame nor I can promise a speedy review for this. Apologies to ted for making life more complicated.

Comment 43

[Yaron]

Thank you Simon. I appreciate your decision.

Ted,
Thanks again for your last patch.
Please keep contributing to the issue.

Comment 44

[Ted Clancy [:tedders1]]

No worries, Simon. I know how things go.

Comment 45

[Liz Henry (:lizzard) (relman/hg->git project)]

This should be marked fixed for 45 since the backout landed in m-c a week ago. 
I suggested uplifting the backout in bug 1216096

Comment 46

[Liz Henry (:lizzard) (relman/hg->git project)]

The backout is risky looking and breaks stuff in aurora. So looks like 43 (and maybe 44) will ship with this bug.

Comment 47

[Michelle Funches - QA]

Attachment: HebrewRTL.png

Still appears to be an issue 
Reference screenshot: 
When cursor is placed in URL bar it is to the Right also true for the Search Bar 
When cursor is placed in the main screen Search area it is to the Left
47.0.1
20160623154057
Mozilla/5.0 (Windows NT 10.0; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0

Comment 48

[Michelle Funches - QA]

Attachment: Color coded area of concern Hebrew RTL

Updated screenshot, 
Green is correct placement
Red shows incorrect placement

Comment 49

[katzelad]

Hi,
I was unable to reproduce this on Windows 10, FF 47.0.1.

Comment 50

[Sylvestre Ledru [:Sylvestre]]

Moving to p3 because no activity for at least 24 weeks.

Comment 51

[Peter Magyari (Desktop QA)]

Since this issue is very old and isn't reproducible anymore, I'm going to close it.