802073 - Receive input event twice from input tag type:time and type:date

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia, defect, P3)

Description

[[e-mail], [e-mail]]

STR:
1. Launch the Settings APP
2. Click on Date & Time
3. Disable Set Automatically
4. Click on Date or Time section
5. Click "OK" button to set the value by the picker.

expected:
Receive "input" event once after clicked the "OK" button.

actual:
Receive "input" event twice after clicked the "OK" button.

Comment 1

[[e-mail], [e-mail]]

I can reproduce the issue in UI Test APP -> Keyboard test -> Input type=date and Input type = time.
If you want to reproduce it, please write some testbed to add "input" event for the tag.

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Attachment: introduce a global keyboard

The problem with the current design is, that every MozKeyboard instance listens for new frames, injects content scripts and listens for Forms:Input messages. That's problematic as the system and settings app access mozKeyboard. Every additional app that accesses it will then create even more listeners and content scripts.

A simple solution is to introduce a "global" keyboard which is initiated once. All other MozKeyboard instances contact it to send messages and be notified about focus changes. Ideally, the global keyboard should probably live in its own component and be initialized on app-startup. Relying on the system app to create a MozKeyboard instance to make the global keyboard listen for new frames sounds very flaky to me but would definitely work for now.

With this fix bug 801838 isn't reproducible anymore. I removed the workaround from bug 801838 before testing, of course.

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 673836 [details] [diff] [review]
introduce a global keyboard

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

Hey Tim, I think the solution is the right approach but It could probably be enhance by Moving the MozGlobalKeyboard code inside a JSM that is loaded by shell.js directly. That would ensure there is only one global keyboard and will follow the same mechanism as some others APIs.

Also instead of directly make calls to MozGlobalKeyboard inside MozKeyboard you may want to use sendAsyncMessage. Does it make sense?

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Moving the global code to a JSM sounds good. Do you really think we should add another level of indirection by using messages to communicate with the JSM? I think it's easier to just load it and call the functions we need. Otherwise we'd need separate messages and handling for all those methods.

Comment 5

[John Hammink]

What is the user facing problem?   Need more info.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to John Hammink from comment #5)
> What is the user facing problem?   Need more info.

We have a couple of bugs that are caused by this. Bug 801838 is a blocker and has an ugly workaround at the moment.

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

Attachment: introduce a global keyboard v2

Moved the global keyboard to a JSM. This is probably not the right component for this bug? We'd need to request approval-aurora, right?

Comment 8

[David Scravaglieri [:scravag]]

Please request approval from 21

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

Ok, but I don't understand why this isn't a blocker. Bug 801838 has an ugly workaround that might break in the future. If we don't fix this bug even more unforeseen problems might pop up the more apps we push OOP.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: introduce a global keyboard v3

Forgot to check for dead wrappers.

Comment 11

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

This is an important change for the stability of the keyboard and it will resolve some blocking-basecamp issues. blocking-basecamp+.

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Attachment: introduce a global keyboard v4

Sorry, had to fix another nit - we need to iterate the array of dead indexes backwards.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Attachment: introduce a global keyboard v5

The MozKeyboard doesn't load the JSM anymore but communicates with it using message managers. I also removed the workaround introduced by bug 801838.

Comment 14

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #13)
> I also removed the workaround introduced by bug 801838.

Haha, I didn't. That's of course a Gaia change and needs a separate PR.

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Please don't land the patch, yet. I still have to figure out some weird focus issues.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Vivien, I had some issues with this patch because of the ime-enabled-state-changed observer in forms.js. Sometimes the timer picker's value was applied and sometimes not. Turnes out that sometimes an <iframe> is the focusedElement. This is because we have a couple of IME state observers (every forms.js instance) and every one of them sends a Forms:Input messages.

Shouldn't the ime-enabled-state-changed observer apply the same check as the focus event handler? We shouldn't focus <iframe>s but only inputs and select boxes and the like, right?

On the other hand I don't understand what the IME state observer is for. If an input element is focused we receive a focus element. If it's blurred then we receive this event as well. I hope you can enlighten me a little here :)

