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?

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.

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.

try https://tbpl.mozilla.org/?tree=Try&rev=6254d7f540c8

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?

Damn, I should have known about that API; I used it half a year ago already :p Anyway, code is muchhhhhh cleaner now.

(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.

(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.

(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.

Fixed nits

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.

Try https://tbpl.mozilla.org/?tree=Try&rev=51db49432ca3

What about the B2G desktop failures? https://tbpl.mozilla.org/php/getParsedLog.php?id=42342992&tree=Try

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?

Patch that does what I said above. Try @ https://tbpl.mozilla.org/?tree=Try&rev=d764a4f2bfbc

(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.

Try against latest mc tip: https://tbpl.mozilla.org/?tree=Try&rev=18af66566af7

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.

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.

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

Combined patch. Try @ https://tbpl.mozilla.org/?tree=Try&rev=62660fc26ca6

Backed out for causing: https://tbpl.mozilla.org/php/getParsedLog.php?id=43338234&tree=B2g-Inbound https://tbpl.mozilla.org/php/getParsedLog.php?id=43347344&tree=B2g-Inbound remote: https://hg.mozilla.org/integration/b2g-inbound/rev/85b5b33a4dc2

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?

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.

(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...

(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?

(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: Check comment 34.

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.

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.

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)

Masayuki, thank you for tracking this bug. Set keyword to ask for checkin. Please be aware that the author of the patch is janjongboom.

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.

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.