831144 - Implement editor key bindings on Android

Status: RESOLVED FIXED

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

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

Android editors have a unique set of key bindings, as implemented through the MovementMethod interface [1]. For example,

Ctrl+Left moves cursor to start of line,
Ctrl+Right moves cursor to end of line,
Alt+Left moves cursor to previous word,
Alt+Right moves cursor to next word

Right now we seem to use Emacs-like bindings [2] because we don't have platform-specific bindings for Android [3], but it should be straightforward to add a set of custom bindings for Android. This would make text input experience in Fennec more in line with the rest of Android (especially on devices with hardware keyboards), and is also necessary to fix bugs (e.g. Bug 706336) which assume the Android bindings.

[1] http://developer.android.com/reference/android/text/method/MovementMethod.html
[2] http://mxr.mozilla.org/mozilla-central/source/content/xbl/builtin/emacs/platformHTMLBindings.xml
[3] http://mxr.mozilla.org/mozilla-central/source/content/xbl/builtin/Makefile.in

Comment 2

[(inactive) Jim Chen [:jchen] [:darchons]]

I think this is worth fixing now with quite a few people requesting this feature.

Comment 3

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add Android XBL key bindings (v1)

Can you review this, Neil? I saw your name as a peer for /content/xbl/builtin

I started out using /content/xbl/builtin/unix as an example, and gathered navigation key bindings from [1] and delete key bindings from [2].

[1] http://androidxref.com/4.2_r1/xref/frameworks/base/core/java/android/text/method/BaseMovementMethod.java
[2] http://androidxref.com/4.2_r1/xref/frameworks/base/core/java/android/text/method/BaseKeyListener.java

Comment 4

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Properly pass meta states to Gecko (v1)

These changes make us able to pass combinations such as Ctrl+A to Gecko.

Comment 5

[[e-mail]]

