624900 - ".com" portion of email address disappears when spanning to password field on facebook.com

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Aakash Desai [:aakashd]]

Build Id:
Mozilla/5.0 (Android; Linux armv71; rv:2.0b9pre) Gecko/20100111 Namoroka/4.0b10pre Fennec/4.0b4pre

Steps to Reproduce:
1. Go to www.facebook.com
2. Tap on the username field and enter an e-mail address
3. Tap on the form assistant down button

Actual Results:
The ".com" disappears

Expected Results:
The ".com" does not disappear.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Does the ".com" part disappear if you just tap somewhere else on the webpage, just to make the textbox lose focus?

Comment 2

[Aakash Desai [:aakashd]]

It's the latter. I only see this happen when tapping down on the form assistant.

Comment 3

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #2)
> It's the latter. I only see this happen when tapping down on the form
> assistant.

Strange. The web content focus should change to the next widget, which should force the keyboard/IME code to commit the remaining text into the original textbox.

Brad - any thoughts on the "focus change" / "ime commit" problem here?

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

WFM on nexus one

Comment 5

[Wesley Johnston (:wesj)]

I have reproduced this fairly regularly on bugzilla. The "com" portion of the email is not committed to the textbox when it blurs.

Comment 6

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #5)
> I have reproduced this fairly regularly on bugzilla. The "com" portion of the
> email is not committed to the textbox when it blurs.

What device? Reopen if you can still repro

Comment 7

[Wesley Johnston (:wesj)]

Droid X

I can definitely still see this with todays nightly, although it occasionally seems to work.

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

This is working for me. Wes, perhaps you can give your device to Mwu to take a look?

Comment 9

[Michael Wu [:mwu]]

Not going to be able to look at this.

Comment 10

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

I've seen it too on Nexus One on gmail.

Comment 12

[Brad Lassey [:blassey] (use needinfo?)]

I'm seeing very different (but still not correct) behavior when I pulled this form code out of facebook and put it in about:home (so its in the chrome process).

For the first .com in the chrome process (which works fine in the content process in my testing) switching to the password field leaves the .com in the username field and also sticks it in the password field. Typing .com again in the username field does not show suggestions as it does in the content process.

Using <input type="text"> instead of <input type="email"> in the chrome process I frequently see double copies of the suggested text either in the same input field or in the one you're switching to.

In the content process I see no difference in behavior between type="text" and type="email"

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

this isn't what I thought it was, reassigning to crowder

Comment 14

[Brad Lassey [:blassey] (use needinfo?)]

I just realized that I don't see this problem in debug builds, which leads me to believe that our IME events are racing with our focus events (and the debug build slows that race in such a way that the IME event wins)

Comment 15

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Chris, this patch fixes this issue. But why were we keeping track of the last blur's seqno separately from the last event's seqno? Am I missing something here?

Comment 16

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 513719 [details] [diff] [review]
patch

The comment this patch deletes somewhat explains what that member does.  After a blur, the widget doesn't have focus anymore, so the IME events targeted at it from before the blur are stale and shouldn't be dispatched.  This patch removes that system, which would I think result in web-visible behavior changes, namely input events dispatched to unfocused elements.  (Someone who knows more IME than me should verify that.)  Also, the FlushPendingRPCQueue() in here is just wrong.

I think we need another solution here; unfortunately this is the peril of async IME.  I don't have any ideas off the top of my head, except for a risky take-messages-matching-type-X-from-pending-queue IPDL API that's too risky for 4.0.  May need to move this to .x.

Comment 17

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Filed bug 636063 on the API mentioned in comment 16.

Comment 18

[Brad Lassey [:blassey] (use needinfo?)]

