Last Comment Bug 753076 - [AccessFu] Make directional controller interoperate with text entries.
: [AccessFu] Make directional controller interoperate with text entries.
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: ARM Android
: -- major (vote)
: mozilla15
Assigned To: Eitan Isaacson [:eeejay]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2012-05-08 13:18 PDT by Eitan Isaacson [:eeejay]
Modified: 2012-05-16 03:48 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Allow d-pad keys to interoperate with text entry as well as virtual cursor. (6.15 KB, patch)
2012-05-14 19:23 PDT, Eitan Isaacson [:eeejay]
dbolter: review+
Details | Diff | Splinter Review

Description Eitan Isaacson [:eeejay] 2012-05-08 13:18:41 PDT
Right now, if you land on an entry, type in some text and then press left or right to move the caret the virtual cursor changes instead. We need to have both modes work in harmony. In native Android widgets, the directional controller will move the caret in an entry, and when it reaches an edge it will move the virtual cursor.
Comment 1 Marco Zehe (:MarcoZ) 2012-05-08 20:47:42 PDT
Will the directional controller up and down also navigate by line in a textarea? This is esp important after fix for bug 752986.
Comment 2 Eitan Isaacson [:eeejay] 2012-05-08 21:06:48 PDT
Good point. The fix for this bug should do both horizontal and vertical nav.
Comment 3 Eitan Isaacson [:eeejay] 2012-05-10 12:34:55 PDT
Apparently, this is also an issue with the enter key. Some devices don't have the d-pad center/ok key. So we need to have return work well in all contexts.
Comment 4 Eitan Isaacson [:eeejay] 2012-05-14 19:23:19 PDT
Created attachment 623909 [details] [diff] [review]
Allow d-pad keys to interoperate with text entry as well as virtual cursor.
Comment 5 Eitan Isaacson [:eeejay] 2012-05-14 19:31:53 PDT
You will love this: There are no caret or text selection events in Android pre-ICS. So don't expect any useful text editing verbosity, besides entering single characters.

You could expect the same in other Android apps. Insert five characters in the google search app, and then press five times left in silence until the focus will move back to the button to the left of the entry. Yay!
Comment 6 Marco Zehe (:MarcoZ) 2012-05-14 23:22:31 PDT
Comment on attachment 623909 [details] [diff] [review]
Allow d-pad keys to interoperate with text entry as well as virtual cursor.

>+          // Don't blur content of caret is not at start of text area.

"if", not "of".

>+        if (this._isEditableText(target))
>+          return;
>+        this.activateCurrent(document);

What about the case of multiline text entries and wanting to insert a new line?
Comment 7 alexander :surkov 2012-05-15 00:36:47 PDT
Comment on attachment 623909 [details] [diff] [review]
Allow d-pad keys to interoperate with text entry as well as virtual cursor.

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

::: accessible/src/jsat/VirtualCursorController.jsm
@@ +39,5 @@
>          return this.chromeWin.gBrowser;
>      }
>    },
>  
> +  handleEvent: function handleEvent(aEvent) {

if you touch it then add a function name

@@ +54,5 @@
>        case aEvent.DOM_VK_RIGHT:
> +        if (this._isEditableText(target) &&
> +            target.selectionEnd != target.textLength)
> +          // Don't move forward if caret is not at end of entry.
> +          return;

is that consequence of unimplemented navigation through text? Ideally (and you can do that here) you should have modes (movement by objects and movement by text (chars and etc)). So you can to switch between modes depending on the focus position and provide shortcuts additionally. Otherwise it's seems if the user is inside textbox then he must to finish all text before he can move further which is not convenient.

@@ +68,5 @@
>        case aEvent.DOM_VK_UP:
> +        if (this._isEditableText(target) == this.MULTI_LINE_EDITABLE &&
> +            target.selectionEnd != 0)
> +          // Don't blur content of caret is not at start of text area.
> +          return;

this might be a behavior the user don't expect because up key in textboxes moves a caret one char left (similar for down key).

@@ +97,5 @@
> +
> +    if (aElement instanceof Ci.nsIDOMHTMLTextAreaElement)
> +      return this.MULTI_LINE_EDITABLE;
> +
> +    return this.NOT_EDITABLE;

you don't have contentEditable and midas documents
Comment 8 David Bolter [:davidb] 2012-05-15 06:28:15 PDT
(In reply to alexander :surkov from comment #7)
> Comment on attachment 623909 [details] [diff] [review]
> @@ +54,5 @@
> >        case aEvent.DOM_VK_RIGHT:
> > +        if (this._isEditableText(target) &&
> > +            target.selectionEnd != target.textLength)
> > +          // Don't move forward if caret is not at end of entry.
> > +          return;
> 
> is that consequence of unimplemented navigation through text? Ideally (and
> you can do that here) you should have modes (movement by objects and
> movement by text (chars and etc)). So you can to switch between modes
> depending on the focus position and provide shortcuts additionally.
> Otherwise it's seems if the user is inside textbox then he must to finish
> all text before he can move further which is not convenient.

Marco, Eitan can you comment on the UI?

Alexander, note we are still in iterative development here.
Comment 9 David Bolter [:davidb] 2012-05-15 06:41:37 PDT
Comment on attachment 623909 [details] [diff] [review]
Allow d-pad keys to interoperate with text entry as well as virtual cursor.

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

r=me. I'm okay taking this so that we can dogfood better but I don't think we're done here :)

Eitan have you told the burgeoning community that nothing is set in stone yet (UI/Ux etc)?

::: accessible/src/jsat/VirtualCursorController.jsm
@@ +39,5 @@
>          return this.chromeWin.gBrowser;
>      }
>    },
>  
> +  handleEvent: function handleEvent(aEvent) {

There is a function name :)

