Closed Bug 624900 Opened 9 years ago Closed 9 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set

Tracking

(fennec2.0+)

VERIFIED FIXED
Tracking Status
fennec 2.0+ ---

People

(Reporter: aakashd, Assigned: blassey)

References

Details

Attachments

(2 files, 1 obsolete file)

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.
tracking-fennec: --- → ?
Does the ".com" part disappear if you just tap somewhere else on the webpage, just to make the textbox lose focus?
It's the latter. I only see this happen when tapping down on the form assistant.
(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?
tracking-fennec: ? → 2.0+
Assignee: nobody → blassey.bugs
WFM on nexus one
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → WORKSFORME
I have reproduced this fairly regularly on bugzilla. The "com" portion of the email is not committed to the textbox when it blurs.
(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
Droid X

I can definitely still see this with todays nightly, although it occasionally seems to work.
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
This is working for me. Wes, perhaps you can give your device to Mwu to take a look?
Assignee: blassey.bugs → mwu
Not going to be able to look at this.
Assignee: mwu → nobody
Assignee: nobody → blassey.bugs
Duplicate of this bug: 633230
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"
this isn't what I thought it was, reassigning to crowder
Assignee: blassey.bugs → crowderbt
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)
Attached patch patch (obsolete) — Splinter Review
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?
Assignee: crowderbt → blassey.bugs
Status: REOPENED → NEW
Attachment #513719 - Flags: review?(jones.chris.g)
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.
Attachment #513719 - Flags: review?(jones.chris.g) → review-
(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?
(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.
(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?
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.
Attached patch patchSplinter Review
this patch makes the seqno logic more restrictive and that's fixing this issue for me
Attachment #513719 - Attachment is obsolete: true
Attachment #514418 - Flags: review?(jones.chris.g)
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.
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=======================
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.
Attached patch patchSplinter Review
Attachment #514427 - Flags: review?(jones.chris.g)
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!
Attachment #514427 - Flags: review?(jones.chris.g) → review+
pushed http://hg.mozilla.org/mozilla-central/rev/056c9ea05c26
Status: NEW → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Duplicate of this bug: 616980
Attachment #514418 - Flags: review?(jones.chris.g)
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.