Bug 92386 - [Qt] Input method hints are not being set.
Summary: [Qt] Input method hints are not being set.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: HTML Editing (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Marcelo Lira
URL:
Keywords: Qt
Depends on: 92744
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-26 08:59 PDT by Marcelo Lira
Modified: 2012-08-16 07:28 PDT (History)
12 users (show)

See Also:


Attachments
Patch (3.21 KB, patch)
2012-07-26 09:06 PDT, Marcelo Lira
no flags Details | Formatted Diff | Diff
Patch (2.40 KB, patch)
2012-07-26 13:33 PDT, Marcelo Lira
no flags Details | Formatted Diff | Diff
Patch (4.50 KB, patch)
2012-07-27 10:53 PDT, Marcelo Lira
no flags Details | Formatted Diff | Diff
Patch (4.82 KB, patch)
2012-07-31 07:38 PDT, Marcelo Lira
no flags Details | Formatted Diff | Diff
Patch (8.09 KB, patch)
2012-08-10 11:01 PDT, Marcelo Lira
no flags Details | Formatted Diff | Diff
Patch (8.00 KB, patch)
2012-08-13 09:38 PDT, Marcelo Lira
no flags Details | Formatted Diff | Diff
Patch (8.05 KB, patch)
2012-08-13 10:39 PDT, Marcelo Lira
no flags Details | Formatted Diff | Diff
Patch for landing (8.08 KB, patch)
2012-08-13 10:49 PDT, Marcelo Lira
no flags Details | Formatted Diff | Diff
Patch (8.05 KB, patch)
2012-08-16 06:16 PDT, Marcelo Lira
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Marcelo Lira 2012-07-26 08:59:29 PDT
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.
Comment 1 Marcelo Lira 2012-07-26 09:06:43 PDT
Created attachment 154664 [details]
Patch
Comment 2 Kenneth Rohde Christiansen 2012-07-26 09:27:15 PDT
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
Comment 3 Marcelo Lira 2012-07-26 13:33:08 PDT
Created attachment 154736 [details]
Patch
Comment 4 Caio Marcelo de Oliveira Filho 2012-07-26 13:48:57 PDT
LGTM.
Comment 5 Marcelo Lira 2012-07-26 13:53:14 PDT
Kenneth, you're right. Ignore the first patch, the second changes much less things, and does not messes with the cases you pointed out.
Comment 6 Simon Hausmann 2012-07-27 07:18:12 PDT
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.
Comment 7 Marcelo Lira 2012-07-27 10:53:38 PDT
Created attachment 154988 [details]
Patch
Comment 8 Caio Marcelo de Oliveira Filho 2012-07-27 10:56:53 PDT
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?
Comment 9 Marcelo Lira 2012-07-30 11:04:47 PDT
(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.
Comment 10 Marcelo Lira 2012-07-31 07:27:54 PDT
Additional information on the Shadow DOM:
http://glazkov.com/2011/01/14/what-the-heck-is-shadow-dom/
Comment 11 Marcelo Lira 2012-07-31 07:38:27 PDT
Created attachment 155531 [details]
Patch
Comment 12 Kenneth Rohde Christiansen 2012-08-02 09:48:10 PDT
Dimitri, Ryosuke, is this correct?
Comment 13 Alexey Proskuryakov 2012-08-02 10:20:56 PDT
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.
Comment 14 Simon Hausmann 2012-08-09 22:37:53 PDT
(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.
Comment 15 Marcelo Lira 2012-08-10 11:01:06 PDT
Created attachment 157759 [details]
Patch
Comment 16 Kenneth Rohde Christiansen 2012-08-10 14:12:40 PDT
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.
Comment 17 Marcelo Lira 2012-08-13 09:38:21 PDT
Created attachment 158020 [details]
Patch
Comment 18 Kenneth Rohde Christiansen 2012-08-13 10:24:39 PDT
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
Comment 19 Marcelo Lira 2012-08-13 10:39:43 PDT
Created attachment 158037 [details]
Patch
Comment 20 Marcelo Lira 2012-08-13 10:49:16 PDT
Created attachment 158039 [details]
Patch for landing
Comment 21 Marcelo Lira 2012-08-16 06:16:46 PDT
Created attachment 158795 [details]
Patch
Comment 22 WebKit Review Bot 2012-08-16 07:28:06 PDT
Comment on attachment 158795 [details]
Patch

Clearing flags on attachment: 158795

Committed r125777: <http://trac.webkit.org/changeset/125777>
Comment 23 WebKit Review Bot 2012-08-16 07:28:12 PDT
All reviewed patches have been landed.  Closing bug.