Closed Bug 831144 Opened 7 years ago Closed 7 years ago

Implement editor key bindings on Android

Categories

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

ARM
Android
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 22

People

(Reporter: jchen, Assigned: jchen)

References

Details

Attachments

(3 files, 4 obsolete files)

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
Duplicate of this bug: 728678
I think this is worth fixing now with quite a few people requesting this feature.
Assignee: nobody → nchen
Status: NEW → ASSIGNED
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
Attachment #717307 - Flags: review?(neil)
These changes make us able to pass combinations such as Ctrl+A to Gecko.
Attachment #717309 - Flags: review?(cpeterson)
Depends on: 844290
(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 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?
Thanks for the quick response Neil!

(In reply to neil@parkwaycc.co.uk 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 neil@parkwaycc.co.uk 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
Attachment #717307 - Attachment is obsolete: true
Attachment #717307 - Flags: review?(neil)
Attachment #717331 - Flags: review?(neil)
(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 on attachment 717309 [details] [diff] [review]
Properly pass meta states to Gecko (v1)

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

LGTM!
Attachment #717309 - Flags: review?(cpeterson) → review+
(In reply to neil@parkwaycc.co.uk 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
Attachment #717331 - Attachment is obsolete: true
Attachment #717331 - Flags: review?(neil)
Attachment #717395 - Flags: review?(neil)
(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 :-(
(In reply to neil@parkwaycc.co.uk 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! :)
Attachment #717395 - Attachment is obsolete: true
Attachment #717395 - Flags: review?(neil)
Attachment #717424 - Flags: review?(neil)
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.
Attachment #717424 - Flags: review?(neil) → review+
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
(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
Flags: needinfo?(masayuki)
Sounds okay. Sorry for the delay.
Flags: needinfo?(masayuki)
Patch bitrotted due to the Makefile DIRS refactor
Attachment #717424 - Attachment is obsolete: true
Attachment #720661 - Flags: review?(neil)
Comment on attachment 720661 [details] [diff] [review]
Add Android XBL key bindings (v5)

The interdiff looked reasonable on first sight to Ms2ger :-)
Attachment #720661 - Flags: review?(neil) → review+
Attachment #720656 - Flags: review?(masayuki) → review+
Flags: in-litmus?(fennec)
Test Cases added in Full Functional Tests Suite:

https://moztrap.mozilla.org/manage/case/7406/
https://moztrap.mozilla.org/manage/case/7407/
Flags: in-moztrap+
Flags: in-litmus?(fennec)
Flags: in-litmus-
You need to log in before you can comment on or make changes to this bug.