webbrowser-app

Merge lp:~osomon/webbrowser-app/keyboard-shortcuts-focus-fixes into lp:webbrowser-app

  1. keyboard-shortcuts-focus-fixes
  2. Merge into trunk
Proposed by Olivier Tilloy
Status: Merged
Approved by: Olivier Tilloy
Approved revision: 1302
Merged at revision: 1305
Proposed branch: lp:~osomon/webbrowser-app/keyboard-shortcuts-focus-fixes
Merge into: lp:webbrowser-app
Diff against target: 333 lines (+93/-48)
2 files modified
src/app/webbrowser/Browser.qml (+52/-47)
tests/autopilot/webbrowser_app/tests/test_keyboard.py (+41/-1)
To merge this branch: bzr merge lp:~osomon/webbrowser-app/keyboard-shortcuts-focus-fixes
Reviewer Review Type Date Requested Status
Ugo Riboni (community) Approve
PS Jenkins bot continuous-integration Approve
Review via email: mp+280083@code.launchpad.net

Commit message

Do not allow interacting with tabs and the chrome while the settings page is shown.

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :

PASSED: Continuous integration, rev:1301
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/2518/
Executed test runs:
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/5687
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/1272
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/1272
        deb: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/1272/artifact/work/output/*zip*/output.zip
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/1272
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-touch/4416
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/5700
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/5700/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/25912

Click here to trigger a rebuild:
http://s-jenkins.ubuntu-ci:8080/job/webbrowser-app-ci/2518/rebuild

review: Approve (continuous-integration)
Revision history for this message
Ugo Riboni (uriboni) wrote :

Works well and code LGTM