Anyway, if I apply the same input element check as the focus event handler setting the time always works because we never set iframes as the focusedElement.

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

First, we have the focus and blur listeners. They listen for focus and blur events and set the focusedElement respectively.

Second, there's the ime-enabled-state-changed observer that does the same but also if the focused element doesn't actually change, but its IME state has.

Looks like the only thing we need is the observer? The focus/blur listeners seem redundant to me.

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

Attachment: introduce a global keyboard v6

I removed the focus and blur event handlers. Everything is now handled by the ime-enabled-state-changed observer and seems to work well for me. I removed the keypress event handlers as well because I was told that this is legacy code that used to handle to hardware 'back' key.

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Attachment: introduce a global keyboard v6b

Turns out we still need the 'blur' handler as the IME state doesn't change for some reason when the app is put in the background. Small fix.

Comment 20

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 677384 [details] [diff] [review]
introduce a global keyboard v6b

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

I tried it on the device in the 'New Contact' panel and I notice 2 things:
 - switching between fields show/hide the keyboard quickly
 - Clicking on the home button close the window and leave the keyboard on the screen for a few seconds

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Vivien Nicolas (:vingtetun) from comment #20)
> I tried it on the device in the 'New Contact' panel and I notice 2 things:
>  - switching between fields show/hide the keyboard quickly

That issue is already present now. For some reason it's much more visible and occurs more often with my patch.

>  - Clicking on the home button close the window and leave the keyboard on
> the screen for a few seconds

Same thing here, that issue isn't caused by my patch but is reproducible without it, too.

