1234459 - Expose full text in the input box to the app

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Right now we have textBeforeCursor and textAfterCursor, with design to expose the 100 characters before/after the cursor. Not only it did not contain the text selected, it force the keyboard app to issue getText() to get the full text.

The original intention was to save some memory, but in forms.js we already getting the full text anyway so it doesn't make sense to get it and only to discard it.

It's a dependency to bug 1228778 because that bug will be non-trivial w/o this fix.

Comment 1

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch v1

This patch exposes the text of the input (which we have already send to the remote process always) through MozInputContext#text property. It also remove the GetText calls and have MozInputContext#getText resolves locally w/o IPC messages.

Tests have passed locally.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=91f46c0533f3

Comment 2

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

Comment on attachment 8700943 [details] [diff] [review]
Patch v1

>diff --git a/dom/inputmethod/MozKeyboard.js b/dom/inputmethod/MozKeyboard.js
>--- a/dom/inputmethod/MozKeyboard.js
>+++ b/dom/inputmethod/MozKeyboard.js
>@@ -747,16 +747,17 @@ InputContextDOMRequestIpcHelper.prototyp
> function MozInputContext(ctx) {
>   this._context = {
>     type: ctx.type,
>     inputType: ctx.inputType,
>     inputMode: ctx.inputMode,
>     lang: ctx.lang,
>     selectionStart: ctx.selectionStart,
>     selectionEnd: ctx.selectionEnd,
>+    text: ctx.value,
>     textBeforeCursor: ctx.textBeforeCursor,
>     textAfterCursor: ctx.textAfterCursor

I'm not sure the life time of this object, though. If the focused editor is an editor of wiki or something, the value becomes much big. Is it okay?

And cannot you to remove textBeforeCursor and textAfterCursor? Because you store whole text with |text| and the cursor position with |selectionStart| and/or |selectionEnd|.

Comment 3

[Tim Guan-tin Chien [:timdream] (please needinfo)]

(In reply to Masayuki Nakano [:masayuki] (Mozilla Japan) from comment #2)
> Comment on attachment 8700943 [details] [diff] [review]
> Patch v1
> 
> >diff --git a/dom/inputmethod/MozKeyboard.js b/dom/inputmethod/MozKeyboard.js
> >--- a/dom/inputmethod/MozKeyboard.js
> >+++ b/dom/inputmethod/MozKeyboard.js
> >@@ -747,16 +747,17 @@ InputContextDOMRequestIpcHelper.prototyp
> > function MozInputContext(ctx) {
> >   this._context = {
> >     type: ctx.type,
> >     inputType: ctx.inputType,
> >     inputMode: ctx.inputMode,
> >     lang: ctx.lang,
> >     selectionStart: ctx.selectionStart,
> >     selectionEnd: ctx.selectionEnd,
> >+    text: ctx.value,
> >     textBeforeCursor: ctx.textBeforeCursor,
> >     textAfterCursor: ctx.textAfterCursor
> 
> I'm not sure the life time of this object, though. If the focused editor is
> an editor of wiki or something, the value becomes much big. Is it okay?
> 

The life time of the string in the content scope, for a properly implemented keyboard, should be the lifetime of the inputContext. It should not hold a copy of the strong any longer than that.

The life time of MozInputContext in MozKeyboard.js is the duration of the input being focused.

I admit my argument on we should expose it is pretty weak:

1) We already calculate that even for contenteditable in 
https://dxr.mozilla.org/mozilla-central/source/dom/inputmethod/forms.js#1274

2) We already sending the entire text across the process boundary from
https://dxr.mozilla.org/mozilla-central/source/dom/inputmethod/forms.js#1170

So if there is any performance issue we should have it already :'(.

If we want to improve the situation, we would need to change (1) and (2) by never getting/sending the whole string across process boundary unless it's something needed for value selector. For the keyboard use case, we would need to implement MozInputContext#textSelected and also make sure we calculate these values from the originating end (forms.js). We would also need non-trival keyboard app changes for it to properly update it's internal states.

Given the state of B2G project, I am not ready to commit to such task. Bug 1228778 would need to wait if we really need to do what proposed above.

> And cannot you to remove textBeforeCursor and textAfterCursor? Because you
> store whole text with |text| and the cursor position with |selectionStart|
> and/or |selectionEnd|.

I am keeping textBeforeCursor/textAfterCursor for compatibility. I can remove it if you think so.

Comment 4

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

> So if there is any performance issue we should have it already :'(.

Okay, sounds good.

>> And cannot you to remove textBeforeCursor and textAfterCursor? Because you
>> store whole text with |text| and the cursor position with |selectionStart|
>> and/or |selectionEnd|.
> 
> I am keeping textBeforeCursor/textAfterCursor for compatibility. I can remove it if you think so.

I don't understand why you don't do that.  It's better to compute them from |text| at accessing MozInputContext#textBeforeCursor and MozInputContext#textAfterCursor. I.e., here:
https://bugzilla.mozilla.org/attachment.cgi?id=8700943&action=diff#a/dom/inputmethod/MozKeyboard.js_sec4

Comment 5

[Olli Pettay [:smaug]]

Would be still good to test performance when editing some wiki page or so.

Comment 6

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch v1.1

Patch updated: removing textBeforeCursor and textAfterCursor calculation in forms.js and move it to a getter in MozKeyboard.js. Create MozInputContextSelectionChangeEventDetail and MozInputContextSurroundingTextChangeEventDetail so I can put getter on them (and have them always getting the values from the MozInputContext).

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6823e5b9f9ec

Comment 7

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

Comment on attachment 8702461 [details] [diff] [review]
Patch v1.1

Looks good to me, although, I'd be happy if you separated the latest change to another patch. But it's okay.

Comment 8

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

Oops, I forgot to say this:

>    get textAfterCursor() {
> -    return this._context.textAfterCursor;
> +    let text = this._context.text;
> +    let start = this._context.selectionStart;
> +    let end = this._context.selectionEnd;
> +    return (end + 100 > text.length) ?
> +      text.substr(start, text.length) :
> +      text.substr(start, end - start + 100);
>    },

Isn't this same as

get textAfterCursor() {
  return text.substr(start, end - start + 100);
},

? If length is larger, the result is automatically shorter string then the length value. (like |text.substr(start, text.length)|)

Comment 9

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch v1.2

Updated according to comment above and update some comments in WebIDL.

Comment 10

[Olli Pettay [:smaug]]

Comment on attachment 8702825 [details] [diff] [review]
Patch v1.2

Could you add some comment to those new interfaces why they are actually interfaces and not dictionaries.

Comment 11

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Attachment: Patch to commit

Comment 12

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/165ea6060598

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/165ea6060598