[webkit-reviews] review denied: [Bug 30023] [Qt] Software input panel is not launched for password fields : [Attachment 40826] [Qt] patch to set Qt::WA_InputMethodEnabled attribute for password fields in EditorClientQt::setInputMethodState

[bugzilla-daemon at webkit.org]

Simon Hausmann <hausmann at webkit.org> has denied Joseph Ligman
<joseph.ligman at nokia.com>'s request for review:
Bug 30023: [Qt] Software input panel is not launched for password fields
https://bugs.webkit.org/show_bug.cgi?id=30023

Attachment 40826: [Qt] patch to set Qt::WA_InputMethodEnabled attribute for
password fields in EditorClientQt::setInputMethodState 
https://bugs.webkit.org/attachment.cgi?id=40826&action=review

------- Additional Comments from Simon Hausmann <hausmann at webkit.org>
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.