Closed Bug 1026997 Opened 7 years ago Closed 7 years ago

InputContext selectionchange event fires too late if going from selected text to unselected

Categories

(Firefox OS Graveyard :: Gaia::System::Input Mgmt, defect)

x86
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
2.1 S2 (15aug)

People

(Reporter: janjongboom, Assigned: janjongboom)

References

Details

Attachments

(1 file, 7 obsolete files)

First: attach the selectionchange listener:

navigator.mozInputMethod.addEventListener('inputcontextchange', function() {
  navigator.mozInputMethod.inputcontext.onselectionchange = function(ev) {
    console.log('onselectionchange', ev.target.selectionStart, ev.target.selectionEnd);
  }
});

Now click on the URL bar in the browser, the whole bar now gets selected. Now tap somewhere in the field to change the cursor position, say to pos. 11.

Nothing happens.

Now change position again, to say 9. You get 2 events.

"onselectionchange" 11 11
"onselectionchange" 9 9

So that's wrong.
Soooo, our caret handling is actually broken. F.e.

setSelectionRange call doesn't trigger an update (found out when writing mochitest for this stuff). But there is also no evens that we can listen to for this as it does not seem to exist in any current standard. Do you have any idea how we can improve this?
Flags: needinfo?(xyuan)
I mean calling setSelectionRange on an input field from content process, not on the inputcontext.
Oh the 'select' event does work, I was put off by it, but I checked the source and that should do it.
Flags: needinfo?(xyuan)
Attached patch bug1026997.patch (obsolete) — Splinter Review
Two fixes:

1. Listen for "select" event on input field as well and update selectioninfo. This way calls from content to setSelectionRange also generate selectionchange calls in InputMethod API
2. Wait a tick before getting the info in mouseup, because the selection didn't change by then yet.
Attachment #8442122 - Flags: review?(xyuan)
Comment on attachment 8442122 [details] [diff] [review]
bug1026997.patch

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

Jan, cool work! Just some concern about timeout. Could you try to make some enhancement if possible?

