left/right/up/down arrow keys should be remapped logically for vertical writing modes

RESOLVED FIXED in mozilla36

Status

()

defect
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: jfkthame, Assigned: jfkthame)

Tracking

(Depends on 2 bugs, Blocks 1 bug)

unspecified
mozilla36
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(12 attachments, 2 obsolete attachments)

24.77 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.76 KB, patch
roc
: review+
Details | Diff | Splinter Review
44.58 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.75 KB, patch
roc
: review+
Details | Diff | Splinter Review
7.04 KB, patch
roc
: review+
Details | Diff | Splinter Review
10.90 KB, patch
roc
: review+
Details | Diff | Splinter Review
14.28 KB, patch
roc
: review+
Details | Diff | Splinter Review
7.40 KB, patch
roc
: review+
Details | Diff | Splinter Review
5.03 KB, patch
roc
: review+
Details | Diff | Splinter Review
30.10 KB, patch
Details | Diff | Splinter Review
5.39 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.73 KB, patch
roc
: review+
Details | Diff | Splinter Review
(Assignee)

Description

5 years ago
In vertical writing mode, the up/down arrows should move backwards/forwards by characters within the line; and the left/right arrows should move to previous/next line. (With the appropriate mapping for left/right arrows depending whether the mode is vertical-rl or vertical-lr.)
(Assignee)

Comment 1

5 years ago
This fixes the behavior by in effect remapping the arrow keys within nsFrameSelection::MoveCaret. This means that we may end up doing something quite different than the aAmount parameter suggests; that seems a bit hackish, but appears to work OK in my testing so far. Is this a reasonable approach, or do we need to make callers query the writing mode so that they can pass in the correct aAmount?
Attachment #8501182 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Assignee: nobody → jfkthame
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
Hmm, on thinking about it, this is a bit problematic: the patch above makes simple arrow keys work pretty well, but we don't get the ability to move forward/backward by word rather than character (cluster) in vertical text.

To make that work, I think we may need to look at the (platform-specific) code that maps (modified-)arrow keystrokes to selection-movement commands, and make that sensitive to writing mode as well.
Right.

I think some refactoring is needed. I think we need a clear distinction in nsFrameSelection between methods that take logical directions and methods that take physical directions. Also I think nsFrameSelection::MoveCaret taking a keycode is a bug; it should take an enum, since AFAICT we're not actually passing keycodes into it.

And then, yes, the code that maps keystrokes to nsFrameSelection calls should be modified to use logical directions.
(Assignee)

Updated

5 years ago
(Assignee)

Comment 4

5 years ago
OK, I've made a start on a more thorough approach here. First, rework nsFrameSelection::MoveCaret and associated code to eliminate the use of "keyCode" values.
Attachment #8522938 - Flags: review?(roc)
(Assignee)

Comment 5

5 years ago
The patch above passes all current tests, with the added bonus that it fixes a handful of existing failures for RTL content in the browserscope HTML editor test. So we can remove those from our known-failures list.
Attachment #8522940 - Flags: review?(roc)
(Assignee)

Comment 6

5 years ago
This fixes nsPeekOffsetStruct and its users so that the position within the line is maintained properly when moving to previous/next line in vertical mode as well as horizontal. So at this point, cursor movement in vertical writing mode seems to work fine, except for the actual remapping of the physical arrow keys (i.e. right-arrow still means next-character, etc.), which will need to be handled by the keyEvent-to-Gecko-command mappings.
Attachment #8522942 - Flags: review?(roc)
(Assignee)

Updated

5 years ago
Attachment #8501182 - Attachment is obsolete: true
Attachment #8501182 - Flags: review?(roc)
Comment on attachment 8522942 [details] [diff] [review]
part 3 - Change desiredX (nscoord) to desiredPos (nsPoint) in nsPeekOffsetStruct, to support maintaining either vertical or horizontal position on inter-line moves.

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

This looks great!

::: layout/generic/nsLineBox.cpp
@@ +752,1 @@
>        // If aX is inside this frame - this is it

fix comment
Attachment #8522942 - Flags: review?(roc) → review+
(Assignee)

Comment 8

5 years ago
For widget key-mapping code to be able to adapt appropriately for vertical content, it needs to be able to determine the writing-mode of the frame that's receiving the keystroke. To support this, we can add a WritingMode to the reply to the NS_QUERY_SELECTED_TEXT event (which may also be wanted for IME support in bug 1076657); then we can use this to decide how to remap arrow keys.
Attachment #8523410 - Flags: review?(roc)
(Assignee)

Comment 9

5 years ago
Handle vertical mode when calling the Cocoa native key-bindings. This gives the expected behavior when editing vertical text on OS X. (Other platforms not yet addressed.)
Attachment #8523411 - Flags: review?(roc)
Comment on attachment 8523410 [details] [diff] [review]
part 4 - Add writing-mode to the reply to the NS_QUERY_SELECTED_TEXT event.

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

r+ with that fixed.

::: dom/events/ContentEventHandler.cpp
@@ +664,5 @@
>    }
>  
> +  nsIFrame* frame = nullptr;
> +  rv = GetFrameForTextRect(focusNode, focusOffset, true, &frame);
> +  NS_ENSURE_SUCCESS(rv, rv);

