Bug 34285 - [GTK][WPE] Password field doesn't get input method
Summary: [GTK][WPE] Password field doesn't get input method
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P2 Normal
Assignee: Christian Dywan
URL:
Keywords: InRadar
Depends on: 205605
Blocks:
  Show dependency treegraph
 
Reported: 2010-01-28 15:01 PST by Jaroslav Klaus
Modified: 2020-01-16 06:45 PST (History)
17 users (show)

See Also:


Attachments
Allow input methods in password input elements (1.84 KB, patch)
2011-02-10 05:22 PST, Christian Dywan
no flags Details | Formatted Diff | Diff
Allow input methods in password input elements, #2 (2.00 KB, patch)
2011-02-11 06:50 PST, Christian Dywan
mrobinson: review-
Details | Formatted Diff | Diff
Patch (3.56 KB, patch)
2019-12-27 08:32 PST, Carlos Garcia Campos
no flags Details | Formatted Diff | Diff
Rebased patch (3.48 KB, patch)
2020-01-10 05:25 PST, Carlos Garcia Campos
zan: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Jaroslav Klaus 2010-01-28 15:01:14 PST
Is it correct password field doesn't get input method?

In FocusController::setFocusedNode there is 
  m_page->editorClient()->setInputMethodState(node->shouldUseInputMethod());

while 
bool HTMLInputElement::shouldUseInputMethod() const
{
    return m_type == TEXT || m_type == SEARCH || m_type == ISINDEX;
}

So for password field setInputMethodState is always called with false that is problem for on-screen-keyboard.

Can you consider to extend  shouldUseInputMethod  to PASSWORD type? Thx
Comment 1 Alexey Proskuryakov 2010-01-28 17:12:50 PST
What platform is this report about? WebKit on Mac completely ignores setInputMethodState() calls, so this cannot affect Safari.
Comment 2 Jaroslav Klaus 2010-01-28 17:15:25 PST
PC/linux. I've jusr updated the bug record.
Comment 3 Jaroslav Klaus 2010-01-28 17:17:16 PST
webkit gtk
Comment 4 Brian Tarricone 2010-02-15 20:11:47 PST
Yes, changing that line does indeed fix it for the gtk port:

--- a/WebCore/html/HTMLInputElement.cpp
+++ b/WebCore/html/HTMLInputElement.cpp
@@ -523,7 +523,7 @@ void HTMLInputElement::aboutToUnload()
 
 bool HTMLInputElement::shouldUseInputMethod() const
 {
-    return m_type == TEXT || m_type == SEARCH || m_type == ISINDEX;
+    return m_type == TEXT || m_type == SEARCH || m_type == ISINDEX || m_type == PASSWORD;
 }
 
 void HTMLInputElement::handleFocusEvent()
Comment 5 Alexey Proskuryakov 2010-02-15 21:35:12 PST
It's really confusing that cross-platform code in WebCore only affects some platforms, and is even against conventions of other ones.
Comment 6 Christian Dywan 2011-02-10 05:22:20 PST
Created attachment 81956 [details]
Allow input methods in password input elements

This patch enables input methods in password entries. There may be security concerns, but it doesn't help if I want to use WebKit on a device without a physical way to type the password.

If there is strong concern, I can modify the patch to make this a setting.
Comment 7 Alexey Proskuryakov 2011-02-10 09:21:39 PST
This needs:

#if PLATFORM(MAC)
    // Platform convention is to only allow Roman characters in passwords.
    ASSERT_NOT_REACHED();
#endif

I would really prefer a deeper refactoring that wouldn't leave us with Gtk specific code in HTMLInputElement.

+    // Arguably insecure. In practise required if the password must be

We normally use American spelling in WebKit (i.e. "practice").
Comment 8 Christian Dywan 2011-02-11 06:50:33 PST
Created attachment 82130 [details]
Allow input methods in password input elements, #2