@@ +68,5 @@
>        case aEvent.DOM_VK_UP:
> +        if (this._isEditableText(target) == this.MULTI_LINE_EDITABLE &&
> +            target.selectionEnd != 0)
> +          // Don't blur content of caret is not at start of text area.
> +          return;

Alexander, we have too few gestures to play with. We can't have up and back do the same thing IMO.

@@ +97,5 @@
> +
> +    if (aElement instanceof Ci.nsIDOMHTMLTextAreaElement)
> +      return this.MULTI_LINE_EDITABLE;
> +
> +    return this.NOT_EDITABLE;

I'd be ok with // XXX for contentEditable and design mode... not sure we need that for this round (on Android).
Comment 10 Eitan Isaacson [:eeejay] 2012-05-15 09:12:22 PDT
(In reply to alexander :surkov from comment #7)
> Comment on attachment 623909 [details] [diff] [review]
> Allow d-pad keys to interoperate with text entry as well as virtual cursor.
> 
> Review of attachment 623909 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: accessible/src/jsat/VirtualCursorController.jsm
> @@ +39,5 @@
> >          return this.chromeWin.gBrowser;
> >      }
> >    },
> >  
> > +  handleEvent: function handleEvent(aEvent) {
> 
> if you touch it then add a function name
> 

Thanks for noticing :)

> @@ +54,5 @@
> >        case aEvent.DOM_VK_RIGHT:
> > +        if (this._isEditableText(target) &&
> > +            target.selectionEnd != target.textLength)
> > +          // Don't move forward if caret is not at end of entry.
> > +          return;
> 
> is that consequence of unimplemented navigation through text? Ideally (and
> you can do that here) you should have modes (movement by objects and
> movement by text (chars and etc)). So you can to switch between modes
> depending on the focus position and provide shortcuts additionally.
> Otherwise it's seems if the user is inside textbox then he must to finish
> all text before he can move further which is not convenient.
> 

See comment 5. We are bringing it to parity with pre-ICS a11y right now, not more.

> @@ +68,5 @@
> >        case aEvent.DOM_VK_UP:
> > +        if (this._isEditableText(target) == this.MULTI_LINE_EDITABLE &&
> > +            target.selectionEnd != 0)
> > +          // Don't blur content of caret is not at start of text area.
> > +          return;
> 
> this might be a behavior the user don't expect because up key in textboxes
> moves a caret one char left (similar for down key).
> 

I tested this. If you are in a middle column in the first row, and press up, the caret goes to offset 0.

> @@ +97,5 @@
> > +
> > +    if (aElement instanceof Ci.nsIDOMHTMLTextAreaElement)
> > +      return this.MULTI_LINE_EDITABLE;
> > +
> > +    return this.NOT_EDITABLE;
> 
> you don't have contentEditable and midas documents

Right.
Comment 11 Eitan Isaacson [:eeejay] 2012-05-15 09:13:27 PDT
(In reply to Marco Zehe (:MarcoZ) from comment #6)
> Comment on attachment 623909 [details] [diff] [review]
> Allow d-pad keys to interoperate with text entry as well as virtual cursor.
> 
> >+          // Don't blur content of caret is not at start of text area.
> 
> "if", not "of".
> 

Thanks for catching.

> >+        if (this._isEditableText(target))
> >+          return;
> >+        this.activateCurrent(document);
> 
> What about the case of multiline text entries and wanting to insert a new
> line?

It works, once the entry or text area has keyboard focus we pass the event on to it.
Comment 12 Eitan Isaacson [:eeejay] 2012-05-15 09:39:49 PDT
(In reply to David Bolter [:davidb] from comment #9)
> Comment on attachment 623909 [details] [diff] [review]
> Allow d-pad keys to interoperate with text entry as well as virtual cursor.
> 
> Review of attachment 623909 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me. I'm okay taking this so that we can dogfood better but I don't think
> we're done here :)
> 
> Eitan have you told the burgeoning community that nothing is set in stone
> yet (UI/Ux etc)?
> 
> ::: accessible/src/jsat/VirtualCursorController.jsm
> @@ +39,5 @@
> >          return this.chromeWin.gBrowser;
> >      }
> >    },
> >  
> > +  handleEvent: function handleEvent(aEvent) {
> 
> There is a function name :)
> 

