Open Bug 1138419 Opened 9 years ago Updated 1 year ago

Shift-Ctrl-Arrow moves in the wrong direction in an RTL textarea

Categories

(Core :: DOM: Selection, defect)

Product:
Core
Component:
DOM: Selection
Version:
36 Branch
Platform:
x86_64
All
Type:
defect

Tracking

()

People

(Reporter: amir.aharoni, Unassigned)

References

Details

(Keywords: regression, rtl)

Attachments

(1 file)

Draft patch, needs more testing
1.59 KB, patch
smontagu
: feedback?
jfkthame
Details | Diff | Splinter Review 
To reproduce:

0. Firefox 36, Fedora 21, installed from the usual Fedora package.
1. Go to the https://he.wikipedia.org/wiki/ASCII?action=edit
2. Click in the middle of the big textarea (#wpTextbox1).
3. Press Shift-Ctrl-right arrow.

Observed: The word to the *left* is selected.
Expected: The word to the *right* must be selected.

It is broken for both left and right direction.

Ctrl-Arrow works correctly for moving the cursor word by word.

This is a regression. Shift-Ctrl-Arrow used to work correctly until Firefox 35.
Regression range in nightlies: http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=be4ba3d5ca9a&tochange=74edfbf0e6a3

I don't see anything likely in that range, we need to narrow it down to the cset.
Keywords: regressionwindow-wanted
4c90e4b108ec
2014-11-22 14:39 +0000 	Jonathan Kew - Bug 1077515 - part 11 - Remap arrow keys for vertical writing mode in the Gtk widget code. r=roc

looks like a likely candidate (I have no clue why it doesn't appear in the pushloghtml list)
Blocks: 1077515
Flags: needinfo?(jfkthame)
I don't think part 11 there can be the guilty cset, as it should only modify behavior at all when the writing-mode is vertical. However, it does seem likely one of the earlier patches in that bug may have inadvertently caused this. I'll try to narrow it down...
Flags: needinfo?(jfkthame)
Ah, this isn't Linux-specific; I see similar behavior with Shift-Option-arrow on OS X selecting in the opposite direction to the expected word move. Which appears to point at one of the non-platform-specific changes in bug 1077515 being the most likely culprit.
OS: Linux → All
Looks like the regression comes from
  https://hg.mozilla.org/mozilla-central/rev/66f1f54e19bc
  Bug 1077515 - part 1 - Eliminate use of keyCode parameters and values in nsFrameSelection. r=roc
Keywords: regressionwindow-wanted
Hmm. It looks to me like the previous behavior may have actually been incorrect, and now things are behaving as designed (if not as desired!).

Note the pref setting bidi.edit.caret_movement_style, which AFAICS was introduced in bug 330175. In all.js, we find that the default is 2:

  // Bidi caret movement style:
  // 0 = logical
  // 1 = visual
  // 2 = visual, but logical during selection
  pref("bidi.edit.caret_movement_style", 2);

Therefore, it's expected that when selecting (i.e. with the Shift key down), movement will be logical (i.e. right-arrow means forward, left-arrow means backward) rather than visual.

Prior to bug 1077515, the code in nsFrameSelection::MoveCaret would set visualMovement to false in this case at http://hg.mozilla.org/mozilla-central/annotate/48966e85eec4/layout/generic/nsSelection.cpp#l750 (because aContinueSelection is true), but then in the switch() at http://hg.mozilla.org/mozilla-central/annotate/48966e85eec4/layout/generic/nsSelection.cpp#l876, we ignore the visualMovement flag and treat LEFT and RIGHT visually regardless. That seems like a bug?

If visual movement is the desired behavior even when selecting, the "fix" is to change bidi.edit.caret_movement_style to 1. Maybe that should be the default? Do user expectations vary across platforms?
I don't know about Mac users, to be honest, but it makes perfect sense to me that Ctrl-Arrow and Shift-Ctrl-Arrow must move in the same direction and the different is supposed to be that when Shift is pressed, selection (highlighting) is supposed to occur.

I know that there's a difference in Firefox between movement and movement with selection: the first is visual and the second is logical. This may have to be accounted for here somehow, but in the cited example even the simple act of selection Hebrew-only text definitely goes backwards, even before we bring embedded English strings into the picture.
(In reply to Jonathan Kew (:jfkthame) from comment #6)
 
> Therefore, it's expected that when selecting (i.e. with the Shift key down),
> movement will be logical (i.e. right-arrow means forward, left-arrow means
> backward) rather than visual.

I don't think this analysis is correct. We're talking about right-to-left text in a right-to-left element, so right-arrow should consistently mean backwards and left-arrow forwards. bidi.edit.caret_movement_style only kicks in when arrowing over reverse-direction text.
(In reply to Simon Montagu :smontagu from comment #8)
> (In reply to Jonathan Kew (:jfkthame) from comment #6)
>  
> > Therefore, it's expected that when selecting (i.e. with the Shift key down),
> > movement will be logical (i.e. right-arrow means forward, left-arrow means
> > backward) rather than visual.
> 
> I don't think this analysis is correct. We're talking about right-to-left
> text in a right-to-left element, so right-arrow should consistently mean
> backwards and left-arrow forwards. bidi.edit.caret_movement_style only kicks
> in when arrowing over reverse-direction text.

Oh, so caret_movement_style doesn't mean what I assumed it meant. OK, time for another look over how the code used to work, and how it works (or fails) now....
In spite of what I said in comment 8, setting bidi.edit.caret_movement_style to 1 does work around the bug. So comment 6 must be mostly correct, except for the presupposition that visual movement should be different from logical movement in RTL text inside a RTL element.
So I think the regression was caused by removing the aKeycode argument to MoveCaret and not maintaining an important distinction between different types of key: "arrow keys" like forward and back are associated with the physical direction of the arrow, so they always need to reverse the direction of movement in an RTL context (except for the case of reverse-direction text, which is handled in PeekOffset). Delete and Backspace, on the other hand, never need to reverse direction.

This patch needs a lot more testing before review, with different combinations of writing mode and the bidi caret options.
Assignee: nobody → smontagu
Attachment #8634701 - Flags: feedback?(jfkthame)
An open question (possibly out of scope for this bug) is which category "Home" and "End" fall into: some keyboard/OS combinations have these as separate keys, but others use a chord with an arrow key, e.g. Cmd-Left/Cmd-Right. It would make sense for Cmd-Right to be equivalent to Home instead of End in a RTL context, but I don't think we've ever done that.
(In reply to Simon Montagu :smontagu from comment #11)

> This patch needs a lot more testing before review, with different
> combinations of writing mode and the bidi caret options.

Also, note bug 1103374: we need to add automated tests for behavior in vertical writing modes.
(In reply to Amir Aharoni from comment #7)
> I don't know about Mac users, to be honest, but it makes perfect sense to me
> that Ctrl-Arrow and Shift-Ctrl-Arrow must move in the same direction and the
> different is supposed to be that when Shift is pressed, selection
> (highlighting) is supposed to occur.

Amir, I want to confirm that this bug also exists in Mac OS and this has been the case for the last two years at least (I stumbled upon this bug report just now, and was surprised to see that it was also filed about 2 years ago).

I agree with you that the movement for CTRL+ARROW should be the same as CTRL+SHIFT+ARROW; they should both match the direction of the arrow itself.
Component: General → Layout: Form Controls
Product: Firefox → Core

Bump. While this is fine in Windows, in MacOS it has been bothering me for a while. Also when doing SHIFT+ARROW, CMD+ARROW - wrong direction. Also not just in RTL text areas - also in LTR boxes but with RTL text content (e.g. hebrew, and also arabic probably). It's working fine in any other application.

MacOS 10.15.7, Firefox 86.0

See Also: → 1734560

The bug assignee didn't login in Bugzilla in the last 7 months.
:emilio, could you have a look please?
For more information, please visit auto_nag documentation.

Assignee: smontagu → nobody
Flags: needinfo?(emilio)

Jonathan, it seems you might have stronger opinions here. The patch in comment 11 probably still applies to some extent...

Anyways this is presumably not specific to textarea and is more about selection movement in general.

Component: Layout: Form Controls → DOM: Selection
Flags: needinfo?(emilio)

Err, meant to ni? for the comment above, sorry for the noise.

Flags: needinfo?(jfkthame)

This behavior can be adjusted via the bidi.edit.caret_movement_style pref; in bug 1646240, we changed the default behavior for Nightly such that AFAICT it behaves "as expected" in Nightly on macOS for me.

:saschanaz, do you think we should roll that out more widely, or are there concerns with it?

Flags: needinfo?(jfkthame) → needinfo?(krosylight)

I'm all for enabling it everywhere. There was some concern since I've seen some older bugs that preferred the logical way, but I've seen no objection from Nightly users so far.

Flags: needinfo?(krosylight)

How can I download and test the Nightly version for macOS?

Note that I think this and bug 1734560 are one and the same; in that bug, I provided extensive tables of what the behavior is and should be. My goal would be to test those using the Nightly version and report back.

Thanks, I just tested it and only certain cases are fixed. Specifically, the SHIFT+ARROW now works logically, i.e. moves the selection caret in the direction of the arrow, regardless of the directionality of text or the directionality of textarea.

However, issues with SHIFT+OPTION+ARROW and SHIFT+COMMAND+ARROW persist. See tables in my two latest comments on bug 1734560.

The current bug is about a subset of those cases, specifically about word selection (i.e. the SHIFT+OPTION+ARROW cases). Those cases are still unresolved in the nightly. So neither this nor bug 1734560 can be closed at this time.

Just to prevent confusion: Currently the code uses "Logical" when it depends on the direction of the text (left means the start, right means the end) and "Visual" when it does not (left means visually left, right means visually right).

Oh, then I meant "visual". And that is what the desirable outcome is.

FCOL, I don't think the piece of code associated with the bidi.edit.caret_movement_style config is the right place to fix some of the issues enumerated above, specifically the issue that is subject of this bug (i.e. the way caret moves for word selections). From what I can gather that config is really about single character movements.

The real issue is in Windows keyboards, there is one "Home" button, and it'll take your caret to the beginig of the line, both in RTL and LTR. But in Mac, there is no such button. Instead, there is a key combo that includes an arrow (in this case, COMMAND+ARROW). And it will be a different arrow you would need to press to go to the beginig of the line in LTR (COMMAND+LEFTARROW) versus in RTL (COMMAND+RIGHTARROW). I am guessing that Firefox's code, simple-mindedly, is assuming that COMMAND+LEFTARROW = HOME, and uses that in both LTR and RTL contexts, and that is not only undesirable (because the direction that caret would move is opposite the visual direction of the arrow button in RTL context), but also, unorthodox; other famous Mac programs (wither Apple programs, or Word, or Google Chrome), do respect the visual direction of the arrow, making Firefox an only exception which is annoying to the users.

So, if I were to make a guess as to how the (pseudo)code that solves the issue in this bug and bug 1734560 would look like, I would guess it would be something like this:

if os == windows
if button pressed = home
if context = LTR
move caret all the way left of line
else
move caret all the way right of line
elif os = mac
if button pressed = combo & combo includes COMMAND + (LEFTARROW or RIGHTARROW)
if second key in combo = LEFTARROW
move caret all the way left of line
else
move caret all the way right of line

Point being a mapping to the HOME button would become irrelevant in macOS, and instead the arrow key itself should be used in the logic.

And same goes for selection mode (where SHIFT is also pressed).

This is all speculation of course.

I am guessing that Firefox's code, simple-mindedly, is assuming that COMMAND+LEFTARROW = HOME, and uses that in both LTR and RTL contexts

This is true AFAIK, I wanted to fix it but I had no macOS to take an actual look. Masayuki may be able to?

Flags: needinfo?(masayuki)

Do you know where in code this is? I have no experience compiling Firefox, but maybe I could give it a try?

Good news is I was able to build Firefox from source on macOS. Bad news is changing the bit of code you pointed to did not make any change in the behavior of Firefox after a rebuild. I will keep digging.

Those commands refer logical direction according to their names.
https://searchfox.org/mozilla-central/rev/449d0a0f57d1e603721cf79645e19c5673c59dd0/widget/CommandList.h#27,39,58,62-64,68-71

Perhaps, there should be corresponding commands which refer physical direction. Then, NativeKeyBindings and ChildView should map Cocoa's physical direction commands to the new ones.

And it might need to touch selection code too if some new commands don't work with nsISelectionController::PhysicalMove.

Unfortunately, 3 big task in my queue, so I cannot work on this bug for now...

Flags: needinfo?(masayuki)

From what I can understand by reviewing the code further, two approaches could be used in theory.

Approach One: let Firefox (or really Cocoa) continue to think that, say, COMMAND+LEFTARROW means HOME (or specifically KEY_NAME_INDEX_Home). Then, in RTL context only, reverse what HOME actually does. This would require substantial modification in NativeKeyBindings.mm and whatever other code actually invokes its functions, such that directionality context can be passed in (currently, NativeKeyBindings.mm has no clue about directionality).

Approach Two: let Firefox/Cocoa have a different interpretation of COMMAND+LEFTARROW based on directionality context, that is in macOS in LTR it would mean KEY_NAME_INDEX_Home and in macOS in RTL it would mean KEY_NAME_INDEX_End. This lets NativeKeyBindings.mm untouched, and moves the definition to somewhere else. KeyNameList.h is where the constants like KEY_NAME_INDEX_Home are defined, but it already depends on an aCPPName called "Home", which I don't know where it is defined. I were to guess, it is macOS itself passing the "Home" key to the software whenever COMMAND+LEFTARROW is pressed (regardless of directionality). So while this option might be "cleaner" it may not be doable if the binding of COMMAND+LEFTARROW to the Home key is at the OS level.

(In reply to Huji from comment #33)

Approach One: let Firefox (or really Cocoa) continue to think that, say, COMMAND+LEFTARROW means HOME (or specifically KEY_NAME_INDEX_Home). Then, in RTL context only, reverse what HOME actually does. This would require substantial modification in NativeKeyBindings.mm and whatever other code actually invokes its functions, such that directionality context can be passed in (currently, NativeKeyBindings.mm has no clue about directionality).

Due to the process boundary, the main process which handles native keyboard events cannot check writing-mode and bidi state real-time. Therefore, considering direction in the main process is the last resort.

Approach Two: let Firefox/Cocoa have a different interpretation of COMMAND+LEFTARROW based on directionality context, that is in macOS in LTR it would mean KEY_NAME_INDEX_Home and in macOS in RTL it would mean KEY_NAME_INDEX_End. This lets NativeKeyBindings.mm untouched, and moves the definition to somewhere else. KeyNameList.h is where the constants like KEY_NAME_INDEX_Home are defined, but it already depends on an aCPPName called "Home", which I don't know where it is defined. I were to guess, it is macOS itself passing the "Home" key to the software whenever COMMAND+LEFTARROW is pressed (regardless of directionality). So while this option might be "cleaner" it may not be doable if the binding of COMMAND+LEFTARROW to the Home key is at the OS level.

In my understanding, Cocoa sends moveToLeftEndOfLine: or moveToRightEndOfLine: command for Cmd-Arrow(Left|Right) rather than moveToBeginningOfLine: or moveToEndOfLine:, isn't it? If so, adding physical move commands is the simplest fix.

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #34)

(In reply to Huji from comment #33)

Approach Two: let Firefox/Cocoa have a different interpretation of COMMAND+LEFTARROW based on directionality context, that is in macOS in LTR it would mean KEY_NAME_INDEX_Home and in macOS in RTL it would mean KEY_NAME_INDEX_End. This lets NativeKeyBindings.mm untouched, and moves the definition to somewhere else. KeyNameList.h is where the constants like KEY_NAME_INDEX_Home are defined, but it already depends on an aCPPName called "Home", which I don't know where it is defined. I were to guess, it is macOS itself passing the "Home" key to the software whenever COMMAND+LEFTARROW is pressed (regardless of directionality). So while this option might be "cleaner" it may not be doable if the binding of COMMAND+LEFTARROW to the Home key is at the OS level.

In my understanding, Cocoa sends moveToLeftEndOfLine: or moveToRightEndOfLine: command for Cmd-Arrow(Left|Right) rather than moveToBeginningOfLine: or moveToEndOfLine:, isn't it? If so, adding physical move commands is the simplest fix.

Your understanding is correct (for instance, see lines 493-512). However, I don't follow the rest of your comment. If I am reading the code right, currently moveToLeftEndOfLineAndModifySelection: simply maps to Command::SelectBeginLine. Which is why when "left" means end of line (i.e. in RTL context), then SHIFT + ArrowLeft i.e. moveToLeftEndOfLineAndModifySelection: doesn't really move the caret to the end of the line. Per first part of your comment above, it is not possible to know bidi state at the time this code is run; then how can we associate SHIFT + ArrowLeft with a different action?

In other words, we need a Command::SelectLeftOfLine command and this command should be bidi-aware. But where are these commands actually defined? My lookups ends in https://searchfox.org/mozilla-central/source/widget/CommandList.h#58 where SelectBeginLine is mapped to cmd_selectBeginLine but I can't tell where the latter is defined.

(In reply to Huji from comment #35)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #34)

In my understanding, Cocoa sends moveToLeftEndOfLine: or moveToRightEndOfLine: command for Cmd-Arrow(Left|Right) rather than moveToBeginningOfLine: or moveToEndOfLine:, isn't it? If so, adding physical move commands is the simplest fix.

Your understanding is correct (for instance, see lines 493-512).

Thank you for verifying it!

However, I don't follow the rest of your comment. If I am reading the code right, currently moveToLeftEndOfLineAndModifySelection: simply maps to Command::SelectBeginLine. Which is why when "left" means end of line (i.e. in RTL context), then SHIFT + ArrowLeft i.e. moveToLeftEndOfLineAndModifySelection: doesn't really move the caret to the end of the line. Per first part of your comment above, it is not possible to know bidi state at the time this code is run; then how can we associate SHIFT + ArrowLeft with a different action?

So, there should be Command::SelectLeftLine for example, and map moveToLeftEndOfLineAndModifySelection: etc to the new commands. Then, these command handlers should use nsISelectionController::PhysicalMove around here and here after registering the command around here and here.

Hi,
I'm having the same issue on Mac (Big Sur), with the latest version of FF. Shift+arrow (or any combination including these two keys) moves in the opposite direction when writing RTL. I have no knowledge in coding, patching, etc', – What can I do to solve this irritating issue?

Just tried nightly. It's better there, (shift+arrow) seems to work fine, but Ctrl+arrow still goes the wrong way. Ugh.

Any help would be appreciated.

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

Severity: major → --

[Tracking Requested - why for this release]:

Description:
Please provide an explanation of the feature or change. Include a description of the user scenario in which it would be used and how the user would complete the task(s).
Screenshots and visual UI specs are welcome, but please include sufficient accompanying explanation so that blind members of the accessibility team are able to understand the feature/change.

How do we test this?
If there is an implementation to test, please provide instructions for testing it; e.g. setting preferences, other preparation, how to trigger the UI, etc.

When will this ship?
Tracking bug/issue:
Design documents (e.g. Product Requirements Document, UI spec):
Engineering lead:
Product manager:

The accessibility team has developed the Mozilla Accessibility Release Guidelines which outline what is needed to make user interfaces accessible:
https://wiki.mozilla.org/Accessibility/Guidelines
Please describe the accessibility guidelines you considered and what steps you've taken to address them:

Describe any areas of concern to which you want the accessibility team to give special attention:

a11y-review: --- → requested
status-firefox109: --- → ?
tracking-firefox109: --- → ?

(In reply to Release mgmt bot [:suhaib / :marco/ :calixte] from comment #41)

In the process of migrating remaining bugs to the new severity system, the severity for this bug cannot be automatically determined. Please retriage this bug using the new severity system.

I'm sorry, the above comment was my cumbersome attempt to reply to this comment that I received by mail, and to change the priority of this issue according to the new system (seems like an S2 or S3 severity to me). Apparently, I don't know how/where to do that 🙃 Probably only the "triage owner" can change the severity, in which case, I wonder why I (and not only him) got the email?

Flags: needinfo?(echen)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #32)

Those commands refer logical direction according to their names.
https://searchfox.org/mozilla-central/rev/449d0a0f57d1e603721cf79645e19c5673c59dd0/widget/CommandList.h#27,39,58,62-64,68-71

Perhaps, there should be corresponding commands which refer physical direction. Then, NativeKeyBindings and ChildView should map Cocoa's physical direction commands to the new ones.

And it might need to touch selection code too if some new commands don't work with nsISelectionController::PhysicalMove.

Unfortunately, 3 big task in my queue, so I cannot work on this bug for now...

Hey there Masayuki, I have no idea about coding... Can you please help guide me about how to fix this?

a11y-review: requested → ---

(In reply to eranteicher from comment #44)

(In reply to Masayuki Nakano [:masayuki] (he/him)(JST, +0900) from comment #32)

Those commands refer logical direction according to their names.
https://searchfox.org/mozilla-central/rev/449d0a0f57d1e603721cf79645e19c5673c59dd0/widget/CommandList.h#27,39,58,62-64,68-71

Perhaps, there should be corresponding commands which refer physical direction. Then, NativeKeyBindings and ChildView should map Cocoa's physical direction commands to the new ones.

And it might need to touch selection code too if some new commands don't work with nsISelectionController::PhysicalMove.

Unfortunately, 3 big task in my queue, so I cannot work on this bug for now...

Hey there Masayuki, I have no idea about coding... Can you please help guide me about how to fix this?

See comment 36.

Severity: -- → S3
Flags: needinfo?(echen)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: