935807 - D-pad arrow keys move focus outside textarea instead of navigating within

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Keyboards and IME, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Using a device with a physical keyboard (Galaxy Q for instance) that has a d-pad:

1. Go to a page with a multiline textarea (such as a bugzilla comment field)
2. Enter a multiline comment
3. Attempt to navigate the lines using the up and down arrows on the d-pad

Expected:

You can move up and down a line using the up and down keys

Actual:

The focus jumps out of the textarea to surrounding elements.

This makes editing multiline textareas quite frustrating. It is a regression, I assume from Mina's patch that added d-pad focus navigation.

Comment 1

[Mina Almasry]

Yep that's me. The way to fix that I think is to restore the lines here:

https://bugzilla.mozilla.org/attachment.cgi?id=546796&action=diff

in file SpatialNavigation.js line 136.

That would be a quick fix and restore the previous behavior. A more thorough fix would make it so that prssing the action key with a text input element highlighted enters text editing mode, and subsequent d-pad key presses with navigate inside the text element. Then maybe pressing the esc key or the back key in the ouya would exit text editing mode, and you're back to jumping from element to element on the page.

Comment 2

[Aaron Train [:aaronmt]]

http://hg.mozilla.org/mozilla-central/pushloghtml?startdate=2013-10-13&enddate=2013-10-14

972d7d7be035	Mina Almasry — Bug 698437 - Allow spatial navigation with directional controller. r=kats

Comment 4

[Aaron Train [:aaronmt]]

Hardware Keyboard need not be required. 

This is reproducible with Hacker's Keyboard in landscape (they expose cursor controls).

User Report: https://support.mozilla.org/en-US/questions/977904
Hacker's Keyboard: https://play.google.com/store/apps/details?id=org.pocketworkstation.pckeyboard&hl=en

Comment 5

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Restore arrow key input field navigation behavior.

I restored the original behavior as mentioned in comment 1. Additionally, I
left the functionality Mina added where selecting a new input element will
select all of the text inside since 1) I feel it makes sense and 2) I don't
have a gamepad to test what the effects of this change would be on it (which
I believe is the original reason this file was re-added).

Tested on TouchPal (it has issues - see bug 943042), Hacker's Keyboard, and a
hardware keyboard.

Comment 6

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Restore arrow key input field navigation behavior.

Changing reviewer tag.

Comment 7

[Mina Almasry]

I'm available for review if no one else would do it btw :-)

Comment 8

[Brian Nicholson (:bnicholson)]

Comment on attachment 8338018 [details] [diff] [review]
Restore arrow key input field navigation behavior.

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

::: toolkit/modules/SpatialNavigation.jsm
@@ +126,5 @@
>    }
>  
> +  if ((currentlyFocused instanceof Ci.nsIDOMHTMLInputElement &&
> +       currentlyFocused.mozIsTextField(false)) ||
> +      currentlyFocused instanceof Ci.nsIDOMHTMLTextAreaElement) {

Nit: tab indentation

@@ +128,5 @@
> +  if ((currentlyFocused instanceof Ci.nsIDOMHTMLInputElement &&
> +       currentlyFocused.mozIsTextField(false)) ||
> +      currentlyFocused instanceof Ci.nsIDOMHTMLTextAreaElement) {
> +    // If there is a text selection, remain in the element.
> +    if (currentlyFocused.selectionEnd - currentlyFocused.selectionStart > 0) {

Is this safe for right-to-left selections and RTL users? Could we just use | selectionStart != selectionEnd |?

@@ +134,5 @@
> +    }
> +
> +    // If there is no text, there is nothing special to do.
> +    if (currentlyFocused.textLength > 0) {
> +      if (key == PrefObserver['keyCodeRight'] ||

Is this RTL-safe?

@@ +140,5 @@
> +        // We are moving forward into the document.
> +        if (currentlyFocused.textLength != currentlyFocused.selectionEnd) {
> +          return;
> +        }
> +      } else {

Nit: } else if (...) {

Comment 9

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: Restore arrow key input field navigation behavior.

Fixed the nits.

(In reply to Brian Nicholson (:bnicholson) from comment #8)
> Is this safe for right-to-left selections and RTL users? Could we just use |
> selectionStart != selectionEnd |?

Done.

> @@ +134,5 @@
> > +    }
> > +
> > +    // If there is no text, there is nothing special to do.
> > +    if (currentlyFocused.textLength > 0) {
> > +      if (key == PrefObserver['keyCodeRight'] ||
> 
> Is this RTL-safe?

Probably not but the precedent exists elsewhere in the code. I think the most
appropriate action would be to file a follow-up since this affects 27.

Note also that RTL text selection seems to be entirely broken (tested on 25 and
28 on Arabic - see https://people.mozilla.org/~mcomella/test/textarea.html).

Comment 10

[Gian-Carlo Pascutto [:gcp]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/595a838d2db8

Comment 11

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

> I think the most appropriate action would be to file a follow-up since this affects 27.

bug 946886.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/595a838d2db8

Comment 13

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Comment on attachment 8342712 [details] [diff] [review]
Restore arrow key input field navigation behavior.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
  Bug 698437

User impact if declined: 
  Users cannot navigate between text fields with the arrow keys

Testing completed (on m-c, etc.):
  Local testing with TouchPal/Hacker's Keyboard/hardware keyboard, merged to m-c today.
 
Risk to taking this patch (and alternatives if risky):
  Low to medium. This affects the _onInputKeyPress method that gets called when a key is pressed so in the worst case, all keyboard input may break. However, the existing method code only allows us to observe keystrokes for the arrow keys and return. The added code specifically only looks for at those keystrokes when a text field is focused, and will only escape back to the behavior that occurred before the regressing bug was implemented (via extra "return;" statements) so the possibility for major error is minimal.

String or IDL/UUID changes made by this patch:
  None

Comment 14

[bhavana bajaj [:bajaj]]

Comment on attachment 8342712 [details] [diff] [review]
Restore arrow key input field navigation behavior.

Approving this change ahead of the merge. Requesting QA verification on nightly/aurora as soon as they can, so we can identify any fallouts. If something major is broken, we should immediately back this out.

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4fb95c60417e