Regression: RTL caret-indicator in wrong direction

NEW
Assigned to

Status

()

P1
major
3 years ago
2 years ago

People

(Reporter: mvocom, Assigned: tedders1)

Tracking

(Blocks: 1 bug, {regression, rtl})

41 Branch
regression, rtl
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41- wontfix, firefox42+ wontfix, firefox43+ wontfix, firefox44+ wontfix, firefox45+ fixed)

Details

Attachments

(5 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
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.
(Reporter)

Updated

3 years ago
Severity: normal → major
Keywords: rtl
Priority: -- → P5
(Reporter)

Updated

3 years ago
Priority: P5 → P1
(Reporter)

Comment 1

3 years ago
Hi Ehsan,

Could you please have a look?

Comment 2

3 years ago
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.

Updated

3 years ago
Blocks: 265070
Has Regression Range: --- → no
Component: Untriaged → Layout: Text
Keywords: regression
Product: Firefox → Core
Flags: needinfo?(tclancy)
(Reporter)

Comment 3

3 years ago
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.
(Assignee)

Comment 4

3 years ago
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?
Flags: needinfo?(tclancy) → needinfo?(smontagu)

Comment 5

3 years ago
noise
(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. :-)
(Reporter)

Comment 6

3 years ago
(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.
(Reporter)

Comment 7

3 years ago
(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.
(Assignee)

Comment 8

3 years ago
> 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.
(Assignee)

Comment 9

3 years ago
And of course, having a pref would allow users to use the old behaviour if they prefer.
(Assignee)

Updated

3 years ago
Assignee: nobody → tclancy
(Reporter)

Comment 10

3 years ago
(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.
Tracking for 43+. 

Ted, what was the bug where we changed this behavior in 41?
status-firefox41: --- → affected
status-firefox42: --- → affected
status-firefox43: --- → affected
status-firefox44: --- → affected
tracking-firefox41: --- → ?
tracking-firefox42: --- → ?
tracking-firefox43: --- → +
tracking-firefox44: --- → +
Flags: needinfo?(tclancy)
(Assignee)

Comment 12

3 years ago
(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.
Flags: needinfo?(tclancy)
Important regression for rtl users, tracking.
status-firefox41: affected → wontfix
tracking-firefox41: ? → -
tracking-firefox42: ? → +
(Reporter)

Comment 14

3 years ago
noise
Thank you Sylvestre.

Updated

3 years ago
Keywords: dev-doc-needed, site-compat
Hmm, now I don't think this requires a site compatibility doc.
Keywords: dev-doc-needed, site-compat
Do you still need info from me here? Implementing the new behaviour behind a pref sounds like a plan.
Flags: needinfo?(smontagu)
(Reporter)

Comment 17

3 years ago
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.
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
Flags: needinfo?(tclancy)
Flags: needinfo?(anygregor)
(Reporter)

Comment 19

3 years ago
noise
Thanks Sylvestre. I appreciate it.
Simon, as you reviewed the patches in bug 1164693, are you ok with the backout? Thanks
Flags: needinfo?(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)
Flags: needinfo?(smontagu)
Depends on: 1216096
(Reporter)

Comment 22

3 years ago
noise
That's great.
Thank you Sylvestre and Simon. I appreciate your work.
(Assignee)

Comment 23

3 years ago
I think I can get this done by the end of the week, if you're willing to wait.
Flags: needinfo?(tclancy)
Thanks Ted, this would be appreciated. However, we are so far in the cycle that I think we will ship 42 with this bug.
Ted, have you been able to work on this?
Too late for 42 but we will take a patch for 43.
status-firefox42: affected → wontfix
Flags: needinfo?(tclancy)
(Reporter)

Comment 26

3 years ago
Ted,

Are you getting along with this patch?
Please update.

Thanks.
(Assignee)

Comment 27

3 years ago
The good news is that it's my highest priority right now.
Flags: needinfo?(tclancy)
(Reporter)

Comment 28

3 years ago
noise
Thank you and best of luck. :)
I appreciate your work.
(Assignee)

Comment 29

3 years ago
Created attachment 8681104 [details] [diff] [review]
bug-1207761-part-1.patch
Attachment #8681104 - Flags: review?(smontagu)
(Assignee)

Comment 30

3 years ago
Created attachment 8681105 [details] [diff] [review]
bug-1207761-part-2.patch
Attachment #8681105 - Flags: review?(smontagu)
(Assignee)

Comment 31

3 years ago
Created attachment 8681196 [details] [diff] [review]
bug-1207761-part-2.patch
Attachment #8681105 - Attachment is obsolete: true
Attachment #8681105 - Flags: review?(smontagu)
Attachment #8681196 - Flags: review?(smontagu)
(Assignee)

Comment 32

3 years ago
Created attachment 8681229 [details] [diff] [review]
bug-1207761-part-2.patch
Attachment #8681196 - Attachment is obsolete: true
Attachment #8681196 - Flags: review?(smontagu)
Attachment #8681229 - Flags: review?(smontagu)
(Assignee)

Comment 33

3 years ago
Created attachment 8681264 [details] [diff] [review]
bug-1207761-part-3.patch
Attachment #8681264 - Flags: review?(smontagu)
(Assignee)

Comment 34

3 years ago
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.
(Reporter)

Comment 35

3 years ago
noise
Great! Many thanks Ted.
(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.
Duplicate of this bug: 1216187
Comment hidden (metoo)
(Reporter)

Comment 39

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

Thanks.
Comment hidden (metoo)
Ted, do you have news on this? Thanks
status-firefox45: --- → affected
Flags: needinfo?(ted.clancy)
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.
(Reporter)

Comment 43

3 years ago
noise
Thank you Simon. I appreciate your decision.

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

Comment 44

3 years ago
No worries, Simon. I know how things go.
Flags: needinfo?(ted.clancy)
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
status-firefox45: affected → fixed
The backout is risky looking and breaks stuff in aurora. So looks like 43 (and maybe 44) will ship with this bug.
status-firefox43: affected → wontfix
tracking-firefox45: --- → +
status-firefox44: affected → wontfix
Created attachment 8770729 [details]
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
Created attachment 8770730 [details]
Color coded area of concern Hebrew RTL

Updated screenshot, 
Green is correct placement
Red shows incorrect placement

Updated

2 years ago
Status: UNCONFIRMED → NEW
Ever confirmed: true

Comment 49

2 years ago
Hi,
I was unable to reproduce this on Windows 10, FF 47.0.1.
Flags: needinfo?(anygregor)
You need to log in before you can comment on or make changes to this bug.