::: dom/inputmethod/forms.js
@@ +395,5 @@
>          // We only listen for this event on the currently focused element.
>          // When the mouse goes up, see if the cursor has moved (or the
>          // selection changed) since the mouse went down. If it has, we
>          // need to tell the keyboard about it
> +        content.setTimeout(function() {

The |setTimeout| is not reliable. We encountered more than once that a proper time for one device is not long enough for another device. This code used to work even without timer. So I hope to remove this timer if possible.

We have selectionListeners for gecko to monitor the selection changes. It is similar to 'select' event, but available for all kinds of editable elements beside the input element.

Could you try if we can use selectionListeners to avoid listening mouseup, keydown, input and similar events for updating the selection info.

selectionListeners could be added by nsISelectionPrivate::addSelectionListener [1]. We can get nsISelectionPrivate through QueryInterface on nsISelection of nsIEditor.

Refer to [2] for adding selectionListeners and [3] for getting selection change events.

[1] http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsISelectionPrivate.idl#50

[2]https://github.com/mozilla/gecko-dev/blob/MOBILE2901_2014050600_RELBRANCH/mobile/android/chrome/content/SelectionHandler.js#L267

[3]https://github.com/mozilla/gecko-dev/blob/MOBILE2901_2014050600_RELBRANCH/mobile/android/chrome/content/SelectionHandler.js#L213

::: dom/inputmethod/mochitest/test_bug1026997.html
@@ +36,5 @@
> +    let action = actions.pop();
> +    let start = action[0], end = action[1];
> +    input.setSelectionRange(start, end);
> +
> +    sendAsyncMessage('test:KeyBoard:setSelectionRange', {

Should we use sync message to ensure this message is sent before inputcontext.onselectionchange triggers?
Attachment #8442122 - Flags: review?(xyuan) → feedback+
Attached patch bug1026997_v3.patch (obsolete) — Splinter Review
Damn, I should have known about that API; I used it half a year ago already :p

Anyway, code is muchhhhhh cleaner now.
Attachment #8442122 - Attachment is obsolete: true
Attachment #8442794 - Flags: review?(xyuan)
(In reply to Yuan Xulei [:yxl] from comment #6)
> Should we use sync message to ensure this message is sent before
> inputcontext.onselectionchange triggers?
Mwa, could be, the selectionchange event has to go through Keyboard.jsm & MozKeyboard.js anyway, so I don't think it'll ever beat this call. Haven't seen it failing on local or on try either...

New try:  https://tbpl.mozilla.org/?tree=Try&rev=9014f7a3a5e2
(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #8)
> (In reply to Yuan Xulei [:yxl] from comment #6)
> > Should we use sync message to ensure this message is sent before
> > inputcontext.onselectionchange triggers?
> Mwa, could be, the selectionchange event has to go through Keyboard.jsm &
> MozKeyboard.js anyway, so I don't think it'll ever beat this call. Haven't
> seen it failing on local or on try either...
The test machine may be quite slow and causes intermittent test failure.
We got piles of such failures for the keyboard api, so I hope not to get more :-).
Comment on attachment 8442794 [details] [diff] [review]
bug1026997_v3.patch

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

I love this patch. Just a tiny fix with setTimeout needed!

::: dom/inputmethod/forms.js
@@ +282,5 @@
>          // element.
>          this._editor.addEditorObserver(this);
>        }
>  
> +      let selection;

Cannot we get selection directly from |this._editor.selection|?

@@ +287,5 @@
> +      if (element instanceof Ci.nsIDOMNSEditableElement) {
> +        selection =
> +          element.QueryInterface(Ci.nsIDOMNSEditableElement).editor.selection;
> +      }
> +      else {

nit: } else {

@@ +318,5 @@
>      this.focusedElement = element;
>    },
>  
> +  notifySelectionChanged:
> +        function fa_notifySelectionChanged(aDocument, aSelection, aReason) {

We don't need to name function in gecko any more :-) and we use this style for chrome code now:

notifySelectionChanged: function(...) {
}

@@ +743,5 @@
>      let selectionInfo = this.getSelectionInfo();
>      if (selectionInfo.changed) {
> +      // A call to setSelectionRange on input field causes 2 selection changes
> +      // one to [0,0] and one to actual value. Both are sent in same tick.
> +      // Prevent firing two events in that scenario, always only use the last 1

I got the similar problem with the edit action events. We want to ignore extra event caused by forwarding keyboard api calling.

Maybe we could handle this with the same way:

When a call to setSelectionRange, this._editing should be true. We don't update selection if detecting this._editing == true. After we finish calling setSelectionRange, manually update selection.

This way could also solve the similar issue caused by ReplaceSurroundingText.
Attachment #8442794 - Flags: review?(xyuan) → feedback+
(In reply to Yuan Xulei [:yxl] from comment #10)
> 
> When a call to setSelectionRange, this._editing should be true. We don't
> update selection if detecting this._editing == true. After we finish calling
> setSelectionRange, manually update selection.
> 
> This way could also solve the similar issue caused by ReplaceSurroundingText.

This is a call from *content*, not via the API. Content process does:

document.querySelector('#some-input').setSelectionRange(3, 4);

This API https://developer.mozilla.org/en-US/docs/Web/API/HTMLInputElement.setSelectionRange

So we don't have any influence on that unless we change the way that works.
Flags: needinfo?(xyuan)
(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #11)
> (In reply to Yuan Xulei [:yxl] from comment #10)
> > 
> > When a call to setSelectionRange, this._editing should be true. We don't
> > update selection if detecting this._editing == true. After we finish calling
> > setSelectionRange, manually update selection.
> > 
> > This way could also solve the similar issue caused by ReplaceSurroundingText.
> 
> This is a call from *content*, not via the API. Content process does:
> 
> document.querySelector('#some-input').setSelectionRange(3, 4);
> 
> This API
> https://developer.mozilla.org/en-US/docs/Web/API/HTMLInputElement.
> setSelectionRange
> 
> So we don't have any influence on that unless we change the way that works.

For this case, we have to use setTimeout. Thanks for the clarification.
Flags: needinfo?(xyuan)
Attached patch bug1026997_v4.patch (obsolete) — Splinter Review
Fixed nits
Attachment #8442794 - Attachment is obsolete: true
Attachment #8444391 - Flags: review?(xyuan)
Comment on attachment 8444391 [details] [diff] [review]
bug1026997_v4.patch

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

r=me if you fix the code below.

::: dom/inputmethod/forms.js
@@ +286,5 @@
> +      let selection = this._editor.selection;
> +      if (selection) {
> +        this._selectionPrivate = selection.QueryInterface(Ci.nsISelectionPrivate);
> +        this._selectionPrivate.addSelectionListener(this);
> +      }

Move this code block inside the above |if (this._editor) ... | block.
Attachment #8444391 - Flags: review?(xyuan) → review+
Attached patch bug1026997_v5.patch (obsolete) — Splinter Review
Attachment #8444391 - Attachment is obsolete: true
Attachment #8445020 - Flags: review+
Hey, maybe you can help here. I think we have potential race condition based on test failure.

I call a function from the inputmethod API like setSelectionRange. This goes like:

MozKeyboard.js -> Keyboard.jsm -> forms.js -> change selection range -> SetSelectionRange:OK -> promise resolves

But there is a second track:

MozKeyboard.js -> Keyboard.jsm -> forms.js -> change selection range -> selectionchange event in forms.js -> timeout -> Forms:SelectionChange -> onselectionchange event

Now if the first track beats the second track, we have weird behavior:

inputcontext.setSelectionRange(2, 0).then(function() {
  inputcontext.selectionStart; // might be 0 or 2 depending on who wins
});

We can fix it by updating the context when SetSelectionRange:OK comes in, but then we have the same problem with SendKey f.e.

I think we should add the selection info with every message we send from forms.js upstream. What do you think?
Flags: needinfo?(xyuan)
Attached patch bug1026997_v6.patch (obsolete) — Splinter Review
Patch that does what I said above.

Try @ https://tbpl.mozilla.org/?tree=Try&rev=d764a4f2bfbc
Attachment #8445020 - Attachment is obsolete: true
Attachment #8445755 - Flags: review?(xyuan)
(In reply to Jan Jongboom [:janjongboom] (Telenor) from comment #18)
> Hey, maybe you can help here. I think we have potential race condition based
> on test failure.
> 
> I call a function from the inputmethod API like setSelectionRange. This goes
> like:
> 
> MozKeyboard.js -> Keyboard.jsm -> forms.js -> change selection range ->
> SetSelectionRange:OK -> promise resolves
> 
> But there is a second track:
> 
> MozKeyboard.js -> Keyboard.jsm -> forms.js -> change selection range ->
> selectionchange event in forms.js -> timeout -> Forms:SelectionChange ->
> onselectionchange event
> 
> Now if the first track beats the second track, we have weird behavior:
> 
> inputcontext.setSelectionRange(2, 0).then(function() {
>   inputcontext.selectionStart; // might be 0 or 2 depending on who wins
> });
> 
> We can fix it by updating the context when SetSelectionRange:OK comes in,
> but then we have the same problem with SendKey f.e.
Can't we fix SendKey and other methods with SetSelectionRange:OK way?
> 
> I think we should add the selection info with every message we send from
> forms.js upstream. What do you think?
We should always keep selectionStart and selectionEnd updated. Does it fix this issue?
Yeah, I think my patch fixes the race condition, let's see what Try says.
BTW, keep in mind that all dom/inputmethod tests are currently (if by "currently" you mean 3+ months and counting...) skipped in the emulator due to the high intermittent failure rate. Otherwise you might have caught the failure there as well.
Comment on attachment 8445755 [details] [diff] [review]
bug1026997_v6.patch

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

r=me except a few changes of the mochitest test file are needed. I'll upload a new patch to show the changes.

::: dom/inputmethod/MozKeyboard.js
@@ +383,5 @@
>  
>      let json = msg.json;
>      let resolver = this.takePromiseResolver(json.requestId);
>  
> +    dump('[MozKeyboard] receiveMessage ' + msg.name + ' has selectioninfo ' +

Remove this log for debug purpose.
Attachment #8445755 - Flags: review?(xyuan) → review+
Attached patch fix mochitest (obsolete) — Splinter Review
To avoid potential race condition issue, I removed the message sent from the frame script in file_test_app.html.

Please revise and merge this patch into yours before checking in.
Attachment #8447719 - Flags: review?(janjongboom)
Flags: needinfo?(xyuan)
Thanks, I'll review asap. Am in Dhaka right now, so need to get some spare time :-)
Comment on attachment 8447719 [details] [diff] [review]
fix mochitest

LGTM
Attachment #8447719 - Flags: review?(janjongboom) → review+
Attached patch bug1026997_v7.patch (obsolete) — Splinter Review
Combined patch. Try @ https://tbpl.mozilla.org/?tree=Try&rev=62660fc26ca6
Attachment #8445755 - Attachment is obsolete: true
Attachment #8447719 - Attachment is obsolete: true
Attachment #8452223 - Flags: review+
Flags: needinfo?(janjongboom)
Hmm, looks like that the test failures are not directly caused by the patch.

1. Load data:text/html,<textarea id="target" style="height: 100px; -moz-appearance: none" spellcheck="false" onkeydown="this.style.display='block';this.style.height='200px';">foo</textarea>
2. Click the editor and move caret to the end of "foo".
3. Press Enter key.

Then, the caret is not moved and line breaker is not inserted. But the reference expects a line breaker is inserted. I'm not sure why the tests don't cause orange with current m-c...

Ehsan, any ideas?
Flags: needinfo?(ehsan)
Oh, I realized that it's reproduced with TSF mode only when I test it on Windows. Not reproduced with IMM mode. One of the differences of the two modes is if IMEContentObserver listens selection change with nsISelectionListener. I guess that when there is a listener of selection change, the tests are failed. Although, I don't understand the reason why it doesn't cause orange of Windows build on tinderbox of m-c yet.
It's odd... On Mac, IMEContentObserver listens selection change with nsISelectionListener but I cannot reproduce the bug mentioned in comment 33...
Ah, I see how to reproduce the bug.

If nsISelectionListener::NotifySelectionChanged() accesses selectionStart or selectionEnd, the bug is reproducible. So, with normal m-c build, this is not reproducible. It is the reason why Windows build of m-c doesn't cause orange in tinderbox.

So, Jan, I guess that if it's possible do call this.updateSelection(); from notifySelectionChanged() with setTimeout(), the orange should be fixed. However, I'm not sure if it's possible.

If it's impossible, you can use nsIEditorObserver::EditAction() (and nsIEditorObserver::CancelEditAction()) for being delay to call it. When nsIEditorObserver::BeforeEditAction() is called, set a flag true. And EditAction() and CancelEditAction() should call updateSelection() if it's put off.

Ehsan, do you have idea the reason why accessing selectionStart and selectionEnd from nsISelectionListener::NotifySelectionChanged() causes such regression? There must be a hidden bug.
The test case in comment 31 seems to suggest that this happens because accessing selectionStart/End can flush.
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #35)
> The test case in comment 31 seems to suggest that this happens because
> accessing selectionStart/End can flush.

Yeah, probably, true. However, why flushing layout causes cancelling an edit action? nsISelectionListener::NotifySelectionChanged() should be called after ns*Editor modifies its DOM tree because of the batch...
Flags: needinfo?(ehsan)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #36)
> (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> comment #35)
> > The test case in comment 31 seems to suggest that this happens because
> > accessing selectionStart/End can flush.
> 
> Yeah, probably, true. However, why flushing layout causes cancelling an edit
> action?

Well, it's a bug.  :-)  I can't reproduce using the test case in comment 31 (on Mac at least) so I can't tell exactly what's happening here.  Do you have a backtrace which shows where the editing operation gets cancelled?  It's hard to say what's happening here without knowing more details.

But generally speaking, when the frame for a text box is destroyed, we detach its editor from the DOM node (since the editor needs to operate on the native anonymous content nodes managed by the frame) and we reattach it when the frame gets recreated.  Many things can go wrong in this process, resulting in bugs like this.

> nsISelectionListener::NotifySelectionChanged() should be called
> after ns*Editor modifies its DOM tree because of the batch...

Why do you think that?  We sometimes modify the selection as part of the editing operation.  Do you have a backtrace to the call to NotifySelectionChanged where this bug happens?
Flags: needinfo?(ehsan)
(In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from comment #37)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #36)
> > (In reply to :Ehsan Akhgari (not reading bugmail, needinfo? me!) from
> > comment #35)
> > > The test case in comment 31 seems to suggest that this happens because
> > > accessing selectionStart/End can flush.
> > 
> > Yeah, probably, true. However, why flushing layout causes cancelling an edit
> > action?
> 
> Well, it's a bug.  :-)  I can't reproduce using the test case in comment 31
> (on Mac at least) so I can't tell exactly what's happening here.  Do you
> have a backtrace which shows where the editing operation gets cancelled? 
> It's hard to say what's happening here without knowing more details.

I'm still looking for where cancels edit action...

> But generally speaking, when the frame for a text box is destroyed, we
> detach its editor from the DOM node (since the editor needs to operate on
> the native anonymous content nodes managed by the frame) and we reattach it
> when the frame gets recreated.  Many things can go wrong in this process,
> resulting in bugs like this.

Sure.

> > nsISelectionListener::NotifySelectionChanged() should be called
> > after ns*Editor modifies its DOM tree because of the batch...
> 
> Why do you think that?  We sometimes modify the selection as part of the
> editing operation.  Do you have a backtrace to the call to
> NotifySelectionChanged where this bug happens?

Because I guess that cancelling edit actions from nsISelectionListener::NotifySelectionChanged() means that it's called before the editor becomes "safe" (in other words, "stable").

I'll try to get the trace when I have time, thanks!
What blocks this from landing? Links in comment 30 have since expired :'(.
Some mochitests fail due to this patch. I pushed the patch try again for you:
https://tbpl.mozilla.org/?tree=Try&rev=c8661cd11915
Yuan:

See comment 34.

By the bug 1052343, I may not be able to research about the orange. So, if the patch can take setTimeout() hack, please use it for now.
Flags: needinfo?(xyuan)
I filed bug 1053048 for the cause of the orange.
Masayuki, thanks.
I made a fix per comment 34. The try tree is closed and I will check it on try server when the tree is open.
Flags: needinfo?(xyuan)
Flags: needinfo?(janjongboom)
Thanks Yuan. I missed the initial back out.
(In reply to Yuan Xulei [:yxl] from comment #44)
> Created attachment 8472203 [details] [diff] [review]
> Use setTimeout to work around bug 1052343
> 
> Masayuki, thanks.
> I made a fix per comment 34. The try tree is closed and I will check it on
> try server when the tree is open.

https://tbpl.mozilla.org/?tree=Try&rev=8fda3f59b06d
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8fda3f59b06d
(In reply to Yuan Xulei [:yxl] from comment #46)
> (In reply to Yuan Xulei [:yxl] from comment #44)
> > Created attachment 8472203 [details] [diff] [review]
> > Use setTimeout to work around bug 1052343
> > 
> > Masayuki, thanks.
> > I made a fix per comment 34. The try tree is closed and I will check it on
> > try server when the tree is open.
> 
> https://tbpl.mozilla.org/?tree=Try&rev=8fda3f59b06d
> https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=8fda3f59b06d

Looks fine. (Mulet Linux x64 Opt's orange is permanent, they are disabled in m-c)
Attachment #8452223 - Attachment is obsolete: true
Masayuki, thank you for tracking this bug.

Set keyword to ask for checkin. Please be aware that the author of the patch is janjongboom.
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/b615a6a84a9c
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → 2.1 S2 (15aug)
Jan and  Yuan, could you request approval‑mozilla‑b2g32?  3rd party IME developer and a vendor need this fix for Firefox OS 2.0.
Comment on attachment 8472203 [details] [diff] [review]
Use setTimeout to work around bug 1052343

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Bug 1026997
User impact if declined:  InputContext selectionchange event is broken.
Testing completed: Yes, https://tbpl.mozilla.org/?tree=Try&rev=8fda3f59b06d
Risk to taking this patch (and alternatives if risky): Low. The patch only impacts the way to generate selectionchange event of the keyboard API, which 
hasn't been used by the gaia keyboard app yet. So the risk to break gaia is low.
String or UUID changes made by this patch: None.
Attachment #8472203 - Flags: approval-mozilla-b2g32?
Is this a regression in 2.0 ? We are typically allowing only 2.0 blockers at this point(2.0+) unless very low risk/high reward. So if you think this should be blocking please confirm the regression question and also nominate for 2.0 ?
(In reply to bhavana bajaj [:bajaj] from comment #53)
> Is this a regression in 2.0 ? We are typically allowing only 2.0 blockers at
> this point(2.0+) unless very low risk/high reward. So if you think this
> should be blocking please confirm the regression question and also nominate
> for 2.0 ?

No a regression in 2.0. The risk is low but not really high reward I think. Due to our limited resources,
I have to cancel approval‑mozilla‑b2g32 request.
Attachment #8472203 - Flags: approval-mozilla-b2g32?
Blocks: 1105678
You need to log in before you can comment on or make changes to this bug.