Merge ~santoshbit2007/oxide:focus_issue_refix into oxide:master

Proposed by Santosh on 2016-12-15
Status: Merged
Merge reported by: Santosh
Merged at revision: not available
Proposed branch: ~santoshbit2007/oxide:focus_issue_refix
Merge into: oxide:master
Diff against target: 167 lines (+103/-1)
6 files modified
qt/core/browser/oxide_qt_contents_view.cc (+4/-1)
qt/core/browser/oxide_qt_contents_view.h (+1/-0)
qt/core/glue/oxide_qt_contents_view_proxy.h (+1/-0)
qt/quick/oxide_qquick_contents_view.cc (+2/-0)
qt/tests/qmltests/core/tst_focus.html (+13/-0)
qt/tests/qmltests/core/tst_focus.qml (+82/-0)
Reviewer Review Type Date Requested Status
Olivier Tilloy Approve on 2017-01-10
Chris Coulson 2016-12-15 Approve on 2017-01-05
Review via email: mp+313393@code.launchpad.net

Description of the change

With this patch now focus changed is applied to webview when ItemChange event is sent to QQuickWebView. Before it was done on focusOut/In event, where query of hasActiveFocus and window-activeFocusItem return inconsistent value causing focus bugs.

To post a comment you must log in.
Olivier Tilloy (osomon) wrote :

The fix looks good to me.
I’ve added a few comments for suggestions to improve (mostly) the tests.
The reference to launchpad bugs in the commit message should be updated to mention bug #1599771.

review: Needs Fixing
Chris Coulson (chrisccoulson) wrote :

I've not had a chance to review this fully yet, but I've left 1 comment inline.

review: Needs Fixing
Chris Coulson (chrisccoulson) wrote :

Looks fine to me - I'll let Olivier comment on the test changes though.

review: Approve
Olivier Tilloy (osomon) wrote :

That looks mostly good to me, I’ve added a few suggestions for the tests.

3b449a0... by Santosh on 2017-01-10

Applied review comments on test case

Olivier Tilloy (osomon) wrote :

Thanks. More suggestions inline.

4c9bc71... by Santosh on 2017-01-10

2: Applied review comments

Olivier Tilloy (osomon) wrote :

See comment inline.

review: Needs Fixing
65670f5... by Santosh on 2017-01-10

Changed Signal Name

Olivier Tilloy (osomon) wrote :

