615208 - Android keyboard does not insert/delete at the caret in content forms

Status: RESOLVED FIXED

(Core Graveyard :: Widget: Android, defect)

Description

[Matt Brubeck (:mbrubeck)]

A regression from bug 599811 or its followups.  This affects hardware and software keyboards on Android:

Steps to reproduce:
1. Open data:text/html,<input%20value=test> (or any page where a form field is pre-filled with text.
2. Tap the form field so the caret appears at the end of the text.
3. Press backspace.
4. Type a letter.

Expected results: Backspace deletes the last character; a letter is inserted at the end of the field.

Actual result: Backspace does nothing; a letter is inserted at the start of the field.

Comment 1

[Matt Brubeck (:mbrubeck)]

In the latest builds, I am getting slightly different but still broken behavior.  Pressing delete works correctly, but if I just type a letter, then the letter appears at the end of the text but the cursor immediately jumps backward to near the start of the text.

Based on my debugging so far, I can see that the OnIMESelectionChange call in OnIMEFocusChange is setting the selection incorrectly to 0,0.

This is expected based on the implementation of TabParent::RecvNotifyIMEFocus.  However, there should be another OnIMESelectionChange shortly afterward with the correct selection coordinates.  For some reason this is never received, possibly related to the "seqno" mechanism in TabParent/PuppetWidget...

Comment 2

[Matt Brubeck (:mbrubeck)]

After rebuilding with just a few lines of added logging, the second OnIMESelectionChange *is* received, and everything works correctly.  I think RecvNotifyIMESelection in a race with SendSelectionEvent, which causes seqno to increment and later NotifyIMESelection messages to be ignored.

Comment 3

[Matt Brubeck (:mbrubeck)]

Now I'm not sure how much of this is new, and how much is just a duplicate of bug 602013.

Comment 4

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

Attachment: patch

this seems to solve the issue for me. Matt, can you confirm?

the member variables should be renamed with an s prefix if we go with this patch though

Comment 5

[Matt Brubeck (:mbrubeck)]

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

With this patch applied to m-c trunk (and no other patches), I am not seeing any buggy behavior.

Comment 6

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

Attachment: patch

Comment 7

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

I don't understand what seqno bug this patch is fixing.  Can you give an example problem?  It's not obvious to me where the bug in this logic is, assuming focus()/blur() state is being maintained correctly (may be a bad assumption!).

Comment 8

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

(In reply to comment #7)
> I don't understand what seqno bug this patch is fixing.  Can you give an
> example problem? 
It looks to me like the puppet widget is being destroyed and a new one created under some circumstances but the TabParent isn't (reloading the page is a good way to reproduce). So, the seqno in the Puppet Widget is being reset to 0 while the seqno in the TabParent stays where it left off.

> It's not obvious to me where the bug in this logic is,
> assuming focus()/blur() state is being maintained correctly (may be a bad
> assumption!).
This may be a bad assumption, but I have no evidence either way for it.

Comment 9

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

another potential fix would be to send a message to the TabParent to reset its seqno whenever the PuppetWidget is created.

Comment 10

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

(In reply to comment #8)
> (In reply to comment #7)
> > I don't understand what seqno bug this patch is fixing.  Can you give an
> > example problem? 
> It looks to me like the puppet widget is being destroyed and a new one created

That shouldn't ever happen.  Is it?

Comment 11

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

(Except after initial TabChild creation, when a temporary about:blank document viewer is created until the real document viewer is.)

Comment 12

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

here's what I'm seeing without interpretation:
1) load http://lassey.us/input.html
2) click in the text box, everything is fine
3) reload the page
4) click in the text box, see the bug as described in comment #0

I've also added some logging, and when we get to step 4, the seqno in PuppetWidget is 0 and the seqno in TabParent is something else (>2, but depends on what you did in step 2). I made the assumption that we're creating a new PuppetWidget since that's the only place I see the seqno get reset.

Comment 13

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

In which PuppetWidget?  There should be two: one is a do-nothing parent wrapper owned by TabChild, needed for our embedding API.  There's exactly one child of that which is the "real" widget, owned by DocumentViewer.

Comment 14

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

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

