1216666 - Rewrite selection and composition handling

Status: RESOLVED FIXED

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

Description

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

Right now, the Gecko side tries to mirror the selection on the Java side. To make IME code more robust, we should let the Java side mirror the Gecko side, similar to what we already do for text changes.

Also, right now the Java side fires off additional "update composition" events periodically to update the composition. To make it more efficient and to make the IME code work better with JS code associated with the input field, we should merge these update composition events into events like text change and span change.

Comment 1

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

Attachment: Separate out Gecko selection from Java selection (v1)

This simplifies the selection handling code in GeckoEditable a lot.

Currently, the Gecko-side selection tries to mirror the Java-side
selection, but this is difficult because the Gecko selection has some
subtle but important behavior differences from the Java selection.
This patch separates out Gecko selection from Java selection so that
they're only loosely coupled. The two selections will periodically
synchronize through events and notifications, but at certain times they
may fall out-of-sync, for example when a composition is active. This
shouldn't affect functionality in a major way, and it's an acceptable
trade-off.

Comment 2

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

Attachment: Update composition as part of replacing text or changing span (v1)

Currently, GeckoEditable periodically fires update composition events to
update the Gecko composition styling. To make the code more efficient
and more robust in dealing with content JS code, this patch merges these
events into events like replacing text, setting span, and removing span.
As a result, a setComposingText call now results in one replacing text
event instead of a replacing text event plus an update composition event.

Comment 3

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

Attachment: Only send key events for single-character strings (v1)

GeckoEditable currently synthesizes key events when committing strings,
to improve web compatibility. However, synthesizing keys is causing
performance problems when the string is long. This patch limits key
synthesis to single-character strings. For longer strings, we commit the
string as a whole. We have other ways of ensuring web compatibility now
(sending dummy key events), so this restriction should not cause
regressions.

Comment 4

[Eugen Sawin [:esawin]]

Comment on attachment 8681315 [details] [diff] [review]
Separate out Gecko selection from Java selection (v1)

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

This looks good, nice code reduction, too.

Comment 5

[Eugen Sawin [:esawin]]

Comment on attachment 8681316 [details] [diff] [review]
Update composition as part of replacing text or changing span (v1)

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

Batching composition events sounds good, can we easily extend the tests to verify it's not messing things up or dropping events?

::: mobile/android/base/GeckoEditable.java
@@ +478,5 @@
> +     * @return Whether there was a composition
> +     */
> +    private boolean icMaybeSendComposition(final CharSequence sequence,
> +                                           final boolean useEntireText,
> +                                           final boolean notifyGecko) {

It might be nicer to use enums instead and avoid boolean arguments for more explicit "flags" handling.

Comment 6

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/746b7e8e0bf5
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f90ff1386f7
https://hg.mozilla.org/integration/mozilla-inbound/rev/3993c6a8d6e6

Comment 7

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/746b7e8e0bf5
https://hg.mozilla.org/mozilla-central/rev/7f90ff1386f7
https://hg.mozilla.org/mozilla-central/rev/3993c6a8d6e6

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/746b7e8e0bf5
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/7f90ff1386f7
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/3993c6a8d6e6