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.)

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?

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.

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.

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.

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.

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

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.

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.)

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.

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.

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?

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.

(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 on attachment 8525583 [details] [diff] [review] part 6 - Create a new nsISelectionController::PhysicalMove command. Review of attachment 8525583 [details] [diff] [review]: ----------------------------------------------------------------- Nice!

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.

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.

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.

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.)

https://hg.mozilla.org/integration/mozilla-inbound/rev/66f1f54e19bc https://hg.mozilla.org/integration/mozilla-inbound/rev/d779e840b214 https://hg.mozilla.org/integration/mozilla-inbound/rev/0c5bb0328ec8 https://hg.mozilla.org/integration/mozilla-inbound/rev/b51ce3f80311 https://hg.mozilla.org/integration/mozilla-inbound/rev/c873b373dc1d https://hg.mozilla.org/integration/mozilla-inbound/rev/b45361b7aab2 https://hg.mozilla.org/integration/mozilla-inbound/rev/e8d6b1cf145c https://hg.mozilla.org/integration/mozilla-inbound/rev/3260d0792a32 https://hg.mozilla.org/integration/mozilla-inbound/rev/228cc7984e2f https://hg.mozilla.org/integration/mozilla-inbound/rev/23fcc9599371 https://hg.mozilla.org/integration/mozilla-inbound/rev/4c90e4b108ec https://hg.mozilla.org/integration/mozilla-inbound/rev/203d3b5da245 We still need a set of tests for all this, when vertical writing-mode is enabled; filed bug 1103374 for this.

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

https://hg.mozilla.org/mozilla-central/rev/66f1f54e19bc https://hg.mozilla.org/mozilla-central/rev/d779e840b214 https://hg.mozilla.org/mozilla-central/rev/0c5bb0328ec8 https://hg.mozilla.org/mozilla-central/rev/b51ce3f80311 https://hg.mozilla.org/mozilla-central/rev/c873b373dc1d https://hg.mozilla.org/mozilla-central/rev/b45361b7aab2 https://hg.mozilla.org/mozilla-central/rev/e8d6b1cf145c https://hg.mozilla.org/mozilla-central/rev/3260d0792a32 https://hg.mozilla.org/mozilla-central/rev/228cc7984e2f https://hg.mozilla.org/mozilla-central/rev/23fcc9599371 https://hg.mozilla.org/mozilla-central/rev/4c90e4b108ec https://hg.mozilla.org/mozilla-central/rev/203d3b5da245 https://hg.mozilla.org/mozilla-central/rev/f2d3a639784f