Summary: | [Qt] Input method hints are not being set. | ||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Marcelo Lira <marcelo.lira> | ||||||||||||||||||||
Component: | HTML Editing | Assignee: | Marcelo Lira <marcelo.lira> | ||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||
Severity: | Normal | CC: | ap, cmarcelo, dglazkov, hausmann, hbono, kenneth, menard, mifenton, rafael.lobo, rniwa, webkit.review.bot, zoltan | ||||||||||||||||||||
Priority: | P2 | Keywords: | Qt | ||||||||||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||
Bug Depends on: | 92744 | ||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||
Attachments: |
|
Description
Marcelo Lira
2012-07-26 08:59:29 PDT
Created attachment 154664 [details]
Patch
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 Created attachment 154736 [details]
Patch
LGTM. 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 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. Created attachment 154988 [details]
Patch
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? (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. Additional information on the Shadow DOM: http://glazkov.com/2011/01/14/what-the-heck-is-shadow-dom/ Created attachment 155531 [details]
Patch
Dimitri, Ryosuke, is this correct? 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. (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. Created attachment 157759 [details]
Patch
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. Created attachment 158020 [details]
Patch
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 Created attachment 158037 [details]
Patch
Created attachment 158039 [details]
Patch for landing
Created attachment 158795 [details]
Patch
Comment on attachment 158795 [details] Patch Clearing flags on attachment: 158795 Committed r125777: <http://trac.webkit.org/changeset/125777> All reviewed patches have been landed. Closing bug. |