(In reply to comment #16)
> Comment on attachment 513719 [details] [diff] [review]
> patch
> 
> The comment this patch deletes somewhat explains what that member does.  After
> a blur, the widget doesn't have focus anymore, so the IME events targeted at it
> from before the blur are stale and shouldn't be dispatched.  This patch removes
> that system, which would I think result in web-visible behavior changes, namely
> input events dispatched to unfocused elements.  (Someone who knows more IME
> than me should verify that.)  

with this patch mIMELastReceivedSeqno is set to the chromeSeqno after a blur and the DispatchEvent now throws out any events with a seqno less than mIMELastReceivedSeqno. So, as far as I can tell we'll never process an event that was sent before the blur after the blur is received.

> Also, the FlushPendingRPCQueue() in here is just
> wrong.
why? (you can answer that on IRC if you prefer)

The intent here is to get any async chrome -> content RPC is processed before our sync blur notification starts throwing out unprocessed events. Any other ideas for doing that?

Comment 19

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #18)
> with this patch mIMELastReceivedSeqno is set to the chromeSeqno after a blur
> and the DispatchEvent now throws out any events with a seqno less than
> mIMELastReceivedSeqno. So, as far as I can tell we'll never process an event
> that was sent before the blur after the blur is received.

If there are two pending IME events after a blur, your patch will discard the first and process the second.  That's wrong.

> 
> > Also, the FlushPendingRPCQueue() in here is just
> > wrong.
> why? (you can answer that on IRC if you prefer)
 
It won't do anything here: it literally does what it says, flushes the pending RPCs, which aren't involved in this code at all.  Why did you add it?

> The intent here is to get any async chrome -> content RPC is processed before
> our sync blur notification starts throwing out unprocessed events. Any other
> ideas for doing that?

Yes, see comment 16 and bug 636063.

Comment 20

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

(In reply to comment #19)
> (In reply to comment #18)
> > with this patch mIMELastReceivedSeqno is set to the chromeSeqno after a blur
> > and the DispatchEvent now throws out any events with a seqno less than
> > mIMELastReceivedSeqno. So, as far as I can tell we'll never process an event
> > that was sent before the blur after the blur is received.
> 
> If there are two pending IME events after a blur, your patch will discard the
> first and process the second.  That's wrong.

Actually, I misread the patch on first look, my fault.  AFAICT the scenario above wouldn't occur, but I think the result is the same semantics as before implemented more simply, which is good, but I don't see how it would fix this bug.  AIUI the bug is that we're dropping text event(s) on blur.  Can you explain how this patch fixes that?

Comment 21

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

A possible workaround for this bug (in lieu of bug 636063) is to have TabParent queue IME messages sent to the TabChild, have TabChild ACK messages as they're processed, then when TabChild sends the 'sync' blur notification, have TabParent returned its queue of un-ACK'd IME events.  Then TabChild would dispatch those events immediately before "actually" blurring itself.  This has potential perf implications from the ACKs and the event queue, but it's less risky.

Comment 22

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

this patch makes the seqno logic more restrictive and that's fixing this issue for me

Comment 23

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 514418 [details] [diff] [review]
patch

I can't comfortably r+ this patch without knowing what's fixing, sorry, and I can't tell from inspection.  Is TabParent sending IME events after the blur maybe?  (Which it shouldn't be doing, but the old code would block but this code would allow.)  Can you try logging the seqnos on blur and on dispatched events to see what changes?  If this patch just happens to be changing timing we won't have fixed this bug.

Comment 24

[Brad Lassey [:blassey] (use needinfo?)]

here's some logging of the sequence numbers. the key here is that the last composition event before the blur is 0 and that will always get dropped with the current implementation.


I/GeckoPuppet(14386): comp event 1
I/GeckoPuppet(14386): text event 2
I/GeckoPuppet(14386): text event 3
I/GeckoPuppet(14386): comp event 4
I/GeckoPuppet(14386): sel event 5
I/GeckoPuppet(14386): comp event 6
I/GeckoPuppet(14386): text event 7
I/GeckoPuppet(14386): text event 8
I/GeckoPuppet(14386): text event 9
I/GeckoPuppet(14386): text event 1104371776
I/GeckoPuppet(14386): comp event 0
I/GeckoPuppet(14386): text event 10
I/GeckoPuppet(14386): comp event 11
I/GeckoPuppet(14386): sel event 12
===================== blur =======================
I/GeckoPuppet(14386): comp event 13
I/GeckoPuppet(14386): text event 14
I/GeckoPuppet(14386): text event 15
I/GeckoPuppet(14386): comp event 16
I/GeckoPuppet(14386): sel event 17
I/GeckoPuppet(14386): comp event 18
I/GeckoPuppet(14386): text event 19
I/GeckoPuppet(14386): text event 20
I/GeckoPuppet(14386): text event 21
I/GeckoPuppet(14386): text event 1104371776
I/GeckoPuppet(14386): comp event 0
I/GeckoPuppet(14386): text event 22
I/GeckoPuppet(14386): comp event 23
I/GeckoPuppet(14386): sel event 24
======================== blur ===================
I/GeckoPuppet(14386): comp event 25
I/GeckoPuppet(14386): text event 26
I/GeckoPuppet(14386): text event 27
I/GeckoPuppet(14386): comp event 28
I/GeckoPuppet(14386): sel event 29
I/GeckoPuppet(14386): comp event 30
I/GeckoPuppet(14386): text event 32
I/GeckoPuppet(14386): text event 33
I/GeckoPuppet(14386): text event 1104371776
I/GeckoPuppet(14386): comp event 0
I/GeckoPuppet(14386): text event 34
I/GeckoPuppet(14386): comp event 35
I/GeckoPuppet(14386): sel event 36
=========================blur==================
I/GeckoPuppet(14386): comp event 37
I/GeckoPuppet(14386): text event 38
I/GeckoPuppet(14386): text event 39
I/GeckoPuppet(14386): comp event 40
I/GeckoPuppet(14386): sel event 41
I/GeckoPuppet(14386): comp event 42
I/GeckoPuppet(14386): text event 43
I/GeckoPuppet(14386): text event 44
I/GeckoPuppet(14386): text event 45
I/GeckoPuppet(14386): text event 1104371776
I/GeckoPuppet(14386): comp event 0
I/GeckoPuppet(14386): text event 46
I/GeckoPuppet(14386): comp event 47
I/GeckoPuppet(14386): sel event 48
================ blur=======================

Comment 25

[Brad Lassey [:blassey] (use needinfo?)]

the composition event with seqno = 0 is coming from here https://mxr.mozilla.org/mozilla-central/source/widget/src/xpwidgets/PuppetWidget.cpp#351

Comment 26

[Brad Lassey [:blassey] (use needinfo?)]

and the text event with the high seqno comes from here https://mxr.mozilla.org/mozilla-central/source/widget/src/xpwidgets/PuppetWidget.cpp#337

Comment 27

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Wow, this looks bad.  Nice find!

My memory of that code is pretty hazy, but I /think/ the right fix is to init both of those with seqno = mIMELastReceivedSeqno.

Comment 28

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: patch

Comment 29

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 514427 [details] [diff] [review]
patch

BTW, I was skimming back through this code, and I think the previous patch that eliminates mIMELastBlurSeqno isn't quite what we want, because mIMELastReceivedSeqno is used to filter NotifyIMESelection() in TabParent.  If mIMELastReceivedSeqno becomes the seqno seen on blur, then TabParent will potentially accept IME events that it would filter previously.

This patch looks good though, thanks!

Comment 30

[Brad Lassey [:blassey] (use needinfo?)]

pushed http://hg.mozilla.org/mozilla-central/rev/056c9ea05c26

Comment 32

[Ayan Shah[:ashah]]

Verified on:

Mozilla/5.0 (Android; Linux armv7l; rv:2.0b13pre) Gecko/20100307
Firefox/4.0b13pre Fennec/4.0b6pre

Device - Nexus One. 
If someone sees it on any other device, please reopen this bug.