Best part of this patch...

> @@ +68,5 @@
> >        case aEvent.DOM_VK_UP:
> > +        if (this._isEditableText(target) == this.MULTI_LINE_EDITABLE &&
> > +            target.selectionEnd != 0)
> > +          // Don't blur content of caret is not at start of text area.
> > +          return;
> 
> Alexander, we have too few gestures to play with. We can't have up and back
> do the same thing IMO.
> 

Yup, need to plan this carefully.

> @@ +97,5 @@
> > +
> > +    if (aElement instanceof Ci.nsIDOMHTMLTextAreaElement)
> > +      return this.MULTI_LINE_EDITABLE;
> > +
> > +    return this.NOT_EDITABLE;
> 
> I'd be ok with // XXX for contentEditable and design mode... not sure we
> need that for this round (on Android).

Sounds good. The biggest omission is actually rtl text. I'll open a separate bug for that.
Comment 14 alexander :surkov 2012-05-15 21:30:29 PDT
(In reply to David Bolter [:davidb] from comment #8)

> Alexander, note we are still in iterative development here.

I realize it but iterative process shouldn't void the designing. The code introduction supposed to be removed in foreseeable future is not right thing.

(In reply to David Bolter [:davidb] from comment #9)

> > +  handleEvent: function handleEvent(aEvent) {
> 
> There is a function name :)

i meant name convention :)

> Alexander, we have too few gestures to play with. We can't have up and back
> do the same thing IMO.

not sure I follow, I meant the behavior should be consistent. Up key moving the cursor from editable area can be considers by uses as a bug.
Comment 15 alexander :surkov 2012-05-15 21:37:23 PDT
(In reply to Eitan Isaacson [:eeejay] from comment #10)
 
> > is that consequence of unimplemented navigation through text? Ideally (and
> > you can do that here) you should have modes (movement by objects and
> > movement by text (chars and etc)). So you can to switch between modes
> > depending on the focus position and provide shortcuts additionally.
> > Otherwise it's seems if the user is inside textbox then he must to finish
> > all text before he can move further which is not convenient.
> > 
> 
> See comment 5. We are bringing it to parity with pre-ICS a11y right now, not
> more.

I don't see where is contradiction.

> I tested this. If you are in a middle column in the first row, and press up,
> the caret goes to offset 0.

it's platform dependent but it's ok if that's a default andorid behavior.
Comment 16 Eitan Isaacson [:eeejay] 2012-05-15 21:42:34 PDT
(In reply to alexander :surkov from comment #14)
> (In reply to David Bolter [:davidb] from comment #9)
> > > +  handleEvent: function handleEvent(aEvent) {
> > 
> > There is a function name :)
> 
> i meant name convention :)
> 

handleEvent is an interface method name for DOM event listeners.

> > Alexander, we have too few gestures to play with. We can't have up and back
> > do the same thing IMO.
> 
> not sure I follow, I meant the behavior should be consistent. Up key moving
> the cursor from editable area can be considers by uses as a bug.

As far as navigation, it is the current broken behaviour on Android that everyone expects. So it is consistent :)
Comment 17 alexander :surkov 2012-05-15 22:02:26 PDT
(In reply to Eitan Isaacson [:eeejay] from comment #16)
> handleEvent is an interface method name for DOM event listeners.

that's fine but convention is methodName: function className_methodName()

> > > Alexander, we have too few gestures to play with. We can't have up and back
> > > do the same thing IMO.
> > 
> > not sure I follow, I meant the behavior should be consistent. Up key moving
> > the cursor from editable area can be considers by uses as a bug.
> 
> As far as navigation, it is the current broken behaviour on Android that
> everyone expects. So it is consistent :)

ok, it makes sense to figure out the correct behavior early
Comment 18 Ed Morley [:emorley] 2012-05-16 03:48:33 PDT
https://hg.mozilla.org/mozilla-central/rev/f24cb2b6f8f9

Note You need to log in before you can comment on or make changes to this bug.