Closed Bug 1175838 Opened 9 years ago Closed 9 years ago

Swift Keyboard broken on www.google.com

Categories

(Firefox for Android Graveyard :: Keyboards and IME, defect)

Product:
Firefox for Android Graveyard
Component:
Keyboards and IME
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox39 unaffected, firefox40 unaffected, firefox41+ fixed)

Status:
RESOLVED DUPLICATE of bug 1177011
Tracking Flags:
Tracking Status
firefox39 --- unaffected
firefox40 --- unaffected
firefox41 + fixed

People

(Reporter: capella, Unassigned)

References

Details

(Keywords: regression)

Attachments

(1 obsolete file) 

Entering a string of text into the www.google.com search editable using the Swift keyboard produces wrong / confusing results on nightly.

See [0] ... I'm simply entering |abcdefg| ... 

I wonder if this is related to recent work for Bug 1162818
Or possibly to followups in bug 549674 comment c17

[0] https://www.dropbox.com/s/p4m4osynvhatljh/maybe_Bug549674.mp4?dl=0
wfm in Beta/39 ... initial regressing / bisect finds range:

   bad  changeset: 238000:119d3c0fd0f6
   good changeset: 235000:052bf572cb94
Thanks! 

I couldn't get the build to install on my N7 (not sure why), so I built against nightly using your 4 changesets.

Unfortunately, I still see the issue  :-/

I'll continue bisecting, and post the results here later.
addl. note: Nightly seems to work fine on my phone/GS3 device ... but if I "Request Desktop Site", then it fails similarly to my N7.
Hmm, odd. According to the video (and the symptom), it looks like same as bug 1162818. However, it's completely fixed and I guess that Google doesn't reframe <input> because it may cause a lot of problems in each browser.

It seems that selection change notification may not be notified as IME expected or IME queries selection range to the TabParent but its cache hasn't been modified by PuppetWidget, yet. I guess that this is the reason of this regression. Isn't the regression cause is bug 1162818?
> Isn't the regression cause is bug 1162818?

Oops, I meant bug 1166436.
Starting from what appears a "good" baseline, and bisecting forward ... by the time I get to changeset 236928:ee5e3a5f27c1 I notice random occurrences of this: 

https://www.dropbox.com/home/BugShots?preview=bug1175838_chgset_236928-ee5e3a5f27c1.mp4

While typing |abcdefgh| the |g| is dropped, the text is "selected", and then the selection is dropped before the |h| is entered.
When I get to changeset 236929:b1b49b2e9529 this becomes reproduceable when typing |abcdefg|

https://www.dropbox.com/home/BugShots?preview=bug1175838_chgset_236929-b1b49b2e9529.mp4
And then on changeset 236930:660f813d551e this is repro'd by typing |ab|

https://www.dropbox.com/home/BugShots?preview=bug1175838_chgset_236930-660f813d551e.mp4
FYI: I've not tested it yet and I'll go to bed soon.
What versions of Fennec are affected?
Flags: needinfo?(markcapella)
Comment on attachment 8624314 [details] [diff] [review]
Possible fix

Review of attachment 8624314 [details] [diff] [review]:
-----------------------------------------------------------------

Steps to test are simply enter the text string |abcdefgh|

Building nightly with the two changesets from comment #10 produces the result [0]
For comparison, included is what I see on Beta 39.0 as delivered by the play store [1]

[0] https://www.dropbox.com/s/0uajglvztuux8az/bug1175838_build_comment10.mp4?dl=0
[1] https://www.dropbox.com/s/3khm8i4sswiekw2/bug1175838_beta_playstore.mp4?dl=0
Attachment #8624314 - Flags: feedback?(markcapella) → feedback+
Beta seems un-affected as downloaded from play store.

Nightly is affected.

I'm assuming Aurora is affected as the chgsets involved were from bug 1149172 comment #8 back around 4/1/15.

I can't confirm because I'm hitting build issues locally switching backwards to aurora, and installing from /pub/mozilla.org is complaining "X App not installed", and I need to step out for a bit :-/
Flags: needinfo?(markcapella)
[Tracking Requested - why for this release]: Regression in common keyboard. Some phones ship with Swype as the default keyboard.
status-firefox39: --- → unaffected
status-firefox40: --- → affected
status-firefox41: --- → affected
tracking-firefox40: --- → ?
tracking-firefox41: --- → ?
Keywords: regression
To be clear, this is regarding SwiftKey Keyboard.

http://www.androidcentral.com/best-android-keyboard#slide2
http://www.cnet.com/how-to/how-to-use-swiftkey-android-ios/

I usually test with: Google default and SwiftKey keyboard.

Sometimes for completeness JellyBean keyboard and Hacker's keyboard.

