Closed Bug 802073 Opened 7 years ago Closed 7 years ago

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

Categories

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

Other
Gonk (Firefox OS)
defect

Tracking

(blocking-basecamp:+, firefox18 fixed)

VERIFIED FIXED
blocking-basecamp +
Tracking Status
firefox18 --- fixed

People

(Reporter: iliu, Assigned: ttaubert)

References

Details

Attachments

(2 files, 8 obsolete files)

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.
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.
Assignee: nobody → ttaubert
Status: NEW → ASSIGNED
Blocks: 796698
Attached patch introduce a global keyboard (obsolete) — Splinter Review
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.
Attachment #673836 - Flags: review?(21)
Component: Builds → Gaia
Blocks: 802508
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?
Attachment #673836 - Flags: review?(21) → review-
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.
blocking-basecamp: ? → +
Priority: -- → P1
What is the user facing problem?   Need more info.
blocking-basecamp: + → ---
(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.
blocking-basecamp: --- → ?
Attached patch introduce a global keyboard v2 (obsolete) — Splinter Review
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?
Attachment #673836 - Attachment is obsolete: true
Attachment #674314 - Flags: review?(21)
Please request approval from 21
blocking-basecamp: ? → -
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.
Attached patch introduce a global keyboard v3 (obsolete) — Splinter Review
Forgot to check for dead wrappers.
Attachment #674314 - Attachment is obsolete: true
Attachment #674314 - Flags: review?(21)
Attachment #674900 - Flags: review?(21)
This is an important change for the stability of the keyboard and it will resolve some blocking-basecamp issues. blocking-basecamp+.
Status: ASSIGNED → NEW
blocking-basecamp: - → +
Status: NEW → ASSIGNED
Attached patch introduce a global keyboard v4 (obsolete) — Splinter Review
Sorry, had to fix another nit - we need to iterate the array of dead indexes backwards.
Attachment #674900 - Attachment is obsolete: true
Attachment #674900 - Flags: review?(21)
Attachment #675497 - Flags: review?(21)
Priority: P1 → --
Priority: -- → P3
Attached patch introduce a global keyboard v5 (obsolete) — Splinter Review
The MozKeyboard doesn't load the JSM anymore but communicates with it using message managers. I also removed the workaround introduced by bug 801838.
Attachment #675497 - Attachment is obsolete: true
Attachment #675497 - Flags: review?(21)
Attachment #676769 - Flags: review?(21)
(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.
Please don't land the patch, yet. I still have to figure out some weird focus issues.
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.
Flags: needinfo?(21)
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.
Attached patch introduce a global keyboard v6 (obsolete) — Splinter Review
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.
Attachment #676769 - Attachment is obsolete: true
Attachment #677376 - Flags: review?(21)
Attached patch introduce a global keyboard v6b (obsolete) — Splinter Review
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.
Attachment #677376 - Attachment is obsolete: true
Attachment #677376 - Flags: review?(21)
Attachment #677384 - Flags: review?(21)
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
Attachment #677384 - Flags: review?(21) → review-
Flags: needinfo?(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.

>  - 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.
(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.
Attached patch introduce a global keyboard v7 (obsolete) — Splinter Review
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.
Attachment #677384 - Attachment is obsolete: true
Attachment #678423 - Flags: review?(21)
Small correction.
Attachment #678423 - Attachment is obsolete: true
Attachment #678423 - Flags: review?(21)
Attachment #678427 - Flags: review?(21)
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 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.
Attachment #678427 - Flags: review?(21) → review+
(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!
(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.
(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;
(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.
Attachment #679127 - Flags: review?(xyuan)
Attachment #679127 - Flags: review?(21)
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 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.
Attachment #679127 - Flags: review?(xyuan) → review+
(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?
Attachment #679127 - Flags: review?(21)
Reviewed and verified on "Unagi device
Build ID:20130102070202
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.