Bug 30023

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?

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

Created attachment 40551 [details] [Qt] patch to make the software input panel work for password fields Added pointer checks for frame, document, and focusnode.

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.

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?

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

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.

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

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!

(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.

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.