This will make OnQuerySelectedText fail when the selection focusNode is in display:none content. I don't think we should change that behavior here. Let's just return the default writing-mode in that case.
Attachment #8523410 - Flags: review?(roc) → review+
Comment on attachment 8523411 [details] [diff] [review]
part 5 - Remap arrow keys for vertical writing-mode in the Cocoa key-bindings code.

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

I guess this still isn't ideal --- if something introduced a strange binding for VK_UP then we could trigger it here with VK_LEFT and that would be weird. On the other hand, if we fix this any other way, we can also get weird behavior (e.g. if we modify the behavior of cmdLeft then if a new key is bound to cmdLeft, it would change behavior which would also likely be wrong). So I can't think of any better approach. Presumably the chances of VK_LEFT/UP/DOWN/RIGHT being rebound in a strange way are minimal anyway.
Attachment #8523411 - Flags: review?(roc) → review+
(Assignee)

Comment 12

5 years ago
Part 5 here remaps the arrow keys successfully (modulo the concern mentioned in comment 11, but I don't see how we can avoid that at some level) for editable text elements on OS X.

However, it doesn't handle the case of using shift-arrow keys to select within non-editable text, because in this case the key mapping isn't handled via the native Cocoa widget code at all; it goes through nsXBLWindowKeyHandler, using the XBL mappings in dom/xbl/builtin/mac/platformHTMLBindings.xml etc. Can we somehow determine -- e.g. in nsXBLWindowKeyHandler::WalkHandlersAndExecute or thereabouts -- whether there's a current selection that a shift-arrow combination will modify (in which case we'd need to find out the writing-mode and remap accordingly before checking the handlers for a match) or not?
Flags: needinfo?(roc)
I think we should introduce physical commands, e.g. shift-VK_UP could map to a new cmd_extendSelectionUp, and shift-alt-VK_LEFT could map to cmd_extendSelectionLeftWord.
Flags: needinfo?(roc)
(Assignee)

Comment 19

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #13)
> I think we should introduce physical commands, e.g. shift-VK_UP could map to
> a new cmd_extendSelectionUp, and shift-alt-VK_LEFT could map to
> cmd_extendSelectionLeftWord.

OK, I've taken a stab at this. I've gone with names cmd_moveLeft, cmd_selectLeft, etc for the simple arrow-key movements and shift-arrow selection; and a parallel set of cmd_moveLeft2, etc., for movement/selection by a larger amount (with the Ctrl key). This avoids putting things like "word" in the command name, given that (for example) Ctrl-RightArrow may be "move right word" in horizontal mode, but it's "move down to line-end" in vertical mode.

I'm not thrilled with that command naming, but nor do I have an idea that I really like better (cmd_moveLeftMore, etc?); suggestions welcomed.
Comment hidden (obsolete)
Comment hidden (obsolete)
Comment on attachment 8525583 [details] [diff] [review]
part 6 - Create a new nsISelectionController::PhysicalMove command.

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

Nice!
Attachment #8525583 - Flags: review?(roc) → review+
Comment on attachment 8525584 [details] [diff] [review]
part 7 - Support physical caret movement and selection commands in nsGlobalWindowCommands.

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

::: dom/base/nsGlobalWindowCommands.cpp
@@ +357,5 @@
> +      if (docShell && docShell->ItemType() == nsIDocShellTreeItem::typeChrome) {
> +        caretOn = false;
> +      }
> +    }
> +  }

Please share this logic with nsSelectMoveScrollCommand::DoCommand where I assume you copied it from.

@@ +374,5 @@
> +                        nsIFocusManager::FLAG_NOSCROLL,
> +                        getter_AddRefs(result));
> +        }
> +        return NS_OK;
> +      }

And share this logic too.
Attachment #8525584 - Flags: review?(roc) → review+
(Assignee)

Comment 24

5 years ago
While testing vertical-enabled builds with these patches, I noticed that patch 10 missed some of the XBL arrow-key bindings. Added those here; carrying over r=roc.
(Assignee)

Updated

5 years ago
Attachment #8525587 - Attachment is obsolete: true
(Assignee)

Comment 25

5 years ago
And here's the Linux version of patch 5, needed to make things work as expected in contentEditable elements, where we rely on the platform key bindings rather than the XBL bindings.
Attachment #8526701 - Flags: review?(roc)
(Assignee)

Comment 26

5 years ago
One more problem here: it turned out that after remapping everything, the up/down arrows don't work in <input> elements on Windows and Linux. This is because the nsFormFillController decides to block those key events. So we need to remap there as well, as in vertical <input> elements it should be considering left/right instead. (I'm sure there will be more key-direction issues to consider when we work on form elements properly, but for now this is enough to get the cursor keys working OK in <input> fields.)
Attachment #8526851 - Flags: review?(roc)
(Assignee)

Comment 28

5 years ago
And a followup to fix bustage on non-unified builds:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f2d3a639784f
Depends on: 1105101

Updated

5 years ago
Depends on: 1104712
Depends on: 1138419

Updated

3 years ago
Depends on: 1153237
Depends on: 1301497
You need to log in before you can comment on or make changes to this bug.