(In reply to comment #7)
> #if PLATFORM(MAC)
>     // Platform convention is to only allow Roman characters in passwords.

This is not just about non-Latin characters, but also virtual keyboards, thusly I made it:

// Platform convention is to disallow input methods for passwords.

> I would really prefer a deeper refactoring that wouldn't leave us with Gtk specific code in HTMLInputElement.

I don't have a Mac and I'm probably not a good person to judge how it would need to be changed. From a quick look I didn't even see password handling at all, so I assume it relies on system behaviour to a good extend.

> +    // Arguably insecure. In practise required if the password must be
> 
> We normally use American spelling in WebKit (i.e. "practice").

Corrected.
Comment 9 Alexey Proskuryakov 2011-02-11 08:43:54 PST
> This is not just about non-Latin characters, but also virtual keyboards, thusly I made it:

Please let me reiterate that virtual keyboard is not an input method on Mac, and works with password fields just great. Generally, it sounds more like an exception than a rule that on-screen keyboards would use input method machinery.

Your new comment seems fine though.

> I don't have a Mac

I've just noticed that even though this function has no effect whatsoever on Mac, it does affect Safari for Windows, quite possibly in a bad way. Thus, deferring to Adam or Enrica for review.
Comment 10 Adam Roben (:aroben) 2011-02-11 08:46:39 PST
I guess someone should test on Windows to see what the platform convention is. What do password fields in the OS do? What do other browsers do? I'm not familiar with input methods, so I don't know how to perform this testing myself.
Comment 11 Martin Robinson 2011-02-17 15:19:27 PST
Comment on attachment 82130 [details]
Allow input methods in password input elements, #2

View in context: https://bugs.webkit.org/attachment.cgi?id=82130&action=review

I believe that in Chromium, they only allow simple input method events.

> Source/WebCore/html/PasswordInputType.cpp:67
> +#if PLATFORM(MAC)
> +    // Platform convention is to disallow input methods for passwords.
> +    ASSERT_NOT_REACHED();
> +#endif

This seems wrong. Why should PLATFORM(MAC) ASSERT_NOT_REACHED() ?

> Source/WebCore/html/PasswordInputType.cpp:70
> +    return true;

If we going to do this, we probably only want to do it for PLATFORM(GTK).
Comment 12 Christian Dywan 2011-02-18 02:13:51 PST
(In reply to comment #11)
> > Source/WebCore/html/PasswordInputType.cpp:67
> > +#if PLATFORM(MAC)
> > +    // Platform convention is to disallow input methods for passwords.
> > +    ASSERT_NOT_REACHED();
> > +#endif
> 
> This seems wrong. Why should PLATFORM(MAC) ASSERT_NOT_REACHED() ?
> 
> > Source/WebCore/html/PasswordInputType.cpp:70
> > +    return true;
> 
> If we going to do this, we probably only want to do it for PLATFORM(GTK).

Please see the previous comments, I did exactly what you are suggesting originally and the feedback was to do it for all ports except Mac which has a different mechanism.
Comment 13 Simon Schampijer 2012-11-14 06:14:33 PST
Hmm, this one is important for OLPC (who uses webkitgtk). At the moment you can not log into any page that has a passoword field using the on screen keyboard.

Can the patch provided on this ticket be revised another time and included in some form? There seem to have been mostly agreement and things seems stuck in the last 10% :/

For reference: http://bugs.sugarlabs.org/ticket/4069
Comment 14 Simon Schampijer 2012-11-18 09:28:05 PST
Actually, this affects Epiphany as well. So Sugar and Epiphany would both profit from a fix here.
Comment 15 Carlos Garcia Campos 2019-12-27 08:21:55 PST
(In reply to Martin Robinson from comment #11)
> Comment on attachment 82130 [details]
> Allow input methods in password input elements, #2
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=82130&action=review
> 
> I believe that in Chromium, they only allow simple input method events.

Both chromium and firefox allow input methods in password fields.
Comment 16 Carlos Garcia Campos 2019-12-27 08:25:00 PST
After bug #205605 we will notify the input methods about the editable purpose, so for password fields we will set the purpose as WEBKIT_INPUT_PURPOSE_PASSWORD. That way the input methods (including virtual keyboards) can ensure the characters won't be visible and be prepared for a password input.
Comment 17 Carlos Garcia Campos 2019-12-27 08:32:34 PST
Created attachment 386446 [details]
Patch

It won't apply, since it depends on bug #205605
Comment 18 Carlos Garcia Campos 2020-01-10 05:25:32 PST
Created attachment 387330 [details]
Rebased patch
Comment 19 Carlos Garcia Campos 2020-01-16 06:44:26 PST
Committed r254675: <https://trac.webkit.org/changeset/254675>
Comment 20 Radar WebKit Bug Importer 2020-01-16 06:45:15 PST
<rdar://problem/58642844>