(In reply to Jim Chen from comment #3)
> I started out using /content/xbl/builtin/unix as an example
Would you mind using hg cp to show that the files were based on existing files?

Comment 6

[[e-mail]]

Comment on attachment 717307 [details] [diff] [review]
Add Android XBL key bindings (v1)

> ifneq (,$(filter OS2 WINNT,$(OS_ARCH)))
> DIRS	= win
> else
> ifeq (cocoa,$(MOZ_WIDGET_TOOLKIT))
> DIRS	= mac
> else
> ifneq (,$(filter qt gtk2,$(MOZ_WIDGET_TOOLKIT)))
> DIRS	= unix
> else
>+ifeq (android,$(MOZ_WIDGET_TOOLKIT))
>+DIRS	= android
>+else
> DIRS	= emacs
> endif
> endif
> endif
>+endif
The placement is confusing, could you put your check outside the unix/emacs check please?

Comment 7

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add Android XBL key bindings (v2)

Thanks for the quick response Neil!

(In reply to [e-mail] from comment #5)
> (In reply to Jim Chen from comment #3)
> > I started out using /content/xbl/builtin/unix as an example
> Would you mind using hg cp to show that the files were based on existing
> files?

Okay. I'm on git right now but I'll do that when I move this to hg.

(In reply to [e-mail] from comment #6)
> Comment on attachment 717307 [details] [diff] [review]
> Add Android XBL key bindings (v1)
> 
> > ifneq (,$(filter OS2 WINNT,$(OS_ARCH)))
> > DIRS    = win
> > else
> > ifeq (cocoa,$(MOZ_WIDGET_TOOLKIT))
> > DIRS    = mac
> > else
> > ifneq (,$(filter qt gtk2,$(MOZ_WIDGET_TOOLKIT)))
> > DIRS    = unix
> > else
> >+ifeq (android,$(MOZ_WIDGET_TOOLKIT))
> >+DIRS    = android
> >+else
> > DIRS    = emacs
> > endif
> > endif
> > endif
> >+endif
> The placement is confusing, could you put your check outside the unix/emacs
> check please?

So like this?

>  ifneq (,$(filter OS2 WINNT,$(OS_ARCH)))
>  DIRS   = win
>  else
>  ifeq (cocoa,$(MOZ_WIDGET_TOOLKIT))
>  DIRS   = mac
>  else
> +ifeq (android,$(MOZ_WIDGET_TOOLKIT))
> +DIRS   = android
> +else
>  ifneq (,$(filter qt gtk2,$(MOZ_WIDGET_TOOLKIT)))
>  DIRS   = unix
>  else
>  DIRS   = emacs
>  endif
>  endif
>  endif
> +endif

Comment 8

[[e-mail]]

(In reply to Jim Chen from comment #7)
> I'm on git right now but I'll do that when I move this to hg.
I've never used git but hg produces git-style diffs so surely git must have a way to show that content/xbl/builtin/android/jar.mn is a copy of content/xbl/builtin/unix/jar.mn?

Comment 9

[Chris Peterson [:cpeterson]]

Comment on attachment 717309 [details] [diff] [review]
Properly pass meta states to Gecko (v1)

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

LGTM!

Comment 10

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add Android XBL key bindings (v3)

(In reply to [e-mail] from comment #8)
> (In reply to Jim Chen from comment #7)
> > I'm on git right now but I'll do that when I move this to hg.
> I've never used git but hg produces git-style diffs so surely git must have
> a way to show that content/xbl/builtin/android/jar.mn is a copy of
> content/xbl/builtin/unix/jar.mn?

According to [1], git does not track copies but instead tries to detect copies (and it does a rather poor job at it when I tried).

I went ahead and moved this to hg. Right now android/platformHTMLBindings.xml is sufficiently different from unix/platformHTMLBindings.xml that I kept it a new file instead of a copy; this way the diff doesn't show deleting all the unix bindings, which I think is more confusing than helpful.

[1] http://stackoverflow.com/questions/1043388/record-file-copy-operation-with-git

Comment 11

[[e-mail]]

(In reply to Jim Chen from comment #10)
> According to [1], git does not track copies but instead tries to detect
> copies (and it does a rather poor job at it when I tried).
> 
> I went ahead and moved this to hg. Right now
> android/platformHTMLBindings.xml is sufficiently different from
> unix/platformHTMLBindings.xml that I kept it a new file instead of a copy;
> this way the diff doesn't show deleting all the unix bindings, which I think
> is more confusing than helpful.
Fair enough. (I did wonder whether the windows bindings looked closer to the android bindings, but I think you would still needed to have delete some of the bindings, which would make the result confusing anyway.)

(In reply to Jim Chen from comment #7)
> So like this?
> >  ifneq (,$(filter OS2 WINNT,$(OS_ARCH)))
> >  DIRS   = win
> >  else
> >  ifeq (cocoa,$(MOZ_WIDGET_TOOLKIT))
> >  DIRS   = mac
> >  else
> > +ifeq (android,$(MOZ_WIDGET_TOOLKIT))
> > +DIRS   = android
> > +else
> >  ifneq (,$(filter qt gtk2,$(MOZ_WIDGET_TOOLKIT)))
> >  DIRS   = unix
> >  else
> >  DIRS   = emacs
> >  endif
> >  endif
> >  endif
> > +endif
Yes, except the extra endif appears to have got lost in the move to hg :-(

Comment 12

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add Android XBL key bindings (v4)

(In reply to [e-mail] from comment #11)
> > >  DIRS   = emacs
> > >  endif
> > >  endif
> > >  endif
> > > +endif
> Yes, except the extra endif appears to have got lost in the move to hg :-(

Oops. Good catch! :)

Comment 13

[[e-mail]]

Comment on attachment 717424 [details] [diff] [review]
Add Android XBL key bindings (v4)

Huh, I wonder why cmd_selectChar/LinePrevious/Next aren't in browser-base.inc; I should file a bug on that.

Comment 14

[(inactive) Jim Chen [:jchen] [:darchons]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3f6e6912e71a
https://hg.mozilla.org/integration/mozilla-inbound/rev/77a94dc888c9

Comment 15

[Ryan VanderMeulen [:RyanVM]]

Backed out for Android mochitest failures.
https://hg.mozilla.org/integration/mozilla-inbound/rev/373699ce4c96

https://tbpl.mozilla.org/php/getParsedLog.php?id=20071168&tree=Mozilla-Inbound

6647 INFO TEST-PASS | /tests/editor/libeditor/html/tests/test_dom_input_event_on_htmleditor.html | input event must be bubbles
6648 INFO TEST-PASS | /tests/editor/libeditor/html/tests/test_dom_input_event_on_htmleditor.html | perhaps, timestamp wasn't set correctly :Mon Feb 25 10:05:21 2013
6649 INFO TEST-PASS | /tests/editor/libeditor/html/tests/test_dom_input_event_on_htmleditor.html | Editor1, body has contenteditable attribute: input event wasn't fired by Undo
6650 INFO TEST-PASS | /tests/editor/libeditor/html/tests/test_dom_input_event_on_htmleditor.html | Editor1, body has contenteditable attribute: input event by Undo wasn't trusted event
6651 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_dom_input_event_on_htmleditor.html | Editor1, body has contenteditable attribute: input event wasn't fired by Redo
6652 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/html/tests/test_dom_input_event_on_htmleditor.html | uncaught exception - TypeError: inputEvent is null at http://mochi.test:8888/tests/editor/libeditor/html/tests/test_dom_input_event_on_htmleditor.html:148
6653 INFO TEST-END | /tests/editor/libeditor/html/tests/test_dom_input_event_on_htmleditor.html | finished in 1345ms

10606 INFO TEST-PASS | /tests/editor/libeditor/text/tests/test_dom_input_event_on_texteditor.html | perhaps, timestamp wasn't set correctly :Mon Feb 25 10:05:48 2013
10607 INFO TEST-PASS | /tests/editor/libeditor/text/tests/test_dom_input_event_on_texteditor.html | <input type="text">: Accel+Z key didn't undo the value
10608 INFO TEST-PASS | /tests/editor/libeditor/text/tests/test_dom_input_event_on_texteditor.html | <input type="text">: input event wasn't fired by Undo
10609 INFO TEST-PASS | /tests/editor/libeditor/text/tests/test_dom_input_event_on_texteditor.html | <input type="text">: input event by Undo wasn't trusted event
10610 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/text/tests/test_dom_input_event_on_texteditor.html | <input type="text">: Accel+Y key didn't redo the value - got  , expected 
10611 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/text/tests/test_dom_input_event_on_texteditor.html | <input type="text">: input event wasn't fired by Redo
10612 ERROR TEST-UNEXPECTED-FAIL | /tests/editor/libeditor/text/tests/test_dom_input_event_on_texteditor.html | uncaught exception - TypeError: inputEvent is null at http://mochi.test:8888/tests/editor/libeditor/text/tests/test_dom_input_event_on_texteditor.html:119
10613 INFO TEST-END | /tests/editor/libeditor/text/tests/test_dom_input_event_on_texteditor.html | finished in 839ms

Comment 16

[(inactive) Jim Chen [:jchen] [:darchons]]

(In reply to Ryan VanderMeulen [:RyanVM] from comment #15)
> /tests/editor/libeditor/text/tests/test_dom_input_event_on_texteditor.html |
> <input type="text">: Accel+Y key didn't redo the value - got  , expected 

Looks like the test uses Accel+Y for redo, which wouldn't work on Android with the new bindings.

Masayuki-san: can test_dom_input_event_on_texteditor.html and test_dom_input_event_on_htmleditor.html use Shift+Accel+Z for redo for all platforms? Shift+Accel+Z works on all platforms [1], but Accel+Y only works on some platforms [2]. If we change to Shift+Accel+Z for all platforms, we don't have to detect the platform, and I don't think the change will impact the test's functionality.

[1] http://mxr.mozilla.org/mozilla-central/source/content/xbl/builtin/input-fields-base.inc#12
[2] http://mxr.mozilla.org/mozilla-central/source/content/xbl/builtin/win/platformHTMLBindings.xml#35

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Sounds okay. Sorry for the delay.

Comment 18

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Change tests to use Shift+Accel+Z for redo (v1)

Comment 19

[(inactive) Jim Chen [:jchen] [:darchons]]

Attachment: Add Android XBL key bindings (v5)

Patch bitrotted due to the Makefile DIRS refactor

Comment 20

[[e-mail]]

Comment on attachment 720661 [details] [diff] [review]
Add Android XBL key bindings (v5)

The interdiff looked reasonable on first sight to Ms2ger :-)

Comment 21

[(inactive) Jim Chen [:jchen] [:darchons]]

Try: https://tbpl.mozilla.org/?tree=Try&rev=725082226a3e

https://hg.mozilla.org/integration/mozilla-inbound/rev/b224ddd1e13d
https://hg.mozilla.org/integration/mozilla-inbound/rev/e019954a133c
https://hg.mozilla.org/integration/mozilla-inbound/rev/c124fcfa7414

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b224ddd1e13d
https://hg.mozilla.org/mozilla-central/rev/e019954a133c
https://hg.mozilla.org/mozilla-central/rev/c124fcfa7414

Comment 23

[Teodora Vermesan (:TeoVermesan)]

Test Cases added in Full Functional Tests Suite:

https://moztrap.mozilla.org/manage/case/7406/
https://moztrap.mozilla.org/manage/case/7407/