Rewrite selection and composition handling

RESOLVED FIXED in Firefox 45, Firefox OS v2.5

Status

()

Firefox for Android
Keyboards and IME
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jchen, Assigned: jchen)

Tracking

Trunk
Firefox 45
Points:
---

Firefox Tracking Flags

(firefox44 affected, firefox45 fixed, b2g-v2.5 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

2 years ago
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.
(Assignee)

Updated

2 years ago
Blocks: 1215163
(Assignee)

Comment 1

2 years ago
Created attachment 8681315 [details] [diff] [review]
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.
Attachment #8681315 - Flags: review?(esawin)
(Assignee)

Comment 2

2 years ago
Created attachment 8681316 [details] [diff] [review]
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.
Attachment #8681316 - Flags: review?(esawin)
(Assignee)

Comment 3

2 years ago
Created attachment 8681317 [details] [diff] [review]
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.
Attachment #8681317 - Flags: review?(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.
Attachment #8681315 - Flags: review?(esawin) → review+
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.
Attachment #8681316 - Flags: review?(esawin) → review+

Updated

2 years ago
Attachment #8681317 - Flags: review?(esawin) → review+
(Assignee)

Comment 6

2 years ago
bugherderlanding
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

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/746b7e8e0bf5
https://hg.mozilla.org/mozilla-central/rev/7f90ff1386f7
https://hg.mozilla.org/mozilla-central/rev/3993c6a8d6e6
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45

Comment 8

2 years ago
bugherderuplift
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
status-b2g-v2.5: --- → fixed
You need to log in before you can comment on or make changes to this bug.