Bug 92386 – [Qt] Input method hints are not being set.

• Component: HTML Editing
• Status: RESOLVED FIXED

[Marcelo Lira]

When WebPage::editorState() is called it tries to get the information from an HTML input element, but it tries obtain this element from Frame::selection()->rootEditableElement(), which does not return the proper input object. Frame::document()->focusedNode() should be used instead, as it is seen on EditorClientQt::setInputMethodState(bool) method.

The method QtWebPageEventHandler::updateTextInputState() must also advertise, by using the Qt::Hints flag, that information on input hints is available.

[Marcelo Lira]

Created attachment 154664 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 154664 [details]
Patch

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

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:484
> -    Element* selectionRoot = frame->selection()->rootEditableElement();
> -    Element* scope = selectionRoot ? selectionRoot : frame->document()->documentElement();
> -
> -    if (!scope)
> -        return result;
> +    HTMLInputElement* input = 0;
> +    if (frame->document() && frame->document()->focusedNode()) {
> +        if (frame->document()->focusedNode()->hasTagName(HTMLNames::inputTag))
> +            input = static_cast<HTMLInputElement*>(frame->document()->focusedNode());
> +    }

You are not explaining this change in the changelog, nor why it is correct. Isn't this going to break terrible with contents-editable and document editing mode? This definitely needs tests to prove otherwise

[Marcelo Lira]

Created attachment 154736 [details]
Patch

[Caio Marcelo de Oliveira Filho]

LGTM.

[Marcelo Lira]

Kenneth, you're right. Ignore the first patch, the second changes much less things, and does not messes with the cases you pointed out.

[Simon Hausmann]

Comment on attachment 154736 [details]
Patch

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

> Source/WebKit2/UIProcess/qt/QtWebPageEventHandler.cpp:430
> +    qApp->inputMethod()->update(Qt::ImQueryInput | Qt::ImEnabled | Qt::ImHints);

I agree about this part, good catch.

> Source/WebKit2/WebProcess/WebPage/WebPage.cpp:487
> +    if (selectionRoot)
> +        selectionRoot = selectionRoot->shadowHost();

I think this needs a test, it's quite a subtle change.

[Marcelo Lira]

Created attachment 154988 [details]
Patch

[Caio Marcelo de Oliveira Filho]

Comment on attachment 154988 [details]
Patch

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

> Source/WebKit2/UIProcess/API/qt/tests/qquickwebview/tst_qquickwebview.cpp:420
> +    runJavaScript("document.getElementById('emailInputField').focus();");

Suggestion: could you also watch for signal that WebView emits informing that the focus was changed?

[Marcelo Lira]

(In reply to comment #8)
> (From update of attachment 154988 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=154988&action=review
> 
> > Source/WebKit2/UIProcess/API/qt/tests/qquickwebview/tst_qquickwebview.cpp:420
> > +    runJavaScript("document.getElementById('emailInputField').focus();");
> 
> Suggestion: could you also watch for signal that WebView emits informing that the focus was changed?

I will look into that, but I would make it a different patch.

[Marcelo Lira]

Additional information on the Shadow DOM:
http://glazkov.com/2011/01/14/what-the-heck-is-shadow-dom/

[Marcelo Lira]

Created attachment 155531 [details]
Patch

[Kenneth Rohde Christiansen]

Dimitri, Ryosuke, is this correct?

[Alexey Proskuryakov]

This patch makes good sense to me conceptually.

What doesn't make a lot of sense is that there is apparently no way to tell whether a function like rootEditableElement() will look inside shadow DOM or not. It feels like some core functions have this isInShadowTree() check internally, and some do not. But it's only tangentially related to this issue.

[Simon Hausmann]

(In reply to comment #13)
> This patch makes good sense to me conceptually.
> 
> What doesn't make a lot of sense is that there is apparently no way to tell whether a function like rootEditableElement() will look inside shadow DOM or not. It feels like some core functions have this isInShadowTree() check internally, and some do not. But it's only tangentially related to this issue.

Perhaps it would make sense to have at least an overload, rootEditableElementRespectingShadowTree(), that other ports can also use?

I see the same "mistake" in the editing code of the other ports.

[Marcelo Lira]

Created attachment 157759 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 157759 [details]
Patch

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

Please improve the comment

> Source/WebKit2/UIProcess/API/qt/tests/qquickwebview/tst_qquickwebview.cpp:423
> +    // The focus of an INPUT is really given to an underlying element in its shadow tree,
> +    // which will not give any input hints. This tests the WebPage::editorState() method
> +    // ability to get the root element (INPUT proper) from the focused shadow element.

I find this hard to understand.

// Setting focus on an input element results in an element in its shadow tree becoming the focus node.
// Input hints should not be set from this shadow tree node but from the input element itself.

[Marcelo Lira]

Created attachment 158020 [details]
Patch

[Kenneth Rohde Christiansen]

Comment on attachment 158020 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +        (WebCore::FrameSelection::rootEditableElementRespectingShadowTree): Similar to WebCore::FrameSelection::rootEditableElement, but returns the first ancestor of the editable element outside the shadow tree.

I would wrap that line

[Marcelo Lira]

Created attachment 158037 [details]
Patch

[Marcelo Lira]

Created attachment 158039 [details]
Patch for landing

[Marcelo Lira]

Created attachment 158795 [details]
Patch

[WebKit Review Bot]

Comment on attachment 158795 [details]
Patch

Clearing flags on attachment: 158795

Committed r125777: <http://trac.webkit.org/changeset/125777>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.