Closed Bug 1234459 Opened 6 years ago Closed 6 years ago

Expose full text in the input box to the app

Categories

(Core :: DOM: Device Interfaces, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: timdream, Assigned: timdream)

References

Details

Attachments

(1 file, 3 obsolete files)

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.
Attached patch Patch v1 (obsolete) — Splinter Review
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
Attachment #8700943 - Flags: superreview?(bugs)
Attachment #8700943 - Flags: review?(masayuki)
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|.
Flags: needinfo?(timdream)
(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.
Flags: needinfo?(timdream)
> 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
Attachment #8700943 - Flags: superreview?(bugs) → superreview+
Would be still good to test performance when editing some wiki page or so.
Attached patch Patch v1.1 (obsolete) — Splinter Review
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
Attachment #8700943 - Attachment is obsolete: true
Attachment #8700943 - Flags: review?(masayuki)
Attachment #8702461 - Flags: superreview?(bugs)
Attachment #8702461 - Flags: review?(masayuki)
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.
Attachment #8702461 - Flags: review?(masayuki) → review+
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)|)
Attached patch Patch v1.2 (obsolete) — Splinter Review
Updated according to comment above and update some comments in WebIDL.
Attachment #8702461 - Attachment is obsolete: true
Attachment #8702461 - Flags: superreview?(bugs)
Attachment #8702825 - Flags: superreview?(bugs)
Attachment #8702825 - Flags: review+
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.
Attachment #8702825 - Flags: superreview?(bugs) → superreview+
Attached patch Patch to commitSplinter Review
Attachment #8702825 - Attachment is obsolete: true
Attachment #8704017 - Flags: superreview+
Attachment #8704017 - Flags: review+
https://hg.mozilla.org/mozilla-central/rev/165ea6060598
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.