I've never tried Swype, but just installed it for a look.
Oops SwiftKey keyboard is also a popular keyboard and is the basis for the Samsung keyboard. So amend my comment 15 :P
(In reply to Mark Capella [:capella] from comment #13)
> Comment on attachment 8624314 [details] [diff] [review]
> Possible fix
> 
> Review of attachment 8624314 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Steps to test are simply enter the text string |abcdefgh|
> 
> Building nightly with the two changesets from comment #10 produces the
> result [0]

Thank you for your test.

(In reply to Mark Capella [:capella] from comment #14)
> I'm assuming Aurora is affected as the chgsets involved were from bug
> 1149172 comment #8 back around 4/1/15.

Oh, okay. Some of the changes look odd to me. Especially, attachment 8585527 [details] [diff] [review] is very bad approach because IMEStateManager and TextComposition don't assume that they are used by widget. So, the lifetime of TextComposition shouldn't be used for managing native IME event handling...

And also I don't understand the change of attachment 8585526 [details] [diff] [review]. If ae->Action() isn't AndroidGeckoEvent::IME_COMPOSE_TEXT, it should cause committing composition. However, immediately before the compositionchange event always dispatched as uncommitted string. This is very odd...

Anyway, jchen, could you check your patches?
Flags: needinfo?(nchen)
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #18)
> (In reply to Mark Capella [:capella] from comment #13)
> > Comment on attachment 8624314 [details] [diff] [review]
> > Possible fix
> 
> (In reply to Mark Capella [:capella] from comment #14)
> > I'm assuming Aurora is affected as the chgsets involved were from bug
> > 1149172 comment #8 back around 4/1/15.
> 
> Oh, okay. Some of the changes look odd to me. Especially, attachment 8585527 [details] [diff] [review]
> [details] [diff] [review] is very bad approach because IMEStateManager and
> TextComposition don't assume that they are used by widget. So, the lifetime
> of TextComposition shouldn't be used for managing native IME event
> handling...

We need to determine if content has an active composition or not. The other option is to keep track of NS_COMPOSITION_* events ourselves, which is very prone to error. We also use TextComposition in widget/ContentCache.cpp [1].

[1] mxr.mozilla.org/mozilla-central/source/widget/ContentCache.cpp?rev=2694ff2ace6a#609 

> And also I don't understand the change of attachment 8585526 [details] [diff] [review]
> [diff] [review]. If ae->Action() isn't AndroidGeckoEvent::IME_COMPOSE_TEXT,
> it should cause committing composition. However, immediately before the
> compositionchange event always dispatched as uncommitted string. This is
> very odd...

It looks okay to me. If ae->Action() is AndroidGeckoEvent::IME_COMPOSE_TEXT, we let the composition remain active, so we don't send a commit event. Otherwise, we send a commit event to commit the composition from the compositionchange event.
Flags: needinfo?(nchen)
(In reply to Jim Chen [:jchen] [:darchons] from comment #19)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #18)
> > (In reply to Mark Capella [:capella] from comment #13)
> > > Comment on attachment 8624314 [details] [diff] [review]
> > > Possible fix
> > 
> > (In reply to Mark Capella [:capella] from comment #14)
> > > I'm assuming Aurora is affected as the chgsets involved were from bug
> > > 1149172 comment #8 back around 4/1/15.
> > 
> > Oh, okay. Some of the changes look odd to me. Especially, attachment 8585527 [details] [diff] [review]
> > [details] [diff] [review] is very bad approach because IMEStateManager and
> > TextComposition don't assume that they are used by widget. So, the lifetime
> > of TextComposition shouldn't be used for managing native IME event
> > handling...
> 
> We need to determine if content has an active composition or not. The other
> option is to keep track of NS_COMPOSITION_* events ourselves, which is very
> prone to error. We also use TextComposition in widget/ContentCache.cpp [1].

Because ContentCache is managing both widget state and content state. It does make sense to use TextComposition for the latter case, but not so for the former case. Any classes in dom/ depend on the behavior of other contents. So, it's unsafe for widget. On the other hand, widget side's state isn't useful for content side because it depends on each native IME framework. Anyway, TextEventDispatcher should manage native IME event state. I'll do that in all platforms.

> > And also I don't understand the change of attachment 8585526 [details] [diff] [review]
> > [diff] [review]. If ae->Action() isn't AndroidGeckoEvent::IME_COMPOSE_TEXT,
> > it should cause committing composition. However, immediately before the
> > compositionchange event always dispatched as uncommitted string. This is
> > very odd...
> 
> It looks okay to me. If ae->Action() is AndroidGeckoEvent::IME_COMPOSE_TEXT,
> we let the composition remain active, so we don't send a commit event.
> Otherwise, we send a commit event to commit the composition from the
> compositionchange event.

Redundant event isn't good. It cause some DOM events which may cause anything. But I hope that it should be ignored in TextComposition. Fujisawa-san hacked it in 41.
The patch for bug 1177011 chgset 95af75ab24cd seems to have corrected this also \o/, so I'm closing.

(I just tested on my N7 and the issue is resolved. If I backout that chgset it recurs.)
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
(In reply to Mark Capella [:capella] from comment #14)
> Beta [39] seems un-affected as downloaded from play store.
> 
> Nightly [41] is affected.
> 
> I'm assuming Aurora [40] is affected as the chgsets involved were from bug
> 1149172 comment #8 back around 4/1/15.

Mark, could you test whether 40 (now on the beta channel) is actually affected or not?  Duplicate-bug 1177011 is a new regression in Firefox 41, which contradicts your assumption in comment 14.

If this bug here really does affect Firefox 40 (as you suspected but couldn't confirm in comment 14), then something subtle and weird is going on, and we may need a different patch for 40.
Flags: needinfo?(markcapella)
Beta / 40.0 from the play store seems un-affected.
Flags: needinfo?(markcapella)
Great, thanks. Updating status flags for Firefox 40 then (and setting firefox41 status flag to "fixed" per comment 22.)
status-firefox40: affected → unaffected
status-firefox41: affected → fixed
tracking-firefox40: + → ---
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: