Ubuntu UI Toolkit

Merge lp:~zsombi/ubuntu-ui-toolkit/focusing-improvements into lp:ubuntu-ui-toolkit/staging

  1. focusing-improvements
  2. Merge into staging
Proposed by Zsombor Egri
Status: Merged
Approved by: Zsombor Egri
Approved revision: 1229
Merged at revision: 1228
Proposed branch: lp:~zsombi/ubuntu-ui-toolkit/focusing-improvements
Merge into: lp:ubuntu-ui-toolkit/staging
Diff against target: 254 lines (+36/-111)
6 files modified
modules/Ubuntu/Components/AbstractButton.qml (+0/-1)
modules/Ubuntu/Components/Pickers/Picker.qml (+0/-1)
modules/Ubuntu/Components/plugin/ucstyleditembase.cpp (+22/-54)
modules/Ubuntu/Components/plugin/ucstyleditembase.h (+1/-0)
tests/unit_x11/tst_components/tst_focus.qml (+12/-0)
tests/unit_x11/tst_components/tst_styleditem.qml (+1/-55)
To merge this branch: bzr merge lp:~zsombi/ubuntu-ui-toolkit/focusing-improvements
Reviewer Review Type Date Requested Status
PS Jenkins bot continuous-integration Approve
Cris Dywan Approve
Review via email: mp+232972@code.launchpad.net

Commit message

Focus should not be taken by a component if disabled. Complete child filtering properly.

To post a comment you must log in.
Revision history for this message
Zsombor Egri (zsombi) wrote :

The child filtering wasn't completed in teh original MR :/ the setFiltersChildMouseEvents() call was there without having the childMouseEventsFilter() being implemented. This caused us to have all sorts of workarounds for which QtQuick provides a solution. Mea culpa.

Revision history for this message
Cris Dywan (kalikiana) wrote :

This makes quite a bit of sense. I wonder how neither of us noticed before… looks sensible.

review: Approve
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:1227
http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/949/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-utopic-touch/4306/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-utopic-amd64-ci/781/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-utopic-armhf-ci/781/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-utopic-i386-ci/781/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/5558/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/949/rebuild

review: Needs Fixing (continuous-integration)
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Autolanding.
More details in the following jenkins job:
http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-autolanding/472/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-utopic-touch/4307/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-utopic-amd64-autolanding/416/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-utopic-armhf-autolanding/416/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-utopic-i386-autolanding/416/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/5559/console

review: Needs Fixing (continuous-integration)
lp:~zsombi/ubuntu-ui-toolkit/focusing-improvements updated
1228. By Zsombor Egri

remove unused d-pointer causing build warning

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

FAILED: Continuous integration, rev:1228
http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/954/
Executed test runs:
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-utopic-touch/4316/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-utopic-amd64-ci/786/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-utopic-armhf-ci/786/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-utopic-i386-ci/786/console
    FAILURE: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/5568/console

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/954/rebuild

review: Needs Fixing (continuous-integration)
lp:~zsombi/ubuntu-ui-toolkit/focusing-improvements updated
1229. By Zsombor Egri

documentation fix

Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:1229
http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/960/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-utopic-touch/4328
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-utopic-amd64-ci/792
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-utopic-armhf-ci/792
        deb: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-utopic-armhf-ci/792/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-utopic-i386-ci/792
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-mako/4128
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/5580
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-utopic-armhf/5580/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/12483

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/ubuntu-sdk-team-ubuntu-ui-toolkit-staging-ci/960/rebuild

review: Approve (continuous-integration)

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'modules/Ubuntu/Components/AbstractButton.qml'
--- modules/Ubuntu/Components/AbstractButton.qml 2014-08-21 05:24:14 +0000
+++ modules/Ubuntu/Components/AbstractButton.qml 2014-09-02 12:49:18 +0000
@@ -103,7 +103,6 @@
103 hoverEnabled: true103 hoverEnabled: true
104104
105 onClicked: {105 onClicked: {
106 button.requestFocus();
107 if (button.__acceptEvents) {106 if (button.__acceptEvents) {
108 pressEffect.start()107 pressEffect.start()
109 button.clicked()108 button.clicked()
110109
=== modified file 'modules/Ubuntu/Components/Pickers/Picker.qml'
--- modules/Ubuntu/Components/Pickers/Picker.qml 2014-08-21 05:24:14 +0000
+++ modules/Ubuntu/Components/Pickers/Picker.qml 2014-09-02 12:49:18 +0000
@@ -294,7 +294,6 @@
294 width: parent ? parent.width : 0294 width: parent ? parent.width : 0
295 clip: true295 clip: true
296 focus: true296 focus: true
297 Mouse.onClicked: picker.requestFocus()
298297
299 model: picker.model298 model: picker.model
300 delegate: picker.delegate299 delegate: picker.delegate
301300
=== modified file 'modules/Ubuntu/Components/plugin/ucstyleditembase.cpp'
--- modules/Ubuntu/Components/plugin/ucstyleditembase.cpp 2014-08-22 16:09:46 +0000
+++ modules/Ubuntu/Components/plugin/ucstyleditembase.cpp 2014-09-02 12:49:18 +0000
@@ -86,7 +86,7 @@
86}86}
8787
88/*!88/*!
89 * \qmlmethod void StyledItemBase::requestFocus()89 * \qmlmethod void StyledItemBase::requestFocus(Qt::FocusReason reason)
90 * \since Ubuntu.Components 1.190 * \since Ubuntu.Components 1.1
91 * The function is similar to \c forceActiveFocus except that it sets focus only91 * The function is similar to \c forceActiveFocus except that it sets focus only
92 * if all the ancestors have activeFocusOnPressed set. Returns true if the request92 * if all the ancestors have activeFocusOnPressed set. Returns true if the request
@@ -96,7 +96,7 @@
96{96{
97 Q_D(UCStyledItemBase);97 Q_D(UCStyledItemBase);
98 bool focusable = d->isParentFocusable();98 bool focusable = d->isParentFocusable();
99 if (focusable) {99 if (focusable && isEnabled()) {
100 QQuickItem::forceActiveFocus(reason);100 QQuickItem::forceActiveFocus(reason);
101 }101 }
102 return focusable;102 return focusable;
@@ -151,57 +151,6 @@
151 * }151 * }
152 * \endqml152 * \endqml
153 *153 *
154 * StyledItem cannot gain focus automaically if there is a MouseArea or other
155 * component grabbing mouse events placed over it. In that case the focus must
156 * be grabbed by the overlapping area explicitly either by calling the \l requestFocus
157 * function or by forwarding mouse events to the covered StyledItem area.
158 * \qml
159 * import QtQuick 2.2
160 * import Ubuntu.Components 1.1
161 *
162 * Column {
163 * width: units.gu(50)
164 * height: units.gu(100)
165 *
166 * StyledItem {
167 * id: scope1
168 * width: parent.width
169 * height: units.gu(30)
170 * activeFocusOnPress: false
171 * Rectangle {
172 * focus: true
173 * anchors {
174 * fill: parent
175 * margins: units.gu(1)
176 * }
177 * color: !activeFocus ? "red" : "green"
178 * MouseArea {
179 * anchors.fill: parent
180 * onClicked: scope1.requestFocus()
181 * }
182 * }
183 * }
184 * StyledItem {
185 * id: scope2
186 * width: parent.width
187 * height: units.gu(30)
188 * activeFocusOnPress: false
189 * Rectangle {
190 * focus: true
191 * anchors {
192 * fill: parent
193 * margins: units.gu(1)
194 * }
195 * color: !activeFocus ? "red" : "green"
196 * MouseArea {
197 * anchors.fill: parent
198 * Mouse.forwardTo: [scope2]
199 * }
200 * }
201 * }
202 * }
203 * \endqml
204 *
205 * The default value is \c false.154 * The default value is \c false.
206 */155 */
207bool UCStyledItemBase::activefocusOnPress() const156bool UCStyledItemBase::activefocusOnPress() const
@@ -223,8 +172,27 @@
223void UCStyledItemBase::mousePressEvent(QMouseEvent *event)172void UCStyledItemBase::mousePressEvent(QMouseEvent *event)
224{173{
225 QQuickItem::mousePressEvent(event);174 QQuickItem::mousePressEvent(event);
226 Q_D(UCStyledItemBase);
227 requestFocus(Qt::MouseFocusReason);175 requestFocus(Qt::MouseFocusReason);
228}176}
229177
178// filter children events as well, so we can catch mouse presses done over child
179// MouseAreas or other mouse grabbers
180bool UCStyledItemBase::childMouseEventFilter(QQuickItem *child, QEvent *event)
181{
182 // only filter pressed events
183 if (event->type() == QEvent::MouseButtonPress) {
184 QMouseEvent *mouse = static_cast<QMouseEvent*>(event);
185 // the event may occur outside of the parent's boundaries if not clipped
186 // therefore must check containment
187 QPointF point = mapFromItem(child, mouse->localPos());
188 if (contains(point)) {
189 QMouseEvent press(event->type(), point, mouse->windowPos(), mouse->screenPos(),
190 mouse->button(), mouse->buttons(), mouse->modifiers());
191 mousePressEvent(&press);
192 }
193 }
194 // let the event be passed to children
195 return false;
196}
197
230#include "moc_ucstyleditembase.cpp"198#include "moc_ucstyleditembase.cpp"
231199
=== modified file 'modules/Ubuntu/Components/plugin/ucstyleditembase.h'
--- modules/Ubuntu/Components/plugin/ucstyleditembase.h 2014-08-20 12:56:51 +0000
+++ modules/Ubuntu/Components/plugin/ucstyleditembase.h 2014-09-02 12:49:18 +0000
@@ -44,6 +44,7 @@
44 UCStyledItemBase(UCStyledItemBasePrivate &, QQuickItem *parent);44 UCStyledItemBase(UCStyledItemBasePrivate &, QQuickItem *parent);
4545
46 void mousePressEvent(QMouseEvent *event);46 void mousePressEvent(QMouseEvent *event);
47 bool childMouseEventFilter(QQuickItem *child, QEvent *event);
4748
48private:49private:
49 Q_DECLARE_PRIVATE(UCStyledItemBase)50 Q_DECLARE_PRIVATE(UCStyledItemBase)
5051
=== modified file 'tests/unit_x11/tst_components/tst_focus.qml'
--- tests/unit_x11/tst_components/tst_focus.qml 2014-08-18 16:02:33 +0000
+++ tests/unit_x11/tst_components/tst_focus.qml 2014-09-02 12:49:18 +0000
@@ -82,6 +82,12 @@
82 Slider {82 Slider {
83 id: slider83 id: slider
84 }84 }
85 StyledItem {
86 id: disabledButton
87 enabled: false
88 width: units.gu(20)
89 height: units.gu(6)
90 }
85 ComboButton {91 ComboButton {
86 id: comboButton92 id: comboButton
87 Rectangle {93 Rectangle {
@@ -211,6 +217,7 @@
211 compare(dropdownButton.focus, true, "Dropdown button hasn't got focused!");217 compare(dropdownButton.focus, true, "Dropdown button hasn't got focused!");
212 compare(comboButton.focus, true, "ComboButton hasn't been focused!");218 compare(comboButton.focus, true, "ComboButton hasn't been focused!");
213 comboButton.expanded = false;219 comboButton.expanded = false;
220 waitForRendering(comboButton);
214 }221 }
215222
216 function test_popover_refocus_data() {223 function test_popover_refocus_data() {
@@ -236,5 +243,10 @@
236 popupCloseSpy.wait();243 popupCloseSpy.wait();
237 verify(popoverTest.focus, "Button focus not restored.");244 verify(popoverTest.focus, "Button focus not restored.");
238 }245 }
246
247 function test_disabled_component_does_not_focus() {
248 mousePress(disabledButton, centerOf(disabledButton).x, centerOf(disabledButton).y);
249 compare(disabledButton.focus, false, "Disabled component shoudl not focus");
250 }
239 }251 }
240}252}
241253
=== modified file 'tests/unit_x11/tst_components/tst_styleditem.qml'
--- tests/unit_x11/tst_components/tst_styleditem.qml 2014-08-20 12:40:19 +0000
+++ tests/unit_x11/tst_components/tst_styleditem.qml 2014-09-02 12:49:18 +0000
@@ -90,58 +90,6 @@
90 }90 }
91 }91 }
92 }92 }
93 StyledItem {
94 id: activeScope2
95 objectName: "activeScope"
96 width: units.gu(50)
97 height: units.gu(20)
98 activeFocusOnPress: true
99
100 Rectangle {
101 width: height
102 height: units.gu(10)
103 color: "green"
104 anchors.centerIn: parent
105
106 StyledItem {
107 objectName: "in_active_scope"
108 activeFocusOnPress: true
109 anchors.fill: parent
110 MouseArea {
111 focus: true
112 objectName: "mouseArea"
113 anchors.fill: parent
114 onClicked: parent.requestFocus()
115 }
116 }
117 }
118 }
119 StyledItem {
120 id: activeScope3
121 objectName: "activeScope"
122 width: units.gu(50)
123 height: units.gu(20)
124 activeFocusOnPress: true
125
126 Rectangle {
127 width: height
128 height: units.gu(10)
129 color: "green"
130 anchors.centerIn: parent
131
132 StyledItem {
133 objectName: "in_active_scope"
134 activeFocusOnPress: true
135 anchors.fill: parent
136 MouseArea {
137 focus: true
138 objectName: "mouseArea"
139 anchors.fill: parent
140 Mouse.forwardTo: [parent]
141 }
142 }
143 }
144 }
145 }93 }
14694
147 UbuntuTestCase {95 UbuntuTestCase {
@@ -152,9 +100,7 @@
152 return [100 return [
153 {tag: "main scope is passive", mainScope: passiveScope, innerScope: "in_passive_scope", focusing: false},101 {tag: "main scope is passive", mainScope: passiveScope, innerScope: "in_passive_scope", focusing: false},
154 {tag: "main scope is active", mainScope: activeScope, innerScope: "in_active_scope", focusing: true},102 {tag: "main scope is active", mainScope: activeScope, innerScope: "in_active_scope", focusing: true},
155 {tag: "MouseArea as child, suppressing", mainScope: activeScope1, innerScope: "mouseArea", focusing: false},103 {tag: "MouseArea as child, gaining", mainScope: activeScope1, innerScope: "mouseArea", focusing: true},
156 {tag: "MouseArea as child, gaining", mainScope: activeScope2, innerScope: "mouseArea", focusing: true},
157 {tag: "MouseArea as child, forwarding", mainScope: activeScope3, innerScope: "mouseArea", focusing: true},
158 ];104 ];
159 }105 }
160 function test_scope_focusing(data) {106 function test_scope_focusing(data) {

Subscribers

People subscribed via source and target branches