1057898 - If the user taps between two inputs, try to send one inputcontextchange event instead of two

Status: RESOLVED FIXED

(Core :: DOM: Device Interfaces, defect)

Description

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

STR:

0. Attach mozInputMethod.oninputcontextchange event listener
1. Go to Contact app -> New contact [+]
2. Focus on name field
3. Focus on last name field

Actual:

1. Got two events, one changes the inputcontext to null and the other sends the new context (on a Flame it separates around 6ms)

Expected:

1. Receiving one event and one only.

Note:

So, there are some regressions in v2.1 on layout switching internally in the keyboard app and I am going to address in bug 1054184. However that can only take us back to what v2.0 had offered. As long as there is a little "gap" in the timeline where the inputcontext is null, the user press can not be registered if the user happen to lift her/his finger at that time.

The easy way to solve this issue is to do what the summary described, but from probing the DOM focus/blur events (see URL) I can see that's foreign to platform. I don't know if we can fix this in forms.js without introducing a magic timeout. I also don't know if not sending the blur Forms:focus message will really keep the previous context valid. I am needinfo these questions to some forms.js hacker here.

The hard way to fix this is to do it in keyboard app where it will be responsible of queue the user touches until the layout is operable again, something is not possible until architecture improvements like bug 1040603 is done. If we want to go this route we can just WONTFIX this bug.

If we can keep the context valid for sometime even after the blur event, I think we can look into further applications like sending a "willblur/blur" event on the context itself so autocomplete can finish the last word, or for the Asian IME to push the first candidate (bug to be filed).

PS This InputMethod API bug is not a regression but an improvement, so please don't set any blocking-b2g flag.

Comment 1

[Yuan Xulei [:yxl] (Leave from May 18)]

It is a desirable behavior that switching between two inputs fires one inputcontextchange event and doesn't contradict the spec [1]. So I think we could fix this in input method API to avoid complicating keyboard app.

But just as Tim mentioned, we may introduce a timeout to detect if a blur-focus event sequence is caused by switching between two inputs.

An alternative to set timeout is to post thread runnable with XPCOM[2]. We need to get the main thread interface, dispatch and queue a runnable. This way should be more efficient than the timeout.

[1] https://wiki.mozilla.org/WebAPI/KeboardIME#Proposed_API
[2] https://github.com/mozilla/gecko-dev/blob/master/testing/xpcshell/head.js#L567

Comment 2

[Jan Jongboom [:janjongboom] (inactive)]

We already use the magic timeout in gaia system app to not pop down a keyboard when this happens, so if we could fix this (even with timeout) in Gecko level, that would also solve the problem for Gaia.

Comment 3

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

Take this bug... I have untested patch in my hard drive.

Comment 4

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

Attachment: Patch v1

Comment 5

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

Attachment: Test for Patch v1

Comment 6

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

Attachment: Patch v1

This is the correct patch.

Try server run (w/ patch):

https://tbpl.mozilla.org/?tree=Try&rev=7a67da8c219c

Comment 7

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

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=7a67da8c219c

Comment 8

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

Hum, test_delete_focused_element.html failed.

Comment 9

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

Attachment: Test for Patch v1 (v1.1)

It turned out the delete element test remove the element and focus on the next element on the same tick, which is exactly the feature we want to roll out in the patch here.

I ended up using 10ms timer instead of a 0ms one because it looks like a 0ms one will still happen keep the blur event from dispatching. setTimeout is magical!

https://tbpl.mozilla.org/?tree=Try&rev=a05fed1ffddc
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=a05fed1ffddc

Comment 10

[Yuan Xulei [:yxl] (Leave from May 18)]

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

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

