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)
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)
2.71 KB,
patch
|
masayuki
:
review+
pavlov
:
approval1.9.2+
|
Details | Diff | Splinter Review |
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
Reporter | ||
Comment 1•15 years ago
|
||
Still a problem in beta1 (final)
Comment 2•15 years ago
|
||
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
Comment 3•15 years ago
|
||
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•15 years ago
|
||
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
Updated•15 years ago
|
Summary: Entering Caps or special characters in password fields is broken → Entering caps or special characters in password fields is broken
Comment 6•15 years ago
|
||
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
Updated•15 years ago
|
Assignee: nobody → tonikitoo
Comment 8•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
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]
Comment 12•15 years ago
|
||
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+ → ?
Comment 13•15 years ago
|
||
I experienced this today as well. If we hit this every release, I suspect our beta users are running into it as well.
Reporter | ||
Comment 14•15 years ago
|
||
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.
Updated•15 years ago
|
tracking-fennec: ? → 1.0+
Assignee | ||
Comment 15•15 years ago
|
||
Assignee: tonikitoo → doug.turner
Attachment #395517 -
Flags: review?
Assignee | ||
Updated•15 years ago
|
Attachment #395517 -
Flags: review? → review?(masayuki)
Comment 16•15 years ago
|
||
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??)
Assignee | ||
Comment 17•15 years ago
|
||
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•15 years ago
|
||
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•15 years ago
|
||
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.
Assignee | ||
Comment 20•15 years ago
|
||
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 21•15 years ago
|
||
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•15 years ago
|
||
(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.
Assignee | ||
Comment 23•15 years ago
|
||
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)
Assignee | ||
Updated•15 years ago
|
Attachment #395671 -
Flags: review?(masayuki)
Comment 24•15 years ago
|
||
(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.
Assignee | ||
Comment 25•15 years ago
|
||
> 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...
Assignee | ||
Comment 26•15 years ago
|
||
Attachment #395671 -
Attachment is obsolete: true
Attachment #395766 -
Flags: review?(masayuki)
Attachment #395671 -
Flags: review?(masayuki)
Comment 27•15 years ago
|
||
> + 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
Assignee | ||
Comment 28•15 years ago
|
||
hoping that this is it! :-)
Attachment #395766 -
Attachment is obsolete: true
Attachment #395775 -
Flags: review?(masayuki)
Attachment #395766 -
Flags: review?(masayuki)
Comment 29•15 years ago
|
||
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+
Assignee | ||
Comment 30•15 years ago
|
||
No, thank you for your guidance! http://hg.mozilla.org/mozilla-central/rev/4c13f13c8e52
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•15 years ago
|
Assignee: doug.turner → nobody
Component: General → Widget: Gtk
Product: Fennec → Core
QA Contact: general → gtk
Assignee | ||
Updated•15 years ago
|
Attachment #395775 -
Flags: approval1.9.2?
Updated•15 years ago
|
Attachment #395775 -
Flags: approval1.9.2? → approval1.9.2+
Assignee | ||
Comment 32•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/cdb9eaae06f1
Keywords: fixed1.9.2
Updated•15 years ago
|
status1.9.2:
--- → beta1-fixed
Keywords: fixed1.9.2
Updated•13 years ago
|
Assignee: nobody → doug.turner
You need to log in
before you can comment on or make changes to this bug.
Description
•