Closed Bug 481950 Opened 15 years ago Closed 15 years ago

Entering caps or special characters using hardware keyboard in password fields is broken

Categories

(Core :: Widget: Gtk, defect)

Other
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED
Tracking Status
status1.9.2 --- beta1-fixed
fennec 1.0+ ---

People

(Reporter: anders, Assigned: dougt)

References

()

Details

(Whiteboard: [See comment 10 for steps to reproduce])

Attachments

(1 file, 4 obsolete files)

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
Still a problem in beta1 (final)
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.
Status: UNCONFIRMED → NEW
tracking-fennec: --- → 1.0b2+
Ever confirmed: true
Antonio - Is this related to the other IME bug you fixed?

It might even be fixed. I need to try to reproduce it.
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
Summary: The key for changing between small letters and large letters are not working in fennec → Entering Caps or special characters in password fields is broken
Summary: Entering Caps or special characters in password fields is broken → Entering caps or special characters in password fields is broken
Actually, bug 480306 fixed this issue for the virtual keyboard. Only the hardware keyboard has issues now.
Summary: Entering caps or special characters in password fields is broken → Entering caps or special characters using hardware keyboard in password fields is broken
Assignee: nobody → tonikitoo
investigating
Status: NEW → ASSIGNED
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
hum ... maybe ignore my comment above (comment #8). I was not actually testing the passwd field. but still, mark how are you testing it ?
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)
Whiteboard: [See comment 10 for steps to reproduce]
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.
tracking-fennec: 1.0b2+ → ?
I experienced this today as well.  If we hit this every release, I suspect our beta users are running into it as well.
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.
tracking-fennec: ? → 1.0+
Attached patch patch for hildon v.1 (obsolete) — Splinter Review
Assignee: tonikitoo → doug.turner
Attachment #395517 - Flags: review?
Attachment #395517 - Flags: review? → review?(masayuki)
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??)
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.
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.
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.
Attached patch patch v.2 (obsolete) — Splinter Review
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().
Attachment #395517 - Attachment is obsolete: true
Attachment #395602 - Flags: review?(masayuki)
Attachment #395517 - Flags: review?(masayuki)
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.
(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.
Attached patch patch v.3 (obsolete) — Splinter Review
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?
Attachment #395602 - Attachment is obsolete: true
Attachment #395602 - Flags: review?(masayuki)
Attachment #395671 - Flags: review?(masayuki)
(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.
> 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...
Attached patch patch v.4 (obsolete) — Splinter Review
Attachment #395671 - Attachment is obsolete: true
Attachment #395766 - Flags: review?(masayuki)
Attachment #395671 - Flags: review?(masayuki)
> +            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
Attached patch patch v.5Splinter Review
hoping that this is it! :-)
Attachment #395766 - Attachment is obsolete: true
Attachment #395775 - Flags: review?(masayuki)
Attachment #395766 - Flags: review?(masayuki)
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!
Attachment #395775 - Flags: review?(masayuki) → review+
No, thank you for your guidance!


http://hg.mozilla.org/mozilla-central/rev/4c13f13c8e52
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee: doug.turner → nobody
Component: General → Widget: Gtk
Product: Fennec → Core
QA Contact: general → gtk
Attachment #395775 - Flags: approval1.9.2?
verified with beta3
Status: RESOLVED → VERIFIED
Attachment #395775 - Flags: approval1.9.2? → approval1.9.2+
Assignee: nobody → doug.turner
You need to log in before you can comment on or make changes to this bug.