Closed Bug 615208 Opened 14 years ago Closed 14 years ago

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


(Core Graveyard :: Widget: Android, defect)

Not set



Tracking Status
fennec 2.0b3+ ---


(Reporter: mbrubeck, Assigned: blassey)




(Whiteboard: [hkb] [vkb])


(1 file, 4 obsolete files)

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.
tracking-fennec: --- → ?
tracking-fennec: ? → 2.0b3+
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...
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.
Now I'm not sure how much of this is new, and how much is just a duplicate of bug 602013.
Attached patch patch (obsolete) — Splinter Review
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
Attachment #493888 - Flags: feedback?(mbrubeck)
Comment on attachment 493888 [details] [diff] [review]

With this patch applied to m-c trunk (and no other patches), I am not seeing any buggy behavior.
Attachment #493888 - Flags: feedback?(mbrubeck) → feedback+
Attached patch patch (obsolete) — Splinter Review
Assignee: mbrubeck → blassey.bugs
Attachment #493888 - Attachment is obsolete: true
Attachment #493897 - Flags: review?(jones.chris.g)
Attachment #493897 - Flags: feedback+
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!).
(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.
another potential fix would be to send a message to the TabParent to reset its seqno whenever the PuppetWidget is created.
(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?
(Except after initial TabChild creation, when a temporary about:blank document viewer is created until the real document viewer is.)
here's what I'm seeing without interpretation:
1) load
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.
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 on attachment 493897 [details] [diff] [review]

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.
Attachment #493897 - Flags: review?(jones.chris.g) → review-
Attached patch alternative patch (obsolete) — Splinter Review
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.
Attachment #494468 - Flags: review?(jones.chris.g)
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.
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 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.
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 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.
Attachment #494468 - Flags: review?(jones.chris.g)
Actually, attachment 494468 [details] [diff] [review] would allow stale IME events to be dispatched in the content process, so it's not quite right.
Attached patch patch (obsolete) — Splinter Review
note, I though it would be better to do this work in Create() rather than the constructor.
Attachment #493897 - Attachment is obsolete: true
Attachment #494468 - Attachment is obsolete: true
Attachment #494787 - Flags: review?(jones.chris.g)
Comment on attachment 494787 [details] [diff] [review]

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.
Attachment #494787 - Flags: review?(jones.chris.g) → review+
Attachment #494787 - Attachment is obsolete: true
Attachment #494828 - Flags: review+
Attachment #494828 - Attachment description: m-b patch for checkin → patch for checkin
Closed: 14 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
(In reply to comment #22)
> Follow bug plz for second NotifyFocus() issue.

filed bug 616404
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.