review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/app/webbrowser/Browser.qml'
2--- src/app/webbrowser/Browser.qml 2015-12-21 20:39:33 +0000
3+++ src/app/webbrowser/Browser.qml 2015-12-22 16:03:10 +0000
4@@ -172,7 +172,7 @@
5
6 FocusScope {
7 anchors.fill: parent
8- visible: !settingsContainer.visible && !historyViewLoader.active && !bookmarksViewLoader.active && !downloadsContainer.visible
9+ visible: !settingsViewLoader.active && !historyViewLoader.active && !bookmarksViewLoader.active && !downloadsContainer.visible
10
11 FocusScope {
12 id: tabContainer
13@@ -223,7 +223,7 @@
14
15 anchors {
16 fill: tabContainer
17- topMargin: (chrome.state == "shown" && chrome.visible) ? chrome.height : 0
18+ topMargin: (chrome.state == "shown") ? chrome.height : 0
19 }
20
21 // Avoid loading the new tab view if the webview is about to load
22@@ -571,11 +571,7 @@
23 objectName: "settings"
24 text: i18n.tr("Settings")
25 iconName: "settings"
26- onTriggered: {
27- settingsComponent.createObject(settingsContainer)
28- settingsContainer.focus = true
29- chrome.findInPageMode = false
30- }
31+ onTriggered: settingsViewLoader.active = true
32 }
33 ]
34
35@@ -896,25 +892,34 @@
36 }
37 }
38
39- FocusScope {
40- id: settingsContainer
41+ Loader {
42+ id: settingsViewLoader
43
44- visible: children.length > 0
45 anchors.fill: parent
46-
47- Component {
48- id: settingsComponent
49-
50- SettingsPage {
51- anchors.fill: parent
52- focus: true
53- settingsObject: settings
54- onDone: destroy()
55- Keys.onEscapePressed: {
56- destroy()
57- internal.resetFocus()
58- }
59- }
60+ active: false
61+
62+ onStatusChanged: {
63+ if (status == Loader.Ready) {
64+ settingsViewLoader.item.forceActiveFocus()
65+ } else {
66+ internal.resetFocus()
67+ }
68+ }
69+
70+ Keys.onEscapePressed: settingsViewLoader.active = false
71+
72+ onActiveChanged: {
73+ if (active) {
74+ chrome.findInPageMode = false
75+ forceActiveFocus()
76+ }
77+ }
78+
79+ sourceComponent: SettingsPage {
80+ anchors.fill: parent
81+ focus: true
82+ settingsObject: settings
83+ onDone: settingsViewLoader.active = false
84 }
85 }
86
87@@ -1813,13 +1818,13 @@
88 KeyboardShortcut {
89 modifiers: Qt.ControlModifier
90 key: Qt.Key_Tab
91- enabled: (chrome.visible || recentView.visible) && !downloadsContainer.visible
92+ enabled: tabContainer.visible || recentView.visible
93 onTriggered: internal.switchToNextTab()
94 }
95 KeyboardShortcut {
96 modifiers: Qt.ControlModifier
97 key: Qt.Key_PageDown
98- enabled: (chrome.visible || recentView.visible) && !downloadsContainer.visible
99+ enabled: tabContainer.visible || recentView.visible
100 onTriggered: internal.switchToNextTab()
101 }
102
103@@ -1827,13 +1832,13 @@
104 KeyboardShortcut {
105 modifiers: Qt.ControlModifier
106 key: Qt.Key_Backtab
107- enabled: (chrome.visible || recentView.visible) && !downloadsContainer.visible
108+ enabled: tabContainer.visible || recentView.visible
109 onTriggered: internal.switchToPreviousTab()
110 }
111 KeyboardShortcut {
112 modifiers: Qt.ControlModifier
113 key: Qt.Key_PageUp
114- enabled: (chrome.visible || recentView.visible) && !downloadsContainer.visible
115+ enabled: tabContainer.visible || recentView.visible
116 onTriggered: internal.switchToPreviousTab()
117 }
118
119@@ -1841,14 +1846,14 @@
120 KeyboardShortcut {
121 modifiers: Qt.ControlModifier | Qt.ShiftModifier
122 key: Qt.Key_W
123- enabled: (chrome.visible || recentView.visible) && !downloadsContainer.visible
124+ enabled: tabContainer.visible || recentView.visible
125 onTriggered: internal.undoCloseTab()
126 }
127
128 KeyboardShortcut {
129 modifiers: Qt.ControlModifier | Qt.ShiftModifier
130 key: Qt.Key_T
131- enabled: (chrome.visible || recentView.visible) && !downloadsContainer.visible
132+ enabled: tabContainer.visible || recentView.visible
133 onTriggered: internal.undoCloseTab()
134 }
135
136@@ -1856,13 +1861,13 @@
137 KeyboardShortcut {
138 modifiers: Qt.ControlModifier
139 key: Qt.Key_W
140- enabled: (chrome.visible || recentView.visible) && !downloadsContainer.visible
141+ enabled: tabContainer.visible || recentView.visible
142 onTriggered: internal.closeCurrentTab()
143 }
144 KeyboardShortcut {
145 modifiers: Qt.ControlModifier
146 key: Qt.Key_F4
147- enabled: (chrome.visible || recentView.visible) && !downloadsContainer.visible
148+ enabled: tabContainer.visible || recentView.visible
149 onTriggered: internal.closeCurrentTab()
150 }
151
152@@ -1870,11 +1875,11 @@
153 KeyboardShortcut {
154 modifiers: Qt.ControlModifier
155 key: Qt.Key_T
156- enabled: (chrome.visible || recentView.visible || bookmarksViewLoader.active || historyViewLoader.active) && !downloadsContainer.visible
157+ enabled: tabContainer.visible || recentView.visible ||
158+ bookmarksViewLoader.active || historyViewLoader.active
159 onTriggered: {
160 openUrlInNewTab("", true)
161 if (recentView.visible) recentView.reset()
162-
163 bookmarksViewLoader.active = false
164 historyViewLoader.active = false
165 }
166@@ -1884,18 +1889,18 @@
167 KeyboardShortcut {
168 modifiers: Qt.ControlModifier
169 key: Qt.Key_L
170- enabled: chrome.visible && !downloadsContainer.visible
171+ enabled: tabContainer.visible
172 onTriggered: internal.focusAddressBar(true)
173 }
174 KeyboardShortcut {
175 modifiers: Qt.AltModifier
176 key: Qt.Key_D
177- enabled: chrome.visible && !downloadsContainer.visible
178+ enabled: tabContainer.visible
179 onTriggered: internal.focusAddressBar(true)
180 }
181 KeyboardShortcut {
182 key: Qt.Key_F6
183- enabled: chrome.visible && !downloadsContainer.visible
184+ enabled: tabContainer.visible
185 onTriggered: internal.focusAddressBar(true)
186 }
187
188@@ -1903,7 +1908,7 @@
189 KeyboardShortcut {
190 modifiers: Qt.ControlModifier
191 key: Qt.Key_D
192- enabled: chrome.visible && !downloadsContainer.visible
193+ enabled: tabContainer.visible
194 onTriggered: {
195 if (currentWebview) {
196 if (BookmarksModel.contains(currentWebview.url)) {
197@@ -1919,7 +1924,7 @@
198 KeyboardShortcut {
199 modifiers: Qt.ControlModifier
200 key: Qt.Key_H
201- enabled: chrome.visible && !downloadsContainer.visible
202+ enabled: tabContainer.visible
203 onTriggered: historyViewLoader.active = true
204 }
205
206@@ -1927,7 +1932,7 @@
207 KeyboardShortcut {
208 modifiers: Qt.ControlModifier | Qt.ShiftModifier
209 key: Qt.Key_O
210- enabled: chrome.visible && !downloadsContainer.visible
211+ enabled: tabContainer.visible
212 onTriggered: bookmarksViewLoader.active = true
213 }
214
215@@ -1935,12 +1940,12 @@
216 KeyboardShortcut {
217 modifiers: Qt.AltModifier
218 key: Qt.Key_Left
219- enabled: chrome.visible && !downloadsContainer.visible
220+ enabled: tabContainer.visible
221 onTriggered: internal.historyGoBack()
222 }
223 KeyboardShortcut {
224 key: Qt.Key_Backspace
225- enabled: chrome.visible && !downloadsContainer.visible
226+ enabled: tabContainer.visible
227 onTriggered: internal.historyGoBack()
228 }
229
230@@ -1948,26 +1953,26 @@
231 KeyboardShortcut {
232 modifiers: Qt.AltModifier
233 key: Qt.Key_Right
234- enabled: chrome.visible && !downloadsContainer.visible
235+ enabled: tabContainer.visible
236 onTriggered: internal.historyGoForward()
237 }
238 KeyboardShortcut {
239 modifiers: Qt.ShiftModifier
240 key: Qt.Key_Backspace
241- enabled: chrome.visible && !downloadsContainer.visible
242+ enabled: tabContainer.visible
243 onTriggered: internal.historyGoForward()
244 }
245
246 // F5 or Ctrl+R: Reload current Tab
247 KeyboardShortcut {
248 key: Qt.Key_F5
249- enabled: chrome.visible && !downloadsContainer.visible
250+ enabled: tabContainer.visible
251 onTriggered: if (currentWebview) currentWebview.reload()
252 }
253 KeyboardShortcut {
254 modifiers: Qt.ControlModifier
255 key: Qt.Key_R
256- enabled: chrome.visible && !downloadsContainer.visible
257+ enabled: tabContainer.visible
258 onTriggered: if (currentWebview) currentWebview.reload()
259 }
260
261@@ -1975,7 +1980,7 @@
262 KeyboardShortcut {
263 modifiers: Qt.ControlModifier
264 key: Qt.Key_F
265- enabled: !newTabViewLoader.active && !bookmarksViewLoader.active && !downloadsContainer.visible
266+ enabled: tabContainer.visible && !newTabViewLoader.active
267 onTriggered: chrome.findInPageMode = true
268 }
269
270
271=== modified file 'tests/autopilot/webbrowser_app/tests/test_keyboard.py'
272--- tests/autopilot/webbrowser_app/tests/test_keyboard.py 2015-12-04 10:51:57 +0000
273+++ tests/autopilot/webbrowser_app/tests/test_keyboard.py 2015-12-22 16:03:10 +0000
274@@ -87,7 +87,6 @@
275
276 def test_new_tab(self):
277 self.main_window.press_key('Ctrl+t')
278-
279 webview = self.main_window.get_current_webview()
280 self.assertThat(webview.url, Equals(""))
281 new_tab_view = self.main_window.get_new_tab_view()
282@@ -400,6 +399,19 @@
283 self.main_window.press_key('Escape')
284 bookmarks_view.wait_until_destroyed()
285
286+ def test_open_settings_exits_findinpage(self):
287+ address_bar = self.main_window.chrome.address_bar
288+ self.main_window.press_key('Ctrl+f')
289+ self.assertThat(address_bar.findInPageMode, Eventually(Equals(True)))
290+ self.assertThat(address_bar.activeFocus, Eventually(Equals(True)))
291+
292+ settings = self.open_settings()
293+ self.assertThat(address_bar.findInPageMode, Eventually(Equals(False)))
294+ self.assertThat(address_bar.activeFocus, Eventually(Equals(False)))
295+
296+ self.main_window.press_key('Escape')
297+ settings.wait_until_destroyed()
298+
299 def test_escape_settings(self):
300 settings = self.open_settings()
301 self.main_window.press_key('Escape')
302@@ -437,3 +449,31 @@
303 self.main_window.press_key('Up')
304 self.assertThat(new_tab_view.activeFocus, Eventually(Equals(False)))
305 self.assertThat(address_bar.activeFocus, Eventually(Equals(True)))
306+
307+ def test_ctrl_w_closes_tabs_only_from_main_view(self):
308+ # Verify that Ctrl+w doesn’t work when either the history
309+ # view, the bookmarks view or the settings view are shown
310+ # (https://launchpad.net/bugs/1524353).
311+ self.main_window.press_key('Ctrl+t')
312+ self.assert_number_webviews_eventually(2)
313+
314+ history_view = self.open_history()
315+ self.main_window.press_key('Ctrl+w')
316+ self.main_window.press_key('Escape')
317+ history_view.wait_until_destroyed()
318+ self.assert_number_webviews_eventually(2)
319+
320+ bookmarks_view = self.open_bookmarks()
321+ self.main_window.press_key('Ctrl+w')
322+ self.main_window.press_key('Escape')
323+ bookmarks_view.wait_until_destroyed()
324+ self.assert_number_webviews_eventually(2)
325+
326+ settings_view = self.open_settings()
327+ self.main_window.press_key('Ctrl+w')
328+ self.main_window.press_key('Escape')
329+ settings_view.wait_until_destroyed()
330+ self.assert_number_webviews_eventually(2)
331+
332+ self.main_window.press_key('Ctrl+w')
333+ self.assert_number_webviews_eventually(1)

Subscribers

People subscribed via source and target branches

to status/vote changes: