Closed
Bug 1234459
Opened 10 years ago
Closed 10 years ago
Expose full text in the input box to the app
Categories
(Core :: DOM: Device Interfaces, defect)
Core
DOM: Device Interfaces
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: timdream, Assigned: timdream)
References
Details
Attachments
(1 file, 3 obsolete files)
28.35 KB,
patch
|
timdream
:
review+
timdream
:
superreview+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
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 2•10 years ago
|
||
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)
Assignee | ||
Comment 3•10 years ago
|
||
(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)
Comment 4•10 years ago
|
||
> 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
Updated•10 years ago
|
Attachment #8700943 -
Flags: superreview?(bugs) → superreview+
Comment 5•10 years ago
|
||
Would be still good to test performance when editing some wiki page or so.
Assignee | ||
Comment 6•10 years ago
|
||
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 7•10 years ago
|
||
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+
Comment 8•10 years ago
|
||
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)|)
Assignee | ||
Comment 9•10 years ago
|
||
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 10•10 years ago
|
||
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+
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8702825 -
Attachment is obsolete: true
Attachment #8704017 -
Flags: superreview+
Attachment #8704017 -
Flags: review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 12•10 years ago
|
||
Keywords: checkin-needed
Comment 13•10 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in
before you can comment on or make changes to this bug.
Description
•