To be clearer, I can't tell what problem this patch is fixing.  We should understand the underlying issue to the point where a comment can be added to this patch explaining its changes.

Comment 15

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

Attachment: alternative patch

The situation is as I described it in comment 8, on reload a new PuppetWidget is created and its seqno is set to 0. TabParent is unaware of this and still holds the last seqno of the recently destroyed PuppetWdiget.

Here is an alternate patch as I suggested in comment 9. Take your pick.

Comment 16

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

We're hitting a path that creates a new window for (a) documentviewer(s) every time we load a new page, which isn't the behavior I saw last time I was in this code.  I'm not sure what's changed in the mean time, or if my earlier investigation was wrong, but since it's apparently mostly holding together except for this bug that's nothing for b3.

Comment 17

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

The puppetwidget being recreated is a red herring; it doesn't materially change the logic of this code.

I missed the analysis in comment 2 and comment 3, which sounds plausible.  If that's on the right track, then I'm missing the reason that either of these patches should reliably fix the problem.

It sounds like the underlying issue is that with a pre-filled text box, we need to atomically notify focus-change and update the caret position.  So it seems like the fix is to make the logic here http://mxr.mozilla.org/mozilla-central/source/widget/src/xpwidgets/PuppetWidget.cpp#399 send the text-hint and selection-change information along atomically with the focus notification.

Brad, if your patches fix this bug in another way that has the same effect, please document how, because it's not obvious to me.

Comment 18

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

Ugh, my understanding of this code had bitrotted.  I disconnected by forgetting that the child synchronizes on *blur*, not focus.

The logic here relies on the assumptions
 - parent starts with seqno 0
 - child starts blurred, so lastBlurSeqno=0
 - child sees every IME event sent by the parent, even if some events are discarded

When the puppetwidget is unexpectedly recreated, then two invariants are violated
 - lastBlurSeqno is wrong: there's an implicit blur on widget creation.  This could result in stale IME events sent parent->child being spurious dispatched in the child when they should have been ignored.
 - lastRecvdSeqno is wrong: the new widget missed all the events seen by the previous widgets.  This could result in not-stale IME events being discarded by the parent, and is the most likely cause of the symptoms here.

It would be nice to fix up the unexpected widget recreation in the content process, but that's too risky for b3, and we probably ought to handle that anyway.  So, I think the closest to "right" fix here is to make the newly created puppetwidget pick up at the state in which the old left off.  The second patch above ends up having a similar effect, but the TabParent code is still correct so I'd rather patch this in the child.  In the PuppetWidget ctor, I'd prefer

  SendNotifyIMEFocus(false, &chromeSeqno));
  mLastBlur = mLastRecvd = chromeSeqno;

I hope that we maintain the invariant that we drop IME focus when navigating to a new page, but this edit would guard us against other code messing that up.

Also, I think Matt was onto a second bug lurking here.  I think that in PuppetWidget::OnIMEFocusChange(), we need to send the result of SendNotifyIMEFocus() atomically with the result of the immediately-after OnIMETextChange(), which can end up sending TextHint and TextChange.  Otherwise an IME event sent from chrome could race with those results and result in those being discarded in the parent.  If that happened, we would see the same symptoms as the above bug.  I'd like to fix that too, but I guess it doesn't need to block b3 since it seems fairly improbable that it would happen in the wild.  Followup suits me.

Comment 19

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

Comment on attachment 494468 [details] [diff] [review]
alternative patch

This ought to work, but I'd prefer the alternate in comment 18, which is a smaller patch too.

Comment 20

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

Actually, attachment 494468 [details] [diff] [review] would allow stale IME events to be dispatched in the content process, so it's not quite right.

Comment 21

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

Attachment: patch

note, I though it would be better to do this work in Create() rather than the constructor.

Comment 22

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

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

Create() is fine, doesn't end up mattering here.

ALOG() won't compile on non-android, needs to go.

Follow bug plz for second NotifyFocus() issue.

Comment 23

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

Attachment: patch for checkin

Comment 24

[Michael Wu [:mwu]]

http://hg.mozilla.org/mozilla-central/rev/bcf68bd9321d

Comment 25

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

(In reply to comment #22)
> Follow bug plz for second NotifyFocus() issue.

filed bug 616404