Last Comment Bug 481950 - Entering caps or special characters using hardware keyboard in password fields is broken
: Entering caps or special characters using hardware keyboard in password field...
Status: VERIFIED FIXED
[See comment 10 for steps to reproduce]
:
Product: Core
Classification: Components
Component: Widget: Gtk (show other bugs)
: Trunk
: Other Linux
: -- normal (vote)
: ---
Assigned To: Doug Turner (:dougt)
:
Mentors:
http://people.mozilla.org/~mfinkle/pa...
: 457829 471160 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2009-03-06 14:39 PST by Anders Rune Jensen
Modified: 2011-06-11 04:11 PDT (History)
10 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
beta1-fixed
1.0+


Attachments
patch for hildon v.1 (1.50 KB, patch)
2009-08-19 22:40 PDT, Doug Turner (:dougt)
no flags Details | Diff | Review
patch v.2 (3.62 KB, patch)
2009-08-20 09:59 PDT, Doug Turner (:dougt)
no flags Details | Diff | Review
patch v.3 (2.54 KB, patch)
2009-08-20 14:02 PDT, Doug Turner (:dougt)
no flags Details | Diff | Review
patch v.4 (2.68 KB, patch)
2009-08-20 20:38 PDT, Doug Turner (:dougt)
no flags Details | Diff | Review
patch v.5 (2.71 KB, patch)
2009-08-20 21:11 PDT, Doug Turner (:dougt)
masayuki: review+
pavlov: approval1.9.2+
Details | Diff | Review

Description Anders Rune Jensen 2009-03-06 14:39:18 PST
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.0.7) Gecko/2009030422 Ubuntu/8.10 (intrepid) Firefox/3.0.7
Build Identifier: fennec 1.0b1

This makes it very hard to enter password if your password has a capital in it :)

Installed on n810 using this:

ftp://ftp.mozilla.org/pub/mobile/nightly/

xulrunner is: xulrunner_1.9.2a1pre-20090306020837_armel.deb

Reproducible: Always
Comment 1 Anders Rune Jensen 2009-03-20 02:11:33 PDT
Still a problem in beta1 (final)
Comment 2 Christian Sejersen 2009-03-20 04:25:32 PDT
I can confirm this as well. There is a workaround as it seems like you just can't change input state in a password field, but you can change it in another field and then put focus back to the password field.
Comment 3 Mark Finkle (:mfinkle) (use needinfo?) 2009-05-19 13:23:20 PDT
Antonio - Is this related to the other IME bug you fixed?

It might even be fixed. I need to try to reproduce it.
Comment 4 Mark Finkle (:mfinkle) (use needinfo?) 2009-05-20 21:33:52 PDT
I tested using web form password fields and HTTP Auth dialog password field, the results are the same:

virtual kb -> no uppercase, lowercase, or special letter can be entered at all
hardware kb -> lowercase letters can be entered, but "shift" and "fn" modifiers do not work
Comment 5 Mark Finkle (:mfinkle) (use needinfo?) 2009-05-20 21:34:48 PDT
*** Bug 457829 has been marked as a duplicate of this bug. ***
Comment 6 Mark Finkle (:mfinkle) (use needinfo?) 2009-05-20 22:00:02 PDT
Actually, bug 480306 fixed this issue for the virtual keyboard. Only the hardware keyboard has issues now.
Comment 7 Antonio Gomes (tonikitoo) 2009-06-15 12:11:36 PDT
investigating
Comment 8 Antonio Gomes (tonikitoo) 2009-06-28 10:07:31 PDT
mark, just tested w/ today's build. Results:

- test page: http://timeless.justdave.net/maemo/realm/picture.html

- vkb - upper, lower and special chars *can* be entered fine
- hkb - idem (including "fn" and "shifted").