Comment 22

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Tim Taubert [:ttaubert] from comment #21)
> (In reply to Vivien Nicolas (:vingtetun) from comment #20)
> > I tried it on the device in the 'New Contact' panel and I notice 2 things:
> >  - switching between fields show/hide the keyboard quickly
> 
> That issue is already present now. For some reason it's much more visible
> and occurs more often with my patch.

I'm quite sure it's much more visible because we're hiding the keyboard on blur but show it on ime-enabled-state-changed - the latter takes a tad more time than the focus event. Now we could listen for the focus event as well and ignore the notification afterwards but there's no way around hiding and re-showing the keyboard as it may have to be re-rendered.

Comment 23

[Tim Taubert [:ttaubert] (inactive)]

Attachment: introduce a global keyboard v7

Ok, I thought a little more about it and I think we *do* need the focus and blur listeners. The ime-enabled-state-changed observer should just be there in case the IME state is enabled/disabled after the input has already been focused.

The issue you described with the flickering keyboard can be solved by using setTimeout() to wait a little before sending the blur message. If the focus and blur message are sent within 20ms the keyboard app will keep the keyboard open.

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

Attachment: introduce a global keyboard v7b

Small correction.

Comment 25

[Chris Peterson [:cpeterson]]

Comment on attachment 678427 [details] [diff] [review]
introduce a global keyboard v7b

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

::: b2g/chrome/content/forms.js
@@ +244,5 @@
> +    this.isKeyboardOpened = false;
> +    this.setFocusedElement(null);
> +  },
> +
> +  isFocusableElement: function fa_isFocusableElement(element) {

Do you think it would be too optimistic to add an isFocusableElement() debug check to setFocusedElement()? Something like:

    if (element && !this.isFocusableElement(element)) {
      throw new Error("element "+element+" is not focusable!");
    }

::: b2g/components/Keyboard.jsm
@@ +3,5 @@
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +'use strict';
> +
> +let EXPORTED_SYMBOLS = ['Keyboard'];

I think EXPORTED_SYMBOLS must now become a property of `this`:

-let EXPORTED_SYMBOLS = ['Keyboard'];
+this.EXPORTED_SYMBOLS = ['Keyboard'];

See khuey's post here:
https://groups.google.com/d/topic/mozilla.dev.platform/Ic2YmqbRyIE/discussion

Comment 26

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 678427 [details] [diff] [review]
introduce a global keyboard v7b

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

r+ but I think you should fix Chris Peterson comment about the EXPORTED_SYMBOLS.

::: b2g/chrome/content/forms.js
@@ +244,5 @@
> +    this.isKeyboardOpened = false;
> +    this.setFocusedElement(null);
> +  },
> +
> +  isFocusableElement: function fa_isFocusableElement(element) {

I think it is. There are some css properties to enable ime that can be use on anything.

Comment 27

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Chris Peterson (:cpeterson) from comment #25)
> > +  isFocusableElement: function fa_isFocusableElement(element) {
> 
> Do you think it would be too optimistic to add an isFocusableElement() debug
> check to setFocusedElement()? Something like:
> 
>     if (element && !this.isFocusableElement(element)) {
>       throw new Error("element "+element+" is not focusable!");
>     }

What do you mean by optimistic? I think we prevented setFocusedElement() from being called with an unfocusable element so I don't think we really need that check, do we?

> > +let EXPORTED_SYMBOLS = ['Keyboard'];
> 
> I think EXPORTED_SYMBOLS must now become a property of `this`:
> 
> -let EXPORTED_SYMBOLS = ['Keyboard'];
> +this.EXPORTED_SYMBOLS = ['Keyboard'];

Good point, thanks!

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Vivien Nicolas (:vingtetun) from comment #26)
> r+ but I think you should fix Chris Peterson comment about the
> EXPORTED_SYMBOLS.

Yes, addressed.

> > +  isFocusableElement: function fa_isFocusableElement(element) {
> 
> I think it is. There are some css properties to enable ime that can be use
> on anything.

There is https://developer.mozilla.org/en-US/docs/CSS/ime-mode which only applies to text fields so I think this filter is appropriate.

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/871a4aa1f78b

Comment 30

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

(In reply to Tim Taubert [:ttaubert] from comment #29)
> https://hg.mozilla.org/integration/mozilla-inbound/rev/871a4aa1f78b

Should the line 
   3.45 +    for (let name of this._messageNames)
be 
   3.45 +    for (let name in this._messageNames)
?

I don't think we support "for (x of xxx)" statement.

The following code prevents <select> element to show option list: 
    1.59 -        if (this.isKeyboardOpened)
    1.60 +        if (this.isIMEDisabled())
    1.61            return;

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Yuan Xulei(:yxl) from comment #30)
> I don't think we support "for (x of xxx)" statement.

We do. I tested the patch :)

> The following code prevents <select> element to show option list: 
>     1.59 -        if (this.isKeyboardOpened)
>     1.60 +        if (this.isIMEDisabled())
>     1.61            return;

Darn, you're absolutely right. Working on a fix, will upload in a minute.

Comment 32

[Tim Taubert [:ttaubert] (inactive)]

Attachment: check IME mode for text inputs only

Comment 33

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 679127 [details] [diff] [review]
check IME mode for text inputs only

Vivien, Yuan, I'd really appreciate a quick review from one or both of you so that I can land this follow-up quickly to not break too many people working on the bleeding edge. Thanks!

Comment 34

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

Comment on attachment 679127 [details] [diff] [review]
check IME mode for text inputs only

r+.
Thanks for your quick fix. It works well for <input>、<textarea> and <select> elements.  The only flaw is that it doesn't work for content editable element.

Comment 35

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Yuan Xulei(:yxl) from comment #34)
> Thanks for your quick fix. It works well for <input>、<textarea> and <select>
> elements.  The only flaw is that it doesn't work for content editable
> element.

Thank you for the quick review! The content editable didn't even work before without the patch. So I suppose we should file a separate bug for that?

Comment 36

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 37

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/871a4aa1f78b
https://hg.mozilla.org/mozilla-central/rev/5c0e7cdc779b

Comment 38

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/4455e1fa4dad
https://hg.mozilla.org/releases/mozilla-aurora/rev/4ae139ff61c8

Comment 39

[sarsenyev]

Reviewed and verified on "Unagi device
Build ID:20130102070202