Bug 30023 – [Qt] Software input panel is not launched for password fields

• Component: WebKit Qt
• Status: RESOLVED FIXED

[Joseph Ligman]

SetInputMethodState is false for password input fields. See HTMLInputElement::shouldUseInputMethod() const: returns false for password.

[Joseph Ligman]

Created attachment 40539 [details]
[Qt] patch to make the software input panel work for password fields

The patch adds the check in void EditorClientQt::setInputMethodState(bool active). Adds a ImPasswordInput hint to QWebPage::inputMethodQuery, could something like this be supported?

[Joseph Ligman]

Created attachment 40546 [details]
[Qt] patch to make the software input panel work for password fields

[Joseph Ligman]

Created attachment 40551 [details]
[Qt] patch to make the software input panel work for password fields

Added pointer checks for frame, document, and focusnode.

[Simon Hausmann]

Comment on attachment 40551 [details]
[Qt] patch to make the software input panel work for password fields


> +        case Qt::ImPasswordInput: {
> +            if (frame->selection()->isContentEditable()) {
> +                if (frame->document() && frame->document()->focusedNode()) {
> +                    if (frame->document()->focusedNode()->hasTagName(HTMLNames::inputTag)) {
> +                        HTMLInputElement* inputElement = static_cast<HTMLInputElement*>(frame->document()->focusedNode());
> +                        return QVariant(inputElement->isPasswordField());
> +                    }
> +                }
> +            }
> +            return QVariant(false);

Given QVariant's implicit constructors it should be sufficient to simply "return false;" here :)

> -    if (view)
> +    if (view) {
> +        if (!active) {
> +            Frame* frame = m_page->d->page->focusController()->focusedOrMainFrame();
> +            if (frame && frame->document() && frame->document()->focusedNode()) {
> +                if (frame->document()->focusedNode()->hasTagName(HTMLNames::inputTag)) {
> +                    HTMLInputElement* inputElement = static_cast<HTMLInputElement*>(frame->document()->focusedNode());
> +                    if (inputElement->isPasswordField()) {
> +                        view->setAttribute(Qt::WA_InputMethodEnabled, true);
> +                        emit m_page->microFocusChanged();
> +                        return;

Wouldn't it be simpler to just replace the last three lines there with

    active = true;

Also, please add a comment explaining why we are diverging from the default
behaviour here. (the fact that in Qt we let the input method determine this and that on the desktop
platforms the qt input method layer will not permit composition of these fields, for security)

Given the code duplication I'm wondering if it would be worth to centralize the above duplicated
code to determine if the focused node is a password protected input field into a helper method?


Please also add a unit test for the behaviour of still enabling input methods when
password fields are focused and that only in that case we are also returning the ImPasswordInput
hint only for these fields.

[Joseph Ligman]

Thanks for the comments. 

Do you have any more ideas on the password hint (Qt::ImPasswordInput)? This is kind of a bummer because the inputcontext will now need to poll for a password input box on each micro focus change (at least that's how I envision this working). Ideally if I could change the inputcontext's state with a view attribute or something similar to Qt::WA_InputMethodEnabled or RequestSoftwareInputPasswordPanel?

[Joseph Ligman]

Created attachment 40826 [details]
[Qt] patch to set Qt::WA_InputMethodEnabled attribute for password fields in EditorClientQt::setInputMethodState

[Simon Hausmann]

Comment on attachment 40826 [details]
[Qt] patch to set Qt::WA_InputMethodEnabled attribute for password fields in EditorClientQt::setInputMethodState 

Joe, the patch is almost correct, apart from two small points :-)


> +            //Setting the Qt::WA_InputMethodEnabled attribute to true for password fields
> +            //The Qt platform is responsible for determining which widget will receive 
> +            //the input method events when the inputMethodHint (Qt::ImhHiddenText) is 
> +            //set.

(Please put a space between the // and the comment text. It's not in the documented coding style, but it's
a defacto standard)

> +            Frame* frame = m_page->d->page->focusController()->focusedOrMainFrame();
> +            if (frame && frame->document() && frame->document()->focusedNode()) {
> +                if (frame->document()->focusedNode()->hasTagName(HTMLNames::inputTag)) {
> +                    HTMLInputElement* inputElement = static_cast<HTMLInputElement*>(frame->document()->focusedNode());
> +                    active = inputElement->isPasswordField();
> +                    if (active)
> +                        view->setInputMethodHints(Qt::ImhHiddenText);

This code will set the ImhHiddenText input method hint, but it will override any other input method hints set on the view.
This function should _preserve_ the other hints set on the view and toggle only the ImhHiddenText, i.e. also _unset_ it
when the input element is _not_ a password field. If you extend your autotest to test focusing a regular input field
after the password field, then the ImhHiddenText hint should not be set anymore.

There's also one more issue that would be great to fix while we're at it :-)

Currently this function operates on the page's view as QWidget. We are trying to migrate away from
using the view(), as it does not work correctly with QGraphicsWebView. We should operate on the QGraphicsWebView
instead of the QGraphicsView that the item is associated with. In other words:

    1) We need two pure virtual methods in QWebPageClient:
        void setInputMethodEnabled(bool enabled);
        void setInputMethodHint(Qt::InputMethodHint hint, bool enable);

and then in QWebViewPrivate we need to map this to the qwidget:

    2)
        void QWebPagePrivate::setInputMethodEnabled(bool enabled)
        {
            view->setAttribute(Qt::WA_InputMethodEnabled, enabled);
        }
        void QWebPagePrivate::setInputMethodHint(Qt::InputMethodHint hint, bool enable)
        {
            if (enable)
                view->setInputMethodHints(view->inputMethodHints() | hint);
            else
                view->setInputMethodHints(view->inputMethodHints() & ~hint);
        }

and the same for QGraphicsWebViewPrivate:

    3)

        void QGraphicsWebViewPrivate::setInputMethodEnabled(bool enabled)
        {
            q->setFlag(QGraphicsItem::ItemAcceptsInputMethod, enabled);
        }
        void QWebPagePrivate::setInputMethodHint(Qt::InputMethodHint hint, bool enable)
        {
            if (enable)
                q->setInputMethodHints(q->inputMethodHints() | hint);
            else
                q->setInputMethodHints(q->inputMethodHints() & ~hint);
        }


That should make it work properly with QWebView and QWebGraphicsView.

[Joseph Ligman]

Created attachment 40910 [details]
[Qt] patch to set Qt::WA_InputMethodEnabled attribute for password fields in EditorClientQt::setInputMethodState

[Simon Hausmann]

Comment on attachment 40910 [details]
[Qt] patch to set Qt::WA_InputMethodEnabled attribute for password fields in EditorClientQt::setInputMethodState 


> +    QWebPageClient* webPageClient = m_page->d->client;
> +    if (webPageClient) {
> +#if QT_VERSION >= 0x040600
> +        Qt::InputMethodHints hints = Qt::ImhNone;
> +        if (!active) {
> +            // Setting the Qt::WA_InputMethodEnabled attribute true and Qt::ImhHiddenText flag
> +            // for password fields. The Qt platform is responsible for determining which widget 
> +            // will receive input method events for password fields.
> +            Frame* frame = m_page->d->page->focusController()->focusedOrMainFrame();
> +            if (frame && frame->document() && frame->document()->focusedNode()) {
> +                if (frame->document()->focusedNode()->hasTagName(HTMLNames::inputTag)) {
> +                    HTMLInputElement* inputElement = static_cast<HTMLInputElement*>(frame->document()->focusedNode());
> +                    active = inputElement->isPasswordField();
> +                    if (active)
> +                        hints |= Qt::ImhHiddenText;
> +              }
> +            }
> +        }
> +        webPageClient->setInputMethodHint(Qt::ImhHiddenText, hints & Qt::ImhHiddenText);

You could probably simplify this by replacing the local "hints" variable with just a boolean :-)

Feel free to make that small change before landing, or change it in a follow-up patch.

The rest of the patch looks good. Thanks Joe!

[Joseph Ligman]

(In reply to comment #9)
> (From update of attachment 40910 [details])
...
> You could probably simplify this by replacing the local "hints" variable with
> just a boolean :-)
> 
> Feel free to make that small change before landing, or change it in a follow-up
> patch.
> 
> The rest of the patch looks good. Thanks Joe!

Thanks for the review. Sorry, I know it looks wrong. I should have commented. My reasoning is that I want to add the rest of the hints (uppercase, numbers, mask etc.. for wap css) in setInputMethodState and thought it would be good to use variable instead of booleans.

[Laszlo Gombos]

Comment on attachment 40910 [details]
[Qt] patch to set Qt::WA_InputMethodEnabled attribute for password fields in EditorClientQt::setInputMethodState 

Committed as http://trac.webkit.org/changeset/49397 with the additional changes suggested by Simon.