mark, could you please tell me if you get the same in this page, and how you are testing (to get results in comment #4) ?




(In reply to comment #4)
> I tested using web form password fields and HTTP Auth dialog password field,
> the results are the same:
> 
> virtual kb -> no uppercase, lowercase, or special letter can be entered at all
> hardware kb -> lowercase letters can be entered, but "shift" and "fn" modifiers
> do not work
Comment 9 Antonio Gomes (tonikitoo) 2009-06-28 10:10:22 PDT
hum ... maybe ignore my comment above (comment #8). I was not actually testing the passwd field. but still, mark how are you testing it ?
Comment 10 Antonio Gomes (tonikitoo) 2009-06-28 21:21:56 PDT
from IRC, a detailed description about the real bug:

<mfinkle> http://people.mozilla.org/~mfinkle/password.html
<mfinkle> it's a simple page with a text and password input
<mfinkle> type into either and the text is reflected into a span so you can read it
<mfinkle> in the password field, the soft kb CAPS button will "stick", as it should but the hard kb will not
<mfinkle> I _can_ type caps and numbers via the hard kb _if_ I press the shift or fn key while pressing the other key but the shift and fn keys do not "stick" or change the mode
<mfinkle> they work fine in the plain text field
<mfinkle> so i can press Fn twice and it will "stick" so i can easily enter numbers (for example)
Comment 11 Mark Finkle (:mfinkle) (use needinfo?) 2009-07-02 10:58:15 PDT
*** Bug 471160 has been marked as a duplicate of this bug. ***
Comment 12 cmtalbert 2009-08-10 14:11:20 PDT
What's the status on this bug?  I just ran into this with a 1.0b3pre build, from today (Aug 10) and still encountered it.  I see that it is stated as blocking b2, but since it obviously missed b2, I'm going to set it back to ? to get it triaged for blocking b3+

I think this must block beta 3.
Comment 13 Joel Maher (:jmaher) 2009-08-10 14:14:44 PDT
I experienced this today as well.  If we hit this every release, I suspect our beta users are running into it as well.
Comment 14 Anders Rune Jensen 2009-08-10 14:19:42 PDT
As a user I havn't tried fennec for a long while because this bug is still not fixed. It makes the browser almost completely useless on a n810.
Comment 15 Doug Turner (:dougt) 2009-08-19 22:40:20 PDT
Created attachment 395517 [details] [diff] [review]
patch for hildon v.1
Comment 16 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2009-08-19 23:28:39 PDT
Excuse me, I don't understand well. Is IME used for inputting the passwords on n810? (Or does the our software keyboard need the IME context??)
Comment 17 Doug Turner (:dougt) 2009-08-19 23:56:09 PDT
On the n810, there is a software keyboard, and a hardware keyboard.  My testing is exclusively using the hardware keyboard (the software keyboard is broken for other reasons).  What currently happens is that you can successfully enter uppercase or symbols if you use a key chord (for example, to press an upper case "A", you can hold shift down and press the "a" character on the keyboard).

However, the platform allows you to also press only the shift key and the next input to be in uppercase.  There is a little software widget at the bottom of the screen which shows which state you're in.  (For example, if you press the shift once, you see an "Up" arrow which means you are in caps for the next char.  If you press shift again, you will see another icon which means that you are in cap lock).

So, on this platform, IME is used for passwords.
Comment 18 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2009-08-20 01:07:22 PDT
Thank you for the explaining.

But...

> @@ -6944,8 +6950,11 @@
>      // if gFocusWindow is null, use the last focused gIMEFocusWindow
>      nsRefPtr<nsWindow> window = gFocusWindow ? gFocusWindow : gIMEFocusWindow;
>  
> -    if (!window || IM_get_input_context(window) != aContext &&
> -        !(window->mIMEData && window->mIMEData->mEnabled == nsIWidget::IME_STATUS_PASSWORD)) 
> +    if (!window || IM_get_input_context(window) != aContext
> +#ifndef MOZ_PLATFORM_HILDON
> +        && !(window->mIMEData && window->mIMEData->mEnabled == nsIWidget::IME_STATUS_PASSWORD)
> +#endif
> +        )

This is strange. The additional condition (!(window->mIMEData && ...)) is added by bug 480306. But looks like that this is wrong approach because bug 480306 comment 14 pointed out a good point. Perhaps, that is the actual cause.

By your patch, this condition can be removed? IM_get_input_context(window) and aContext should be the mContext always.
Comment 19 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2009-08-20 03:09:15 PDT
Er, but the simple context uses a simple IME, so, we don't disable the IME perfectly on the password filed of Linux. I have a question:

In nsWindow::SetIMEEnabled:

> 6743     nsRefPtr<nsWindow> focusedWin = gIMEFocusWindow;
> 6744     if (focusedWin && focusedWin->mIMEData)
> 6745         focusedIm = focusedWin->mIMEData->mContext;

We are using non-actual context for the checking. But the hildon only code uses this variable for the APIs:

> 6759 #ifdef MOZ_PLATFORM_HILDON
> 6760         if (mIMEData->mEnabled) {
> 6761             // It is not desired that the hildon's autocomplete mechanism displays
> 6762             // user previous entered passwds, so lets make completions invisible
> 6763             // in these cases.
> 6764             int mode;
> 6765             g_object_get (G_OBJECT(focusedIm), "hildon-input-mode", &mode, NULL);
> 6766 
> 6767             if (mIMEData->mEnabled == IME_STATUS_ENABLED)
> 6768                 mode &= ~HILDON_GTK_INPUT_MODE_INVISIBLE;
> 6769             else if (mIMEData->mEnabled == nsIWidget::IME_STATUS_PASSWORD)
> 6770                mode |= HILDON_GTK_INPUT_MODE_INVISIBLE;
> 6771 
> 6772             g_object_set (G_OBJECT(focusedIm), "hildon-input-mode", (HildonGtkInputMode)mode, NULL);
> 6773             gIMEVirtualKeyboardOpened = PR_TRUE;
> 6774             hildon_gtk_im_context_show (focusedIm);
> 6775         } else {
> 6776             gIMEVirtualKeyboardOpened = PR_FALSE;
> 6777             hildon_gtk_im_context_hide (focusedIm);
> 6778         }
> 6779 #endif

Cannot you fix this mistake, and cannot it fix this bug too? This is just my guess. But I wonder why such wrong code should work as expected. If this is the true cause of this bug, we are creating a spaghetti code.
# And the if state of #6767 is wrong... I hope that Maemo developers request the IME code review to me.
Comment 20 Doug Turner (:dougt) 2009-08-20 09:59:14 PDT
Created attachment 395602 [details] [diff] [review]
patch v.2

I think the important parts of this patch is to ensure that IME is enabled for passwords on hildon and that we use the "full" context (not the mSimpleContext).

I also attempted to clean up SetIMEEnabled().
Comment 21 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2009-08-20 11:19:17 PDT
Comment on attachment 395602 [details] [diff] [review]
patch v.2

ok.

> -    GtkIMContext *focusedIm = nsnull;
>      // XXX Don't we need to check gFocusWindow?
>      nsRefPtr<nsWindow> focusedWin = gIMEFocusWindow;
> -    if (focusedWin && focusedWin->mIMEData)
> -        focusedIm = focusedWin->mIMEData->mContext;
> -
> -    if (focusedIm && focusedIm == mIMEData->mContext) {
> -        // Release current IME focus if IME is enabled.
> -        if (IsIMEEditableState(mIMEData->mEnabled)) {
> -            focusedWin->ResetInputState();
> -            focusedWin->IMELoseFocus();
> -        }
> -
> +
> +    // Release current IME focus if IME is enabled.
> +    if (IsIMEEditableState(mIMEData->mEnabled)) {
> +        focusedWin->ResetInputState();
> +        focusedWin->IMELoseFocus();
> +    }
> +
> +    if (mIMEData->mContext) {

Why do you need this change? This change should break something. E.g., IM_preedit_changed_cb refers gIMEFocusWindow. So, if the window isn't gIMEFocusWindow, the new behavior will be changed.

> -            g_object_get (G_OBJECT(focusedIm), "hildon-input-mode", &mode, NULL);
> -
> +            g_object_get (G_OBJECT(mIMEData->mContext), "hildon-input-mode", &mode, NULL);
> +            

g_object_get (G_OBJECT(IMEGetContext()), "hildon-input-mode", &mode, NULL);

Isn't this better? Actually, this is same as your code, but I think so. And also:

> +            g_object_set (G_OBJECT(mIMEData->mContext), "hildon-input-mode", (HildonGtkInputMode)mode, NULL);

and

> +            hildon_gtk_im_context_hide (mIMEData->mContext);

And this isn't your work, but if you fix this, I'm happy.

>              if (mIMEData->mEnabled == IME_STATUS_ENABLED)
>                  mode &= ~HILDON_GTK_INPUT_MODE_INVISIBLE;
>              else if (mIMEData->mEnabled == nsIWidget::IME_STATUS_PASSWORD)
> -               mode |= HILDON_GTK_INPUT_MODE_INVISIBLE;

This should be:

if (IMEIsEnabledState())
    mode &= ...
else if (mIMEData->mEnabled == nsIWidget::IME_STATUS_PASSWORD)
   mode |= ...

So, the current code ignores the plug-in state. E.g., if the plug-in is focused after password field, the IME context still think the focus is in the password filed.

Otherwise, looks good.
Comment 22 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2009-08-20 11:33:51 PDT
(In reply to comment #21)
> And this isn't your work, but if you fix this, I'm happy.
> 
> >              if (mIMEData->mEnabled == IME_STATUS_ENABLED)
> >                  mode &= ~HILDON_GTK_INPUT_MODE_INVISIBLE;
> >              else if (mIMEData->mEnabled == nsIWidget::IME_STATUS_PASSWORD)
> > -               mode |= HILDON_GTK_INPUT_MODE_INVISIBLE;
> 
> This should be:
> 
> if (IMEIsEnabledState())
>     mode &= ...
> else if (mIMEData->mEnabled == nsIWidget::IME_STATUS_PASSWORD)
>    mode |= ...
> 
> So, the current code ignores the plug-in state. E.g., if the plug-in is focused
> after password field, the IME context still think the focus is in the password
> filed.

Oops, you'll change the behavior of IMEIsEnabledState(), sorry.

So, I hope that you'll add nsIWidget::IME_STATUS_PLUGIN checking in if condition.
Comment 23 Doug Turner (:dougt) 2009-08-20 14:02:37 PDT
Created attachment 395671 [details] [diff] [review]
patch v.3

Thank you for your feedback so far.  It has been quite helpful.

I am not sure how best to handle the plugin state.  Should we just not explicitly ask hildon to use the software keyboard when mEnabled equals IME_STATUS_PASSWORD?
Comment 24 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2009-08-20 20:11:54 PDT
(In reply to comment #23)
> Thank you for your feedback so far.  It has been quite helpful.

You're welcome, and thank you for your work too.

> I am not sure how best to handle the plugin state.

The 'plug-in state' means we don't touch to the IME context. If the plug-in needs to change the IME state -- e.g., someone creates password fields on them -- , they should touch to the IME context themselves. I.e., They should call hildon_gtk_im_context_* and set the hildon-input-mode directly.

> Should we just not
> explicitly ask hildon to use the software keyboard when mEnabled equals
> IME_STATUS_PASSWORD?

How about the native password filed of n810? We should use same behavior as far as possible.

> +            if (IMEIsEnabledState())
>                  mode &= ~HILDON_GTK_INPUT_MODE_INVISIBLE;
>              else if (mIMEData->mEnabled == nsIWidget::IME_STATUS_PASSWORD)
>                 mode |= HILDON_GTK_INPUT_MODE_INVISIBLE;

I'm sorry, you're confused by me. Please check comment 22. This should be:

if (mIMEData->mEnabled == IME_STATUS_ENABLED ||
    mIMEData->mEnabled == IME_STATUS_PLUGIN)
...
else if (mIMEData->mEnabled == nsIWidget::IME_STATUS_PASSWORD)
...

Otherwise, great.
Comment 25 Doug Turner (:dougt) 2009-08-20 20:37:29 PDT
> Should we just not
> explicitly ask hildon to use the software keyboard when mEnabled equals
> IME_STATUS_PASSWORD?


^^ ignore me.  i was asking about the IME_STATUS_PLUGIN state which you already explained.  New patch coming up...
Comment 26 Doug Turner (:dougt) 2009-08-20 20:38:58 PDT
Created attachment 395766 [details] [diff] [review]
patch v.4
Comment 27 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2009-08-20 20:48:35 PDT
> +            if (IMEIsEnabledState())
>                  mode &= ~HILDON_GTK_INPUT_MODE_INVISIBLE;
> -            else if (mIMEData->mEnabled == nsIWidget::IME_STATUS_PASSWORD)
> +            else if (mIMEData->mEnabled == nsIWidget::IME_STATUS_PASSWORD ||
> +                     mIMEData->mEnabled == nsIWidget::IME_STATUS_PLUGIN )
>                 mode |= HILDON_GTK_INPUT_MODE_INVISIBLE;

No, IMEIsEnabledState returns true when the state is enabled, plugin and password by your patch (only on Maemo). So, this code means:

if (mIMEData->mEnabled == nsIWidget::IME_STATUS_ENABLED ||
    mIMEData->mEnabled == nsIWidget::IME_STATUS_PASSWORD ||
    mIMEData->mEnabled == nsIWidget::IME_STATUS_PLUGIN)
...
else if (mIMEData->mEnabled == nsIWidget::IME_STATUS_PASSWORD ||
         mIMEData->mEnabled == nsIWidget::IME_STATUS_PLUGIN)

So, the else if block is never used.

I meant the plugin mode should same as enabled mode (this is normal mode). Therefore this code should be the bottom of comment 24.

if (mIMEData->mEnabled == IME_STATUS_ENABLED ||
    mIMEData->mEnabled == IME_STATUS_PLUGIN)
... // non-password field case
else if (mIMEData->mEnabled == nsIWidget::IME_STATUS_PASSWORD)
... // only password filed case
Comment 28 Doug Turner (:dougt) 2009-08-20 21:11:49 PDT
Created attachment 395775 [details] [diff] [review]
patch v.5

hoping that this is it! :-)
Comment 29 Masayuki Nakano [:masayuki] (Mozilla Japan) (working slowly due to injured) 2009-08-20 21:15:27 PDT
Comment on attachment 395775 [details] [diff] [review]
patch v.5

> +                mIMEData->mEnabled == nsIWidget::IME_STATUS_PLUGIN )

else if (mIMEData->mEnabled == nsIWidget::IME_STATUS_PASSWORD )

remove the unneeded whitespaces before ')'.

r=masayuki with them.

Thank you for your work again!
Comment 30 Doug Turner (:dougt) 2009-08-20 21:32:55 PDT
No, thank you for your guidance!


http://hg.mozilla.org/mozilla-central/rev/4c13f13c8e52
Comment 31 Joel Maher (:jmaher) 2009-08-26 08:25:41 PDT
verified with beta3

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