::: dom/inputmethod/forms.js
@@ +302,5 @@
>            });
>          });
>          if (del && element === self.focusedElement) {
>            // item was deleted, fake a blur so all state gets set correctly
> +          self.hideKeyboard();

Why will you change this? This code is a workaround of bug 559561 for which a blur event should be fired when focused element is removed.

@@ +672,5 @@
> +
> +    // Wait for the next tick before unset the focused element and etc.
> +    // If the user move from one input from another,
> +    // the remote process should get one Forms:Input message instead of two.
> +    this.waitForNextTick(function() {

Conventionally, we bind the callback function explicitly before passing it as argument.

Comment 11

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

(In reply to Yuan Xulei [:yxl] from comment #10)
> Comment on attachment 8479653 [details] [diff] [review]
> Patch v1
> 
> Review of attachment 8479653 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: dom/inputmethod/forms.js
> @@ +302,5 @@
> >            });
> >          });
> >          if (del && element === self.focusedElement) {
> >            // item was deleted, fake a blur so all state gets set correctly
> > +          self.hideKeyboard();
> 
> Why will you change this? This code is a workaround of bug 559561 for which
> a blur event should be fired when focused element is removed.
> 

hideKeyboard() will send out the blur event; I want to centralize the call here. Do you think we should always end a blur event in this case (and I should revert the change in delete element test case?)

Comment 12

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 8479704 [details] [diff] [review]
Test for Patch v1 (v1.1)

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

::: dom/inputmethod/mochitest/test_delete_focused_element.html
@@ +35,5 @@
>  
>    content.setTimeout(function() {
> +    content.setTimeout(function() {
> +      textarea.focus();
> +    }, 10);

I suggest you set a larger timeout, such as 100ms, or more to avoid intermittent failure on try server.

Comment 13

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

(In reply to Yuan Xulei [:yxl] from comment #12)
> >    content.setTimeout(function() {
> > +    content.setTimeout(function() {
> > +      textarea.focus();
> > +    }, 10);
> 
> I suggest you set a larger timeout, such as 100ms, or more to avoid
> intermittent failure on try server.

I tend to think we should not hide intermittent failure by longer timeouts but actually convert the test to a series of async ping-and-pong like the test I added (but not this bug).

Comment 14

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #11)
> (In reply to Yuan Xulei [:yxl] from comment #10)
> > Comment on attachment 8479653 [details] [diff] [review]
> > Patch v1
> > 
> > Review of attachment 8479653 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: dom/inputmethod/forms.js
> > @@ +302,5 @@
> > >            });
> > >          });
> > >          if (del && element === self.focusedElement) {
> > >            // item was deleted, fake a blur so all state gets set correctly
> > > +          self.hideKeyboard();
> > 
> > Why will you change this? This code is a workaround of bug 559561 for which
> > a blur event should be fired when focused element is removed.
> > 
> 
> hideKeyboard() will send out the blur event; I want to centralize the call
> here. Do you think we should always end a blur event in this case (and I
> should revert the change in delete element test case?)
hideKeyboard() in your patch send out a blur message:
 sendAsyncMessage("Forms:Input", { "type": "blur" })
but not a blur event. 
Do you confuse the event with the message?
The blur event, which is handled by FormAssistant.handleEvent, triggers hideKeyboard().
hideKeyboard() doesn't need to send out blur events.

Comment 15

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #13)
> (In reply to Yuan Xulei [:yxl] from comment #12)
> > >    content.setTimeout(function() {
> > > +    content.setTimeout(function() {
> > > +      textarea.focus();
> > > +    }, 10);
> > 
> > I suggest you set a larger timeout, such as 100ms, or more to avoid
> > intermittent failure on try server.
> 
> I tend to think we should not hide intermittent failure by longer timeouts
> but actually convert the test to a series of async ping-and-pong like the
> test I added (but not this bug).

That will be great if we can avoid using setTimeout.

Comment 16

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

(In reply to Yuan Xulei [:yxl] from comment #14)
> > hideKeyboard() will send out the blur event; I want to centralize the call
> > here. Do you think we should always end a blur event in this case (and I
> > should revert the change in delete element test case?)
> hideKeyboard() in your patch send out a blur message:
>  sendAsyncMessage("Forms:Input", { "type": "blur" })
> but not a blur event. 
> Do you confuse the event with the message?
> The blur event, which is handled by FormAssistant.handleEvent, triggers
> hideKeyboard().
> hideKeyboard() doesn't need to send out blur events.

I am sorry, I was meant to say "I want to centralized all blur message to hideKeyoard()", but you are right, I don't really need to change this bit because eventually handleEvent() will trigger hideKeyboard().

However, still think this change should be kept because it look strange to me for a singleton to call it's own handleEvent interface with a fake event; it should just call that method (in this case, hideKeyboard()) directly).

Comment 17

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

Attachment: Patch for commit, part 1

Comment 18

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

Attachment: Patch for commit, part 2

Comment 19

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

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=c6a7fa56f10d

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

Comment 20

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

There is one strange case on the phone I think we need to address in Gaia first.

1. focus on an input
2. press sleep to turn off the screen
3. press sleep to turn back on
4. swipe and unlock

we would get an unfocused keyboard in this case... not sure why. Will investigate tomorrow.

Comment 21

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

I am not sure if this should make it into 2.1 but maybe it shouldn't (out of preventing blockers from popping up)

Comment 22

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

/tests/dom/base/test/test_bug976673.html failed on comment 19.

Comment 23

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

Attachment: Patch v1.2

So to work with the problem on comment 20, I have to check if this.focusedElement is falsey. I don't know why it become |null| after that next tick when the screen is off. I am fairly certain nothing in forms.js have changed it to null (I have placed |dump| to all setters and none of them are called). I will file a follow-up bug to investigate that.

I've not yet figured out the reason why the test failed in comment 22. I am currently building emulator to figure out that :-/ (hence, no try push yet for this updated patch).

Comment 24

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #23)
> Created attachment 8480378 [details] [diff] [review]
> Patch v1.2
> 
> So to work with the problem on comment 20, I have to check if
> this.focusedElement is falsey. I don't know why it become |null| after that
> next tick when the screen is off. I am fairly certain nothing in forms.js
> have changed it to null (I have placed |dump| to all setters and none of
> them are called). I will file a follow-up bug to investigate that.
If the focusedElement is dead, we will get null value from the getter and without setting it to null:

 https://github.com/mozilla/gecko-dev/blob/7c9c4378dfc902996186c63f0d6e4f2d552cc335/dom/inputmethod/forms.js#L230

 ...
 get focusedElement() {
   if (this._focusedElement && Cu.isDeadWrapper(this._focusedElement))
     this._focusedElement = null;
   return this._focusedElement;
 },
 ...

Comment 25

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

(In reply to Yuan Xulei [:yxl] from comment #24)
> If the focusedElement is dead, we will get null value from the getter and
> without setting it to null:
> 
>  https://github.com/mozilla/gecko-dev/blob/
> 7c9c4378dfc902996186c63f0d6e4f2d552cc335/dom/inputmethod/forms.js#L230
> 
>  ...
>  get focusedElement() {
>    if (this._focusedElement && Cu.isDeadWrapper(this._focusedElement))
>      this._focusedElement = null;
>    return this._focusedElement;
>  },
>  ...

That's exactly one of the places I had put the |dump()| and I am pretty sure we did not run into that |if| block. That's why I said I haven't found the reason yet.

Comment 26

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

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

Never mind, I found the offender:

      case "Forms:Select:Blur": {
        this.setFocusedElement(null);
        break;
      }

Comment 27

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

Attachment: Patch part 1, v1.3

ulei, sorry for the naive mistake. So it turned out when the screen is off, the blur event is caused by us calling setFocusedElement() from "Forms:Select:Blur" message. Since the event triggers event listener and then hideKeyboard(), it doesn't really make sense for us to call setFocusedElement() directly.

With this patch, setFocusedElement() is always called by showKeyboard() or hideKeyboard() so the event is always handled properly.

Comment 28

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

https://tbpl.mozilla.org/?tree=Try&rev=0e14f0d230cd
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=0e14f0d230cd

Comment 29

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 8480423 [details] [diff] [review]
Patch part 1, v1.3

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

Thanks for digging into this.

Comment 30

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

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #22)
> /tests/dom/base/test/test_bug976673.html failed on comment 19.

This passes locally with the new patch, and locally with the old one. So I can't reproduce the test error on my machine.

Let's see what try run returns in comment 28.

Comment 31

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

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #30)
> Let's see what try run returns in comment 28.

m-4 on emulator unfortunately failed again. I will abuse try tomorrow to get more stuff dumped to see what's going on there.

Comment 32

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

Attachment: Patch v1.4

So it turned out I need to delete the focusedElement.blur() call inside the setFocusedElement() method, since with the last patch, setFocusedElement() is always called when the element is already blurred (or removed from the DOM; in this case we don't really care.)

With the all green try run I can confirm that's the only cause for the failed test, so this should be the last review request.

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

I can't confirm, but it seems the test will only fail in the specific timing on the servers.

Comment 33

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #32)
> Created attachment 8480586 [details] [diff] [review]
> Patch v1.4
> 
> So it turned out I need to delete the focusedElement.blur() call inside the
> setFocusedElement() method, since with the last patch, setFocusedElement()
> is always called when the element is already blurred (or removed from the
> DOM; in this case we don't really care.)
I afraid it's not true that setFocusedElement() is always called when the element is
already blurred. When page hides and form submitted, the keyboard will be closed, but
the focus seems to remain and should be killed:
  https://github.com/mozilla/gecko-dev/blob/3757575378f34afd3f9a7d4553d2eeca1cd1f0f9/dom/inputmethod/forms.js#L373
We need double check with these cases before removing blur() call inside the
setFocusedElement() method.
> 
> With the all green try run I can confirm that's the only cause for the
> failed test, so this should be the last review request.
> 
> https://tbpl.mozilla.org/?tree=Try&rev=f531edea8b7a
> 
> I can't confirm, but it seems the test will only fail in the specific timing
> on the servers.
Could you explain why focusedElement.blur() cause `dom/base/test/test_bug976673.html` failed?

Comment 34

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 8480586 [details] [diff] [review]
Patch v1.4

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

I'll cancel the review temporarily until comment 33 addressed.

Comment 35

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

(In reply to Yuan Xulei [:yxl] from comment #33)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> comment #32)
> > Created attachment 8480586 [details] [diff] [review]
> > Patch v1.4
> > 
> > So it turned out I need to delete the focusedElement.blur() call inside the
> > setFocusedElement() method, since with the last patch, setFocusedElement()
> > is always called when the element is already blurred (or removed from the
> > DOM; in this case we don't really care.)
> I afraid it's not true that setFocusedElement() is always called when the
> element is
> already blurred. When page hides and form submitted, the keyboard will be
> closed, but
> the focus seems to remain and should be killed:
>  

In this case, let's try

      case "pagehide":
      case "beforeunload":
        // We are only interested to the pagehide and beforeunload events from
        // the root document.
        if (target && target != content.document) {
          break;
        }
        // fall through
      case "submit":
        if (this.focusedElement) {
          this.focusedElement.blur();
        }        
        break;

      case "blur":
        if (this.focusedElement) {
          this.hideKeyboard();
          this.selectionStart = -1;
          this.selectionEnd = -1;
        }

        break;

since that will preserve the blur event?

> > I can't confirm, but it seems the test will only fail in the specific timing
> > on the servers.
> Could you explain why focusedElement.blur() cause
> `dom/base/test/test_bug976673.html` failed?

I only have a theory, and we can prove that by annotating forms.js and push the patch to try if we need to.

So the said test basically try to move focus between frames and make sure the input on the parent frame, when focused, remove the focus on the input in the iframe (and vise versa). Without my patch, we basically call |focusedElement.blur()| in the blur callback itself. However, my patch move that to the next tick callback, and somehow only on try server, the next tick callback will be run *after* the next postMessage event, so we remove the focus in the wrong time and cause the error.

I still think we should remove |focusedElement.blur()| from setFocusedElement(), because I don't think we should mutate the real focus state from that function -- setFocusedElement() function should be regarded as a state switcher of our internal |focusedElement| state -- hence the proposal.

I will try this on my phone and also push it to try, if you agree with this. Thank you for the in-depth discussion.

Comment 36

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

Attachment: Patch v1.5 (part 1)

Tested locally for original STR in bug 820057 and works as expected.

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

Comment 37

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 8481106 [details] [diff] [review]
Patch v1.5 (part 1)

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

Tim, thanks.
The patch is reasonable and well addresses comment 33. So I will give r+.

Comment 38

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #35)
...
> I still think we should remove |focusedElement.blur()| from
> setFocusedElement(), because I don't think we should mutate the real focus
> state from that function -- setFocusedElement() function should be regarded
> as a state switcher of our internal |focusedElement| state -- hence the
> proposal.
> 
> I will try this on my phone and also push it to try, if you agree with this.
> Thank you for the in-depth discussion.
I'm not against removing |focusedElement.blur()| from setFocusedElement() as long as we don't break anything:)
Moreover this change will make code logic more clear.

Comment 39

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

checkin-needed as all try runs have turned green in comment 36.

Comment 40

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5f66ff9f11f2
https://hg.mozilla.org/integration/mozilla-inbound/rev/de5cefa8e52e

Comment 41

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5f66ff9f11f2
https://hg.mozilla.org/mozilla-central/rev/de5cefa8e52e

Comment 42

[Ryan VanderMeulen [:RyanVM]]

Actually, this made Gaia UI tests perma-fail. Backed out.
https://hg.mozilla.org/mozilla-central/rev/9b3fbc370631

https://tbpl.mozilla.org/php/getParsedLog.php?id=47036173&tree=Mozilla-Inbound

Comment 43

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

Sigh. I am now building B2G Desktop to see if I could reproduce that locally.

I might create a separate bug for landing the test fixes first.

Comment 44

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

I can reproduce the test error locally and visually confirm the patch might break something.

I however have trouble figuring out how to get dump() from the B2G Desktop during the test.

Comment 45

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

So indeed we have a valid bug on the patch somehow similar to bug 914100 comment 31. To reproduce, go to Message -> New Message -> wait for the keyboard to show -> tap the To field again. It looks like Message app will blur the contenteditable and focus on it again in the same function loop. Since the focus element is not updated we remove the keyboard on next tick.

Comment 46

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

Attachment: Patch v1.6 (part 1)

So to fix the problem above, instead of check the focused element in hideKeyboardSync(), I check the focus counter instead.

I also removed two |if (this.focusedElement === el)| checks in |setFocusedElement()| and |showKeyboard()| to ensure the states (counter, selection etc.) got updated and inputcontextchange got sent when the application blur and focus on the same element.

Comment 47

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

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

Comment 48

[Yuan Xulei [:yxl] (Leave from May 18)]

Comment on attachment 8482096 [details] [diff] [review]
Patch v1.6 (part 1)

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

Thanks. The patch covers the case I neglected.

::: dom/inputmethod/forms.js
@@ -242,5 @@
>      let self = this;
>  
> -    if (element === this.focusedElement)
> -      return;
> -

I don't think it is necessary to remove this check. I guess you remove this to
update the focus count. If so, please call |this.focusedElement = element| and 
return earlier to avoid removing and adding the same event listeners and observer 
redundantly.
Moreover, I don't think we need to increase the focus count here.
When setting focus on an input, we may get more than more focus event and the
|if (this.focusedElement === el)| check will filter out duplicated focus event.
I think we should regard the successive focus events from the same element as one single focus event and only ask the keyboard to show once.

@@ -641,5 @@
>    },
>  
>    showKeyboard: function fa_showKeyboard(target) {
> -    if (this.focusedElement === target)
> -      return;

We cannot remove this check simply.
If we find the keyboard is already shown for the focusedElement, we don't need to show it again, but only update the input state info.

To update the state info, you could call `this.updateSelection()`.

Comment 49

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

(In reply to Yuan Xulei [:yxl] from comment #48)
> Comment on attachment 8482096 [details] [diff] [review]
> Patch v1.6 (part 1)
> 
> Review of attachment 8482096 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks. The patch covers the case I neglected.
> 
> ::: dom/inputmethod/forms.js
> @@ -242,5 @@
> >      let self = this;
> >  
> > -    if (element === this.focusedElement)
> > -      return;
> > -
> 
> I don't think it is necessary to remove this check. I guess you remove this
> to update the focus count. If so, please call 
> |this.focusedElement = element| and  return earlier to avoid removing 
> and adding the same event listeners and observer redundantly.

Adding the same event listener to has no effect, according to how event target interface works.
Additionally, we do remove/disconnect all previous event listeners cleanly. I don't think the redundantly hurts us if we unapply/apply the stuff cleanly when switching states.

> Moreover, I don't think we need to increase the focus count here.

Something as to be different so hideKeyboardSync() could return early accordingly. In the patch I choose the counter. I don't know what else to do on that check (except adding another flag like |this._shouldRemoveFocus| but I really like not to do that).

> When setting focus on an input, we may get more than more focus event and the
> |if (this.focusedElement === el)| check will filter out duplicated focus
> event.

Do you have a STR to reproduce this to make the check necessary? Shouldn't be find the root cause of that instead of filter it out? These lines was introduced all the way back in bug 796080 and no comment was documented on what would happen if there isn't such check.

> I think we should regard the successive focus events from the same element
> as one single focus event and only ask the keyboard to show once.
> 
> @@ -641,5 @@
> >    },
> >  
> >    showKeyboard: function fa_showKeyboard(target) {
> > -    if (this.focusedElement === target)
> > -      return;
> 
> We cannot remove this check simply.
> If we find the keyboard is already shown for the focusedElement, we don't
> need to show it again, but only update the input state info.
> 
> To update the state info, you could call `this.updateSelection()`.

Why would the keyboard already shown when showKeyboard() is called? The only caller of showKeyboard() is handleEvent:"focus" according to the code.

Could you give me more guidance on that? Thanks.

Comment 50

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #49)
> (In reply to Yuan Xulei [:yxl] from comment #48)
> > Comment on attachment 8482096 [details] [diff] [review]
> > Patch v1.6 (part 1)
> > 
> > Review of attachment 8482096 [details] [diff] [review]:
> > -----------------------------------------------------------------
> > 
> > Thanks. The patch covers the case I neglected.
> > 
> > ::: dom/inputmethod/forms.js
> > @@ -242,5 @@
> > >      let self = this;
> > >  
> > > -    if (element === this.focusedElement)
> > > -      return;
> > > -
> > 
> > I don't think it is necessary to remove this check. I guess you remove this
> > to update the focus count. If so, please call 
> > |this.focusedElement = element| and  return earlier to avoid removing 
> > and adding the same event listeners and observer redundantly.
> 
> Adding the same event listener to has no effect, according to how event
> target interface works.
> Additionally, we do remove/disconnect all previous event listeners cleanly.
> I don't think the redundantly hurts us if we unapply/apply the stuff cleanly
> when switching states.
> 
> > Moreover, I don't think we need to increase the focus count here.
> 
> Something as to be different so hideKeyboardSync() could return early
> accordingly. In the patch I choose the counter. I don't know what else to do
> on that check (except adding another flag like |this._shouldRemoveFocus| but
> I really like not to do that).
I'm not against using counter and I think it is a good way to make hideKeyboardSync()
return early. But if setFocusedElement() is called with the same the element, we don't
need to increase the counter. That's why I think you should keep the 
|if (element === this.focusedElement)| check.
> 
> > When setting focus on an input, we may get more than more focus event and the
> > |if (this.focusedElement === el)| check will filter out duplicated focus
> > event.
> 
> Do you have a STR to reproduce this to make the check necessary? Shouldn't
> be find the root cause of that instead of filter it out? These lines was
> introduced all the way back in bug 796080 and no comment was documented on
> what would happen if there isn't such check.
I'll provide a test file to reproduce the case of duplicated focus events later.
> 
> > I think we should regard the successive focus events from the same element
> > as one single focus event and only ask the keyboard to show once.
> > 
> > @@ -641,5 @@
> > >    },
> > >  
> > >    showKeyboard: function fa_showKeyboard(target) {
> > > -    if (this.focusedElement === target)
> > > -      return;
> > 
> > We cannot remove this check simply.
> > If we find the keyboard is already shown for the focusedElement, we don't
> > need to show it again, but only update the input state info.
> > 
> > To update the state info, you could call `this.updateSelection()`.
> 
> Why would the keyboard already shown when showKeyboard() is called? The only
> caller of showKeyboard() is handleEvent:"focus" according to the code.
> 
> Could you give me more guidance on that? Thanks.
When focus on a new input element, we may get several focus events and those
events make showKeyboard() being called several times. 
I'll give a example tomorrow.

Comment 51

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

(In reply to Yuan Xulei [:yxl] from comment #50)
> > Something as to be different so hideKeyboardSync() could return early
> > accordingly. In the patch I choose the counter. I don't know what else to do
> > on that check (except adding another flag like |this._shouldRemoveFocus| but
> > I really like not to do that).
> I'm not against using counter and I think it is a good way to make
> hideKeyboardSync()
> return early. But if setFocusedElement() is called with the same the
> element, we don't
> need to increase the counter. That's why I think you should keep the 
> |if (element === this.focusedElement)| check.

Allow me to rephrase comment 49. So the approach of this bug is to move Focus:Input:"blur" message to the next tick, so that if there is a focus event follows a blur event in the same function loop, the blur message will be cancelled (and the remote process will only hear one focus message).

There are two ways to make a focus event happen right after the blur event, and before the next tick:

1) When the user taps an input while the focus is on another input
2) When the app programmatically calls blur() and calls focus() in one function loop, on the same input.
3) When the app programmatically calls blur() and calls focus() in one function loop, on a *different* input.

The return early part of hideKeyboardSync() should catch both scenarios, and fail to catch it (and not stopping the blur message) will mistakenly hide the keyboard -- a regression we can't afford.

To check against (1) and (3) it's quite simple, since the focusedElement will be different, |if (element === this.focusedElement)| will be enough -- that's patch v1.5.

(2) is tricky and it's indeed what we have encountered in Message app. What will happen on this case with patch v1.5 is:

a) blur() calls hideKeyboard() (*not* hideKeyboardSync()), so no properties (counter & focusedElement) is changed in that call. 
b) focus() triggers showKeyboard() and then setFocusedElement(), but it will get return early (since the target element is the same as the current FocusedElement). 
c) next tick hits and find the focusedElement unchanged, so blur message is not cancelled.

So to work around that, in patch v1.6, I remove the return early part in (b) and changed the check in (c) to work against counter. The side effect (which I would argue is the correct effect) is that we will send a Focus:Input message on the same input field again in (b).

I agree this is more complex then I have previously thought, but let's figured out how to cover all possible cases.

Comment 52

[Yuan Xulei [:yxl] (Leave from May 18)]

Thanks for the above explanation. It's quite clear and you patch could solve the case (2) of Comment 51.

I wonder if we could improve it to work around the duplicated focus events issue. Each time focusing on any input element in the UI tests app, we get two focus events and setFocusedElement() in forms.js is called twice. 

Step to reproduce:

1. Add a dump log to setFocusedElment():
  https://github.com/mozilla/gecko-dev/blob/3b25cb1121b5a528ae7678ec75b52261c477c7ec/dom/inputmethod/forms.js#L241
  Build the master build for a flame device.
2. Open gaia UI tests and select keyboard
3. On the keyboard page, touch the any input field and monitor the logcat output

expected:
one setFocusedElment log is dumped when the keyboard is open.

actual:
two setFocusedElment logs is dumped.

Comment 53

[Yuan Xulei [:yxl] (Leave from May 18)]

Attachment: Patch v1.7 (part 1)

This may solve the issue of  Comment 51 and Comment 52.

Comment 54

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

(In reply to Yuan Xulei [:yxl] from comment #52)
> Step to reproduce:
> 
> 1. Add a dump log to setFocusedElment():
>  
> https://github.com/mozilla/gecko-dev/blob/
> 3b25cb1121b5a528ae7678ec75b52261c477c7ec/dom/inputmethod/forms.js#L241
>   Build the master build for a flame device.
> 2. Open gaia UI tests and select keyboard
> 3. On the keyboard page, touch the any input field and monitor the logcat
> output

I have tried this too, and I can confirm there are duplicated focus event, but the events are not filed on input. Using |dump('==XX==' + target.toString() + '\n');| I got these two event when I touch the white area of keyboard test page, effectively move the focus from UI test menu to keyboard test frame:

I/Gecko   (17820): ==XX==[object HTMLDocument]
I/Gecko   (17820): ==XX==[object Window]

and when I touch the input element I got one expected focus event.

I/Gecko   (17820): ==XX==[object HTMLInputElement]

Incidentally, the previous events are not handled if body is not contenteditable. So we don't really need to filter out the event. However, since we do redirect HTML/document/window focus to body, maybe we do want to filter them out.

(In reply to Yuan Xulei [:yxl] from comment #53)
> Created attachment 8482687 [details] [diff] [review]
> Patch v1.7 (part 1)
> 
> This may solve the issue of  Comment 51 and Comment 52.

I ended up use diff to compare v1.6 and v1.7. Besides s/fa_hideKeyboardSync/fa_showKeyboardSync/ I don't have any strong opinion on this patch. I don't think there will be a side effect to move |sendKeyboardState| to the next tick so I think we can land this. Do you want to push this to try first?

(I previously want to put |this.setFocusedElement(null);| to next tick because I am not sure if removing the focusedElement there will mistakenly invalid any inflight promise. But I realized once hideKeyboard() is called, the inputcontext will always be invalid eventually -- being replaces with a new one of the same input or another input, or to null, so that's not really an issue.)

Comment 55

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Yuan Xulei [:yxl] from comment #53)
> Created attachment 8482687 [details] [diff] [review]
> Patch v1.7 (part 1)
> 
> This may solve the issue of  Comment 51 and Comment 52.

push to try for this patch:

https://tbpl.mozilla.org/?tree=Try&rev=6ca5b11028ba
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=6ca5b11028ba

Comment 56

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #54)
> (In reply to Yuan Xulei [:yxl] from comment #52)
> > Step to reproduce:
> > 
> > 1. Add a dump log to setFocusedElment():
> >  
> > https://github.com/mozilla/gecko-dev/blob/
> > 3b25cb1121b5a528ae7678ec75b52261c477c7ec/dom/inputmethod/forms.js#L241
> >   Build the master build for a flame device.
> > 2. Open gaia UI tests and select keyboard
> > 3. On the keyboard page, touch the any input field and monitor the logcat
> > output
> 
> I have tried this too, and I can confirm there are duplicated focus event,
> but the events are not filed on input. Using |dump('==XX==' +
> target.toString() + '\n');| I got these two event when I touch the white
> area of keyboard test page, effectively move the focus from UI test menu to
> keyboard test frame:
> 
> I/Gecko   (17820): ==XX==[object HTMLDocument]
> I/Gecko   (17820): ==XX==[object Window]
> 
> and when I touch the input element I got one expected focus event.
> 
> I/Gecko   (17820): ==XX==[object HTMLInputElement]
> 
> Incidentally, the previous events are not handled if body is not
> contenteditable. So we don't really need to filter out the event. However,
> since we do redirect HTML/document/window focus to body, maybe we do want to
> filter them out.
Thanks for the clarification. You are right that I don't need to worry about the
duplicated focus event. So we can safely remove the two
|if (this.focusedElement === el)| checks in |setFocusedElement()| and |showKeyboard()|.

Comment 57

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

(In reply to Yuan Xulei [:yxl] from comment #56)
> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> comment #54)
> > Incidentally, the previous events are not handled if body is not
> > contenteditable. So we don't really need to filter out the event. However,
> > since we do redirect HTML/document/window focus to body, maybe we do want to
> > filter them out.
> Thanks for the clarification. You are right that I don't need to worry about
> the
> duplicated focus event. So we can safely remove the two
> |if (this.focusedElement === el)| checks in |setFocusedElement()| and
> |showKeyboard()|.

Not really. Please the note begin with "However" ...

We should land your patch instead.

Comment 58

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

Try push for patch v1.7:

https://tbpl.mozilla.org/?tree=Try&rev=5305cc8527fe

Comment 59

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

I pushed to try again because the test was not included in your push in comment 55.

Comment 60

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

I am fixing test_delete_focused_element.html again.

Comment 61

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

Yulei, 

It turned out patch v1.7 has some issue. The way to fix the test error is:

$ hg di
diff --git a/dom/inputmethod/mochitest/test_delete_focused_element.html b/dom/inputmethod/mochitest/test_delete_focused_element.html
--- a/dom/inputmethod/mochitest/test_delete_focused_element.html
+++ b/dom/inputmethod/mochitest/test_delete_focused_element.html
@@ -24,18 +24,20 @@ inputmethod_setup(function() {
 function appFrameScript() {
   let input = content.document.getElementById('test-input');
   let textarea = content.document.createElement('textarea');
   textarea.lang = 'en';
 
   content.document.body.appendChild(textarea);
 
   textarea.onfocus = function() {
-    textarea.parentNode.removeChild(textarea);
-    sendAsyncMessage('test:InputMethod:finished', {});
+    content.setTimeout(function() {
+      textarea.parentNode.removeChild(textarea);
+      sendAsyncMessage('test:InputMethod:finished', {});
+    }, 10);
   };
 
   content.setTimeout(function() {
     content.setTimeout(function() {
       textarea.focus();
     }, 10);
 
     input.parentNode.removeChild(input);

So that we don't remove the input in the sync loop when |textarea.focus()| is called. Without this fix we will be sending Forms::Input::blur message twice. I am hesitate to fix the test because there shouldn't be any scenarios in app that make us sending two Forms::Input::blur messages. We need to figure out a better approach here.

Comment 62

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

I think we can leverage |this.isKeyboardOpened| property in this case to make sure we don't send the blur message twice.

(We however can send focus message twice to switch to next input -- this is the intention of this bug)

I am going to try it tomorrow.

Comment 63

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

I am going to create two more test cases to fully cover what we have encountered here -- let's not rely on the possibility of Gaia (and Gaia UI tests) never change it's behavior:

1. Switch from one input to another should generate one Forms:input:focus message
2. Calling input.blur() and input.focus() in the same function loop should generate one Forms:input:focus message
3. Calling input.focus() and input.blur() in the same function loop should generate zero message.

Replace blur() with "remove the focused element" above should result the same behavior, but let's not duplicate the tests?

Comment 64

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

Attachment: Patch v2.0 (part 1)

The only difference between this and v1.7 is the |!this.isKeyboardOpened| check in hideKeyboardSync(). When the property is not set it simply means focus message was not sent, so we shouldn't send the blur message there.

Comment 65

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

Attachment: Patch v2.0 (part 2)

Here is the updated test_two_inputs.html that covers all the issues we have encountered. I annotated the patch with comment to specify what happen on each step in app and what we expect in IME.

Noted that I still have to add one more setTimeout() on delete_focused_element.html because all the focus and removal steps all has to be async there. Maybe it's worthy to pave over that test with test_two_inputs.html in the next bug.

Comment 66

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

Try push:

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

(I made some changes in the comment for the test script so they are not 100% identical)

Comment 67

[Yuan Xulei [:yxl] (Leave from May 18)]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #61)
> Yulei, 
> 
> It turned out patch v1.7 has some issue. The way to fix the test error is:
> 
> $ hg di
> diff --git a/dom/inputmethod/mochitest/test_delete_focused_element.html
> b/dom/inputmethod/mochitest/test_delete_focused_element.html
> --- a/dom/inputmethod/mochitest/test_delete_focused_element.html
> +++ b/dom/inputmethod/mochitest/test_delete_focused_element.html
> @@ -24,18 +24,20 @@ inputmethod_setup(function() {
>  function appFrameScript() {
>    let input = content.document.getElementById('test-input');
>    let textarea = content.document.createElement('textarea');
>    textarea.lang = 'en';
>  
>    content.document.body.appendChild(textarea);
>  
>    textarea.onfocus = function() {
> -    textarea.parentNode.removeChild(textarea);
> -    sendAsyncMessage('test:InputMethod:finished', {});
> +    content.setTimeout(function() {
> +      textarea.parentNode.removeChild(textarea);
> +      sendAsyncMessage('test:InputMethod:finished', {});
> +    }, 10);
>    };
>  
>    content.setTimeout(function() {
>      content.setTimeout(function() {
>        textarea.focus();
>      }, 10);
>  
>      input.parentNode.removeChild(input);
> 
> So that we don't remove the input in the sync loop when |textarea.focus()|
> is called. Without this fix we will be sending Forms::Input::blur message
> twice. I am hesitate to fix the test because there shouldn't be any
> scenarios in app that make us sending two Forms::Input::blur messages. We
> need to figure out a better approach here.
Good point!
Agree with you that we should not send two Forms::Input::blur messages for
the same input. Likewise, should not send two Forms::Input::focus messages as well.

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #63)
> I am going to create two more test cases to fully cover what we have
> encountered here -- let's not rely on the possibility of Gaia (and Gaia UI
> tests) never change it's behavior:
> 
> 1. Switch from one input to another should generate one Forms:input:focus
> message
> 2. Calling input.blur() and input.focus() in the same function loop should
> generate one Forms:input:focus message
> 3. Calling input.focus() and input.blur() in the same function loop should
> generate zero message.

Maybe we could make case 2 and 3 more general.

Assuming the input doesn't have focus:
a) calling blur() and focus() alternately in the same function for one or more
times should generate one Forms:input:focus message.

b) calling focus() and blur() alternately in the same function for one or more
times should not generate any message.

Assuming the input has focus:
b) calling focus() and blur() alternately in the same function for one or more
times should generate one Forms:input:blur message.

a) calling blur() and focus() alternately in the same function for one or more
times should not generate any message.

> 
> Replace blur() with "remove the focused element" above should result the
> same behavior, but let's not duplicate the tests?
OK.

Comment 68

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

> (In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from
> comment #63)
> > I am going to create two more test cases to fully cover what we have
> > encountered here -- let's not rely on the possibility of Gaia (and Gaia UI
> > tests) never change it's behavior:
> > 
> > 1. Switch from one input to another should generate one Forms:input:focus
> > message
> > 2. Calling input.blur() and input.focus() in the same function loop should
> > generate one Forms:input:focus message
> > 3. Calling input.focus() and input.blur() in the same function loop should
> > generate zero message.
> 
> Maybe we could make case 2 and 3 more general.
> 
> Assuming the input doesn't have focus:
> a) calling blur() and focus() alternately in the same function for one or
> more
> times should generate one Forms:input:focus message.
> 
> b) calling focus() and blur() alternately in the same function for one or
> more
> times should not generate any message.
> 
> Assuming the input has focus:
> b) calling focus() and blur() alternately in the same function for one or
> more
> times should generate one Forms:input:blur message.
> 
> a) calling blur() and focus() alternately in the same function for one or
> more
> times should not generate any message.
> 

The current patch will satisfy the first (a) and the second (b) because calling blur()/focus() on the unfocused/focused element has no effect and geneates no events, respectively. :)

Comment 69

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

checkin-needed, all green on comment 66.

Comment 70

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

Actually... there are yet to be identified Gecko regression going on (bug 1063049, bug 1062816 ...) so this should sit for more hours until the cause is identified.

Comment 71

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

False alarm. bug 1063049 is a layout bug.

Comment 72

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/87e71506ff5f
https://hg.mozilla.org/integration/b2g-inbound/rev/89d894e837d5

Comment 73

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/87e71506ff5f
https://hg.mozilla.org/mozilla-central/rev/89d894e837d5