Last Comment Bug 747492 - Flash can open VKB even when the plugin element does not have focus
: Flash can open VKB even when the plugin element does not have focus
Status: VERIFIED FIXED
VKB
: flashplayer
Product: Firefox for Android
Classification: Client Software
Component: Keyboards and IME (show other bugs)
: unspecified
: ARM Android
: -- normal (vote)
: Firefox 15
Assigned To: James Willcox (:snorp) (jwillcox@mozilla.com)
:
Mentors:
http://www.zasmakujegancji.pl/
: 750229 750899 757218 (view as bug list)
Depends on: 707353
Blocks: 757218
  Show dependency treegraph
 
Reported: 2012-04-20 11:49 PDT by Chris Peterson [:cpeterson]
Modified: 2016-07-29 14:24 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
verified
fixed
betaN+


Attachments
Don't show vkb when plugin is focused on Android (1.67 KB, patch)
2012-05-23 13:11 PDT, James Willcox (:snorp) (jwillcox@mozilla.com)
cpeterson: review+
blassey.bugs: approval‑mozilla‑beta+
Details | Diff | Splinter Review

Description Chris Peterson [:cpeterson] 2012-04-20 11:49:36 PDT
STR:
1. (After bug 707353's showKeyboard patch lands), load http://www.zasmakujegancji.pl/

AR:
The VKB will open immediately.

ER:
The stock browser does not open the VKB until after the user taps input focus to Flash text box.
Comment 1 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-04-25 05:56:27 PDT
When tapping on http://people.mozilla.org/~mwargers/tests/plugins/flash/317375_flash_wmode.swf , I get to see the vkb too in current trunk build. That's not what the stock browser is doing.
Is that also covered by this bug?
Comment 2 Chris Peterson [:cpeterson] 2012-04-25 11:35:13 PDT
mw22, is that Flash SWF file _supposed_ to open the VKB? If not, that sounds like a new bug (likely a regression from bug 707353).

Maybe Flash SWF files loaded directly with a .swf URL (instead of an <embed> tag) automatically have input focus? I wonder if that is the problem with zasmakujegancji.pl, too.
Comment 3 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-04-26 02:47:00 PDT
(In reply to Chris Peterson (:cpeterson) from comment #2)
> mw22, is that Flash SWF file _supposed_ to open the VKB? If not, that sounds
> like a new bug (likely a regression from bug 707353).

No, it isn't supposed to, I filed now bug 749120 for it.
Comment 4 Aaron Train [:aaronmt] 2012-04-30 07:28:15 PDT
*** Bug 750229 has been marked as a duplicate of this bug. ***
Comment 5 Aaron Train [:aaronmt] 2012-04-30 07:29:25 PDT
Adrian found another test-case with the URL: http://studio1290.com/emphatic-put-down-the-drink-studio1290/
Comment 6 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-05-01 16:02:37 PDT
*** Bug 750899 has been marked as a duplicate of this bug. ***
Comment 7 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-05-02 13:27:12 PDT
It seems that Flash inside iframes is triggering the VKB: http://people.mozilla.org/~mwargers/tests/plugins/flash/iframe_flash.html
Comment 8 Chris Peterson [:cpeterson] 2012-05-08 13:25:14 PDT
I don't know if this is a different bug, but I see a VKB regression when viewing zombo.com. With Nightly 2012-05-03, there is no VKB (correct behavior), but with Nightly 2012-05-04, the VKB opens automatically (fail).

Here is there changeset regression range:
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=2db9df42823d&tochange=9ebf3dc839c5
Comment 9 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-05-23 13:11:18 PDT
Created attachment 626573 [details] [diff] [review]
Don't show vkb when plugin is focused on Android
Comment 10 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-05-23 13:15:54 PDT
A little more explanation is needed. When a plugin is focused, nsIMEStateManager sends calls nsWindow::SetInputContext with IMEState::PLUGIN for mEnabled and IMEState::OPEN_STATE_NOT_SUPPORTED for mOpen. We only pass the value of mEnabled up to GeckoInputConnection. If it is anything other than DISABLED, we show the vkb. That's why it shows up whenever you focus a plugin (which occurs when you tap it or enable it via tap-to-click).

In ANPSystem, Flash will explicit tells us whether or not it wants to have the vkb open or closed. To open it, we use IMEState::PLUGIN and IMEState::OPEN. This hack makes it so we only show the vkb when that condition occurs.
Comment 11 Chris Peterson [:cpeterson] 2012-05-23 14:21:51 PDT
Comment on attachment 626573 [details] [diff] [review]
Don't show vkb when plugin is focused on Android

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

LGTM with nits.

::: widget/android/nsWindow.cpp
@@ +2086,5 @@
>  
> +    int enabled = int(aContext.mIMEState.mEnabled);
> +
> +    // Only show the virtual keyboard for plugins if mOpen is set appropriately.
> +    // This avoids showing it whenver a plugin is focused. Bug 747492

s/whenver/whenever/ :)

@@ +2088,5 @@
> +
> +    // Only show the virtual keyboard for plugins if mOpen is set appropriately.
> +    // This avoids showing it whenver a plugin is focused. Bug 747492
> +    if (aContext.mIMEState.mEnabled == IMEState::PLUGIN &&
> +        aContext.mIMEState.mOpen != IMEState::OPEN) {

Since the mEnabled/mOpen state combinations are so confusing, maybe we should add some asserts to make sure our assumptions don't change. If IMEState::PLUGIN, maybe something like:

    MOZ_ASSERT(mOpen == IMEState::OPEN || mOpen == IMEState::OPEN_STATE_NOT_SUPPORTED);

Adding asserts for the non-plugin states would be helpful, but are not within the scope of this fix.
Comment 12 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-05-24 07:42:49 PDT
(In reply to Chris Peterson (:cpeterson) from comment #11)
> Comment on attachment 626573 [details] [diff] [review]
> Don't show vkb when plugin is focused on Android
> 
> Review of attachment 626573 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> LGTM with nits.
> 
> ::: widget/android/nsWindow.cpp
> @@ +2086,5 @@
> >  
> > +    int enabled = int(aContext.mIMEState.mEnabled);
> > +
> > +    // Only show the virtual keyboard for plugins if mOpen is set appropriately.
> > +    // This avoids showing it whenver a plugin is focused. Bug 747492
> 
> s/whenver/whenever/ :)

Thanks, fixed.

> 
> @@ +2088,5 @@
> > +
> > +    // Only show the virtual keyboard for plugins if mOpen is set appropriately.
> > +    // This avoids showing it whenver a plugin is focused. Bug 747492
> > +    if (aContext.mIMEState.mEnabled == IMEState::PLUGIN &&
> > +        aContext.mIMEState.mOpen != IMEState::OPEN) {
> 
> Since the mEnabled/mOpen state combinations are so confusing, maybe we
> should add some asserts to make sure our assumptions don't change. If
> IMEState::PLUGIN, maybe something like:
> 
>     MOZ_ASSERT(mOpen == IMEState::OPEN || mOpen ==
> IMEState::OPEN_STATE_NOT_SUPPORTED);
> 
> Adding asserts for the non-plugin states would be helpful, but are not
> within the scope of this fix.

Well mOpened can be OPEN, CLOSED, (in response to Flash requests) or OPEN_STATE_NOT_SUPPORTED (focused) when mEnabled is PLUGIN. So I don't know that the assert for PLUGIN really buys us a lot. Adding one for the other states makes more sense to me.
Comment 13 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-05-24 07:54:37 PDT
Upon testing some more, we also get DISABLED and OPEN together (!). So I think I'm going to hold off on adding any assertions because it will just look crazy. We do need to get this sorted out in a better way, though.
Comment 14 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-05-24 08:05:06 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/00b634ca080f
Comment 15 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-05-24 08:12:00 PDT
Filed bug 758226 to address the wonkyness with IME states
Comment 16 :Ms2ger (⌚ UTC+1/+2) 2012-05-25 08:29:32 PDT
https://hg.mozilla.org/mozilla-central/rev/00b634ca080f
Comment 17 Aaron Train [:aaronmt] 2012-05-25 10:39:25 PDT
Aurora nom?
Comment 18 Catalin Suciu [:csuciu] 2012-05-30 04:41:39 PDT
Verified fixed on:
Nightly 15.0a1 (2012-05-29) 
Device: Galaxy SII (Android 2.3.4)

Will this be nominated for Aurora?
Comment 19 Kevin Brosnan [:kbrosnan] 2012-05-31 19:56:14 PDT
*** Bug 757218 has been marked as a duplicate of this bug. ***
Comment 20 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-06-07 15:43:00 PDT
Verified Nightly 06/06/2012 on Galaxy S II
Comment 21 James Willcox (:snorp) (jwillcox@mozilla.com) 2012-06-14 08:40:28 PDT
Comment on attachment 626573 [details] [diff] [review]
Don't show vkb when plugin is focused on Android

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 
User impact if declined: 
Testing completed (on m-c, etc.): 
Risk to taking this patch (and alternatives if risky): 
String or UUID changes made by this patch:
Comment 23 Catalin Suciu [:csuciu] 2012-06-15 06:08:36 PDT
Unable to reproduce on:
Beta 14.0a7 Build 3
Device: Galaxy SII (Android 2.3.4)
Comment 24 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-06-15 09:53:22 PDT
Verified for 14 b7 candidate 3 on Samsung Galaxy S II and Samsung Galaxy Nexus

Note You need to log in before you can comment on or make changes to this bug.