That looks good to me now. I haven’t had a chance to run the tests locally because I’m in the middle of rebuilding my local copy of oxide, but I’ll trust that you did and that they pass.

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1diff --git a/qt/core/browser/oxide_qt_contents_view.cc b/qt/core/browser/oxide_qt_contents_view.cc
2index 2698877..af47e66 100644
3--- a/qt/core/browser/oxide_qt_contents_view.cc
4+++ b/qt/core/browser/oxide_qt_contents_view.cc
5@@ -296,6 +296,10 @@ void ContentsView::visibilityChanged() {
6 view()->VisibilityChanged();
7 }
8
9+void ContentsView::activeFocusChanged() {
10+ view()->FocusChanged();
11+}
12+
13 QVariant ContentsView::inputMethodQuery(Qt::InputMethodQuery query) const {
14 return input_method_context_->Query(query);
15 }
16@@ -319,7 +323,6 @@ void ContentsView::handleInputMethodEvent(QInputMethodEvent* event) {
17
18 void ContentsView::handleFocusEvent(QFocusEvent* event) {
19 input_method_context_->FocusChanged(event);
20- view()->FocusChanged();
21
22 event->accept();
23 }
24diff --git a/qt/core/browser/oxide_qt_contents_view.h b/qt/core/browser/oxide_qt_contents_view.h
25index 10f9772..ab5a74a 100644
26--- a/qt/core/browser/oxide_qt_contents_view.h
27+++ b/qt/core/browser/oxide_qt_contents_view.h
28@@ -79,6 +79,7 @@ class ContentsView : public QObject,
29 void windowChanged() override;
30 void wasResized() override;
31 void visibilityChanged() override;
32+ void activeFocusChanged() override;
33 QVariant inputMethodQuery(Qt::InputMethodQuery query) const override;
34 void handleKeyEvent(QKeyEvent* event) override;
35 void handleInputMethodEvent(QInputMethodEvent* event) override;
36diff --git a/qt/core/glue/oxide_qt_contents_view_proxy.h b/qt/core/glue/oxide_qt_contents_view_proxy.h
37index fd7137c..252381f 100644
38--- a/qt/core/glue/oxide_qt_contents_view_proxy.h
39+++ b/qt/core/glue/oxide_qt_contents_view_proxy.h
40@@ -80,6 +80,7 @@ class ContentsViewProxy {
41 virtual void windowChanged() = 0;
42 virtual void wasResized() = 0;
43 virtual void visibilityChanged() = 0;
44+ virtual void activeFocusChanged() = 0;
45
46 virtual QVariant inputMethodQuery(Qt::InputMethodQuery query) const = 0;
47
48diff --git a/qt/quick/oxide_qquick_contents_view.cc b/qt/quick/oxide_qquick_contents_view.cc
49index e2d92ad..4884e7d 100644
50--- a/qt/quick/oxide_qquick_contents_view.cc
51+++ b/qt/quick/oxide_qquick_contents_view.cc
52@@ -221,6 +221,8 @@ void ContentsView::handleItemChange(QQuickItem::ItemChange change) {
53
54 if (change == QQuickItem::ItemVisibleHasChanged) {
55 proxy()->visibilityChanged();
56+ } else if (change == QQuickItem::ItemActiveFocusHasChanged) {
57+ proxy()->activeFocusChanged();
58 }
59 }
60
61diff --git a/qt/tests/qmltests/core/tst_focus.html b/qt/tests/qmltests/core/tst_focus.html
62new file mode 100644
63index 0000000..245c3e5
64--- /dev/null
65+++ b/qt/tests/qmltests/core/tst_focus.html
66@@ -0,0 +1,13 @@
67+<html>
68+<body>
69+ <input type="text" id="input" onfocusout="focusChanged(false)"
70+ onfocusin="focusChanged(true)" autofocus>
71+ <script>
72+ function focusChanged(value) {
73+ var e = new CustomEvent("oxidefocusstate", { bubbles: true, detail: { status: value } });
74+ document.documentElement.dispatchEvent(e);
75+ }
76+ </script>
77+</body>
78+</html>
79+
80diff --git a/qt/tests/qmltests/core/tst_focus.qml b/qt/tests/qmltests/core/tst_focus.qml
81new file mode 100644
82index 0000000..472ed6e
83--- /dev/null
84+++ b/qt/tests/qmltests/core/tst_focus.qml
85@@ -0,0 +1,82 @@
86+import QtQuick 2.0
87+import QtTest 1.0
88+import com.canonical.Oxide 1.0
89+import Oxide.testsupport 1.0
90+
91+TestWebView {
92+ id: webView
93+
94+ property bool htmlInputElementFocus: true
95+
96+ focus: true
97+
98+ messageHandlers: [
99+ ScriptMessageHandler {
100+ msgId: "FOCUS-STATE"
101+ contexts: [ "oxide://testutils/" ]
102+ callback: function(msg) {
103+ webView.htmlInputElementFocus = msg.payload;
104+ }
105+ }
106+ ]
107+
108+ SignalSpy {
109+ id: spy
110+ target: webView
111+ signalName: "activeFocusChanged"
112+ }
113+
114+ TestCase {
115+ id: testcase;
116+ name: "webview_focus"
117+ when: windowShown
118+
119+ function initTestCase() {
120+ verify(webView.activeFocus);
121+ }
122+
123+ function init() {
124+ spy.clear();
125+
126+ webView.url = "https://testsuite/tst_focus.html";
127+ verify(webView.waitForLoadSucceeded(),
128+ "Timed out waiting for successful load");
129+
130+ webView.getTestApi().evaluateCode(
131+ "document.addEventListener(\"oxidefocusstate\", function(event) {
132+ oxide.sendMessage(\"FOCUS-STATE\", event.detail.status);
133+ });", true);
134+
135+ verify(webView.activeFocus);
136+ verify(webView.htmlInputElementFocus);
137+ }
138+
139+ function test_toggle_focus_onclicked() {
140+ var rect = webView.getTestApi().getBoundingClientRectForSelector("#input");
141+
142+ // clicking outside input field should unfocus the input element.
143+ var x1 = webView.width / 2;
144+ var y1 = (rect.y + rect.height + webView.height) / 2;
145+ mouseClick(webView, x1, y1, Qt.LeftButton);
146+ verify(webView.activeFocus);
147+ compare(spy.count, 0);
148+ tryCompare(webView, "htmlInputElementFocus", false);
149+
150+ // clicking inside input field restore the focus on input element.
151+ mouseClick(webView, rect.x + rect.width / 2, rect.y + rect.height / 2, Qt.LeftButton);
152+ verify(webView.activeFocus);
153+ compare(spy.count, 0);
154+ tryCompare(webView, "htmlInputElementFocus", true);
155+ }
156+
157+ function test_toggle_focus() {
158+ webView.focus = false;
159+ tryCompare(webView, "activeFocus", false);
160+ tryCompare(webView, "htmlInputElementFocus", false);
161+
162+ webView.focus = true;
163+ tryCompare(webView, "activeFocus", true);
164+ tryCompare(webView, "htmlInputElementFocus", true);
165+ }
166+ }
167+}

Subscribers

People subscribed via source and target branches

to all changes: