Merge lp:~rpadovani/webbrowser-app/addressBarFullWidth into lp:webbrowser-app

Proposed by Riccardo Padovani
Status: Needs review
Proposed branch: lp:~rpadovani/webbrowser-app/addressBarFullWidth
Merge into: lp:webbrowser-app
Diff against target: 162 lines (+33/-5)
5 files modified
debian/control (+2/-0)
src/app/webbrowser/AddressBar.qml (+10/-2)
src/app/webbrowser/Chrome.qml (+9/-3)
tests/autopilot/webbrowser_app/tests/test_addressbar_states.py (+10/-0)
tests/autopilot/webbrowser_app/tests/test_suggestions.py (+2/-0)
To merge this branch: bzr merge lp:~rpadovani/webbrowser-app/addressBarFullWidth
Reviewer Review Type Date Requested Status
PS Jenkins bot continuous-integration Needs Fixing
Olivier Tilloy Needs Fixing
Review via email: mp+239039@code.launchpad.net

Commit message

Set addressbar at full width when is focused, as per design

Description of the change

Set addressbar at full width when is focused, as per design

To post a comment you must log in.
Revision history for this message
PS Jenkins bot (ps-jenkins) wrote :
review: Needs Fixing (continuous-integration)
Revision history for this message
Olivier Tilloy (osomon) wrote :

The change looks good, but it breaks a large number of autopilot tests in at least two different ways:

 - Some tests focus the address bar and then try to open the drawer, but the button is hidden. Some of these tests can simply be removed, and some will need to be rewritten to account for the new behaviour.

 - A number of tests focus the address bar and then try to click the clear button, but since the width of the address bar is animated, in the time it takes for the cursor to move to where the clear button was, it has moved away, and the click happens outside the button. This is mostly an issue on desktop, but could potentially affect touch devices as well. We need a way for the autopilot tests to wait until the address bar has finished expanding before clicking the clear button.

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

Sorry, but I needed 2 months to understand autopilot :P

Jokes aside, I deleted test_cannot_bookmark_empty_page because when you open a new tab the address bar is focused and users cannot unfocus it, so he can't click anymore the bookmark menu.

I also deleted test_suggestions_hidden_while_drawer_open because now user can't open the drawer while the address bar is focused

I fixed test_selecting_tab_focuses_webview adding a click to the middle of the page to unfocus the address bar

I fixed also test_special_characters adding a delay before the clear button is clicked

test_looses_focus_when_reloading it's a special case: I fixed it (so now it clicks on the button), but fails anywhere: this evidence a real problem. Indeed, on top of some pixels at the center of the reload icon, (where the autopilot presses), there is a MouseArea that activates the tooltip for copy/paste. I spend some time on it, but I didn't understand the root cause. I'll do further investigation in next days. Meanwhile, if you want, try to take a look.

I think is a toolkit bug - for sure it's not an autopilot issue, because I'm able to reproduce it, so before merging that branch I have to fix the problem

763. By Riccardo Padovani

Merged from trunk

764. By Riccardo Padovani

fixing autopilot

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

Hide left action in the address bar when editing

766. By Riccardo Padovani

Fixed webbrowser_app.tests.test_addressbar_states.TestAddressBarStates.test_looses_focus_when_reloading

Revision history for this message
Riccardo Padovani (rpadovani) wrote :

Actually, upstream fixed bug #1384278, so per design we can hide the left action in the edit mode.

So I changed the code to hide left action in edit mode and to change keyboard status.

I also changed webbrowser_app.tests.test_addressbar_states.TestAddressBarStates.test_looses_focus_when_reloading to use "Enter" on keyboard instead of clicking the reload button.

The branch now should be ready to be merged

767. By Riccardo Padovani

Merged from trunk

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

There are several build failures that need fixing:

  tests/autopilot/webbrowser_app/tests/test_addressbar_states.py:51:44: F821 undefined name 'Eventually'
  tests/autopilot/webbrowser_app/tests/test_addressbar_states.py:51:55: F821 undefined name 'Equals'

and

  src/app/webbrowser/AddressBar.qml:20:1: module "Ubuntu.Keyboard" is not installed
     import Ubuntu.Keyboard 0.1

review: Needs Fixing
Revision history for this message
Olivier Tilloy (osomon) wrote :

28 + visible: addressbar.state != "editing"
76 + visible: addressbar.state != 'editing'
85 + visible: enabled && addressbar.state != 'editing'
110 + visible: addressbar.state != 'editing'

The "editing" state of AddressBar was recently removed. Instead, use addressbar.activeFocus.

review: Needs Fixing
768. By Riccardo Padovani

Fixed autopilot import

769. By Riccardo Padovani

Added qtdeclarative5-ubuntu-keyboard-extensions0.1 to debian/control

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

merged from upstream

771. By Riccardo Padovani

Sobstituted addressbar.state != 'editing' with addressbar.activeFocus

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

The jenkins node for the i386 build failed, I've restarted a new ci run.

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

There is still one comparison of addressbar.state with "editing" in src/app/webbrowser/AddressBar.qml.

review: Needs Fixing
Revision history for this message
Olivier Tilloy (osomon) wrote :

The change to test_looses_focus_when_reloading makes the test meaningless (or at the very least the name needs to be updated too): the test was about clicking on the reload button, not validating the current URL, which takes a different code path.

review: Needs Fixing
Revision history for this message
Olivier Tilloy (osomon) wrote :

> So I changed the code to hide left action in edit mode and to change keyboard
> status.

I’m not sure it should always be hidden. At the very least, not on desktop where there is no OSK by default.

Revision history for this message
Riccardo Padovani (rpadovani) wrote :

Okay, so:
- reload icon is hiddend only when virtual keyboard is visibile
- test runs only on desktop

as we discussed Friday

The test fails, but I don't think it's related to this branch, because it fails also on trunk: there is a little square in the middle of the reload icon where, for I don't know which reason, the click opens the copy/paste dialog

772. By Riccardo Padovani

Implemented case for physical keyboard

773. By Riccardo Padovani

fixed test_looses_focus_when_reloading

774. By Riccardo Padovani

merged from upstream

Revision history for this message
Riccardo Padovani (rpadovani) wrote :

Argh, forgot to push, sorry :S

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

Merged from upstream and fixed conflicts

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

Merged from trunk

777. By Riccardo Padovani

removed qtdeclarative5-private-dev

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

Merge from upstream and fix conflicts

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

Unmerged revisions

778. By Riccardo Padovani

Merge from upstream and fix conflicts

777. By Riccardo Padovani

removed qtdeclarative5-private-dev

776. By Riccardo Padovani

Merged from trunk

775. By Riccardo Padovani

Merged from upstream and fixed conflicts

774. By Riccardo Padovani

merged from upstream

773. By Riccardo Padovani

fixed test_looses_focus_when_reloading

772. By Riccardo Padovani

Implemented case for physical keyboard

771. By Riccardo Padovani

Sobstituted addressbar.state != 'editing' with addressbar.activeFocus

770. By Riccardo Padovani

merged from upstream

769. By Riccardo Padovani

Added qtdeclarative5-ubuntu-keyboard-extensions0.1 to debian/control

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'debian/control'
--- debian/control 2015-05-27 05:31:12 +0000
+++ debian/control 2015-06-12 09:40:41 +0000
@@ -21,6 +21,7 @@
21 qtdeclarative5-dev,21 qtdeclarative5-dev,
22 qtdeclarative5-private-dev,22 qtdeclarative5-private-dev,
23 qtdeclarative5-ubuntu-ui-toolkit-plugin,23 qtdeclarative5-ubuntu-ui-toolkit-plugin,
24 qtdeclarative5-ubuntu-keyboard-extensions0.1,
24 xvfb,25 xvfb,
25Standards-Version: 3.9.526Standards-Version: 3.9.5
26Homepage: https://launchpad.net/webbrowser-app27Homepage: https://launchpad.net/webbrowser-app
@@ -41,6 +42,7 @@
41 qml-module-qtquick2 (>= 5.4) | qtdeclarative5-qtquick2-plugin (>= 5.4),42 qml-module-qtquick2 (>= 5.4) | qtdeclarative5-qtquick2-plugin (>= 5.4),
42 qml-module-qtquick-dialogs | qtdeclarative5-dialogs-plugin,43 qml-module-qtquick-dialogs | qtdeclarative5-dialogs-plugin,
43 qml-module-qtquick-window2 | qtdeclarative5-window-plugin,44 qml-module-qtquick-window2 | qtdeclarative5-window-plugin,
45 qtdeclarative5-ubuntu-keyboard-extensions0.1,
44 qtdeclarative5-ubuntu-ui-toolkit-plugin,46 qtdeclarative5-ubuntu-ui-toolkit-plugin,
45 qtdeclarative5-ubuntu-web-plugin (= ${binary:Version}),47 qtdeclarative5-ubuntu-web-plugin (= ${binary:Version}),
46 qtdeclarative5-unity-action-plugin,48 qtdeclarative5-unity-action-plugin,
4749
=== modified file 'src/app/webbrowser/AddressBar.qml'
--- src/app/webbrowser/AddressBar.qml 2015-05-26 21:42:54 +0000
+++ src/app/webbrowser/AddressBar.qml 2015-06-12 09:40:41 +0000
@@ -17,6 +17,7 @@
17 */17 */
1818
19import QtQuick 2.019import QtQuick 2.0
20import Ubuntu.Keyboard 0.1
20import Ubuntu.Components 1.121import Ubuntu.Components 1.1
21import Ubuntu.Components.Popups 1.022import Ubuntu.Components.Popups 1.0
22import com.canonical.Oxide 1.0 as Oxide23import com.canonical.Oxide 1.0 as Oxide
@@ -57,9 +58,11 @@
57 primaryItem: Item {58 primaryItem: Item {
58 id: icons59 id: icons
5960
60 width: iconsRow.width + units.gu(1)61 width: visible ? iconsRow.width + units.gu(1) : 0
61 height: units.gu(2)62 height: units.gu(2)
6263
64 visible: !(Qt.inputMethod.visible && addressbar.activeFocus)
65
63 Row {66 Row {
64 id: iconsRow67 id: iconsRow
6568
@@ -84,7 +87,8 @@
84 height: parent.height87 height: parent.height
85 width: height88 width: height
8689
87 visible: addressbar.activeFocus || addressbar.loading || !addressbar.text90 visible: (addressbar.activeFocus && !Qt.inputMethod.visible) ||
91 addressbar.loading || !addressbar.text
8892
89 enabled: addressbar.text93 enabled: addressbar.text
90 opacity: enabled ? 1.0 : 0.394 opacity: enabled ? 1.0 : 0.3
@@ -97,6 +101,10 @@
97 reload ? "reload" :101 reload ? "reload" :
98 looksLikeAUrl ? "stock_website" : "search"102 looksLikeAUrl ? "stock_website" : "search"
99103
104 InputMethod.extensions: {
105 "enterKeyIconSource": action.reload ? "reload" : "keyboard-enter"
106 }
107
100 MouseArea {108 MouseArea {
101 objectName: "actionButton"109 objectName: "actionButton"
102110
103111
=== modified file 'src/app/webbrowser/Chrome.qml'
--- src/app/webbrowser/Chrome.qml 2015-05-26 21:42:54 +0000
+++ src/app/webbrowser/Chrome.qml 2015-06-12 09:40:41 +0000
@@ -50,7 +50,9 @@
50 iconColor: internal.iconColor50 iconColor: internal.iconColor
5151
52 height: chrome.height52 height: chrome.height
53 width: height * 0.853 width: visible ? height * 0.8 : 0
54
55 visible: !addressbar.activeFocus
5456
55 anchors {57 anchors {
56 left: parent.left58 left: parent.left
@@ -70,7 +72,7 @@
70 iconColor: internal.iconColor72 iconColor: internal.iconColor
7173
72 height: chrome.height74 height: chrome.height
73 visible: enabled75 visible: enabled && !addressbar.activeFocus
74 width: visible ? height * 0.8 : 076 width: visible ? height * 0.8 : 0
7577
76 anchors {78 anchors {
@@ -103,10 +105,12 @@
103 chrome.webview.forceActiveFocus()105 chrome.webview.forceActiveFocus()
104 chrome.webview.url = requestedUrl106 chrome.webview.url = requestedUrl
105 }107 }
108
106 onRequestReload: {109 onRequestReload: {
107 chrome.webview.forceActiveFocus()110 chrome.webview.forceActiveFocus()
108 chrome.webview.reload()111 chrome.webview.reload()
109 }112 }
113
110 onRequestStop: chrome.webview.stop()114 onRequestStop: chrome.webview.stop()
111115
112 Connections {116 Connections {
@@ -136,7 +140,9 @@
136 iconColor: internal.iconColor140 iconColor: internal.iconColor
137141
138 height: chrome.height142 height: chrome.height
139 width: height * 0.8143 width: visible ? height * 0.8: 0
144
145 visible: !addressbar.activeFocus
140146
141 anchors {147 anchors {
142 right: parent.right148 right: parent.right
143149
=== modified file 'tests/autopilot/webbrowser_app/tests/test_addressbar_states.py'
--- tests/autopilot/webbrowser_app/tests/test_addressbar_states.py 2015-02-17 20:52:30 +0000
+++ tests/autopilot/webbrowser_app/tests/test_addressbar_states.py 2015-06-12 09:40:41 +0000
@@ -14,8 +14,14 @@
14# You should have received a copy of the GNU General Public License14# You should have received a copy of the GNU General Public License
15# along with this program. If not, see <http://www.gnu.org/licenses/>.15# along with this program. If not, see <http://www.gnu.org/licenses/>.
1616
17from testtools.matchers import Equals
18from autopilot.matchers import Eventually
19from autopilot.platform import model
20
17from webbrowser_app.tests import StartOpenRemotePageTestCaseBase21from webbrowser_app.tests import StartOpenRemotePageTestCaseBase
1822
23import unittest
24
1925
20class TestAddressBarStates(StartOpenRemotePageTestCaseBase):26class TestAddressBarStates(StartOpenRemotePageTestCaseBase):
2127
@@ -42,6 +48,7 @@
42 self.main_window.go_to_url(url)48 self.main_window.go_to_url(url)
43 address_bar.activeFocus.wait_for(False)49 address_bar.activeFocus.wait_for(False)
4450
51 @unittest.skipIf(model() != "Desktop", "on desktop only")
45 def test_looses_focus_when_reloading(self):52 def test_looses_focus_when_reloading(self):
46 address_bar = self.main_window.address_bar53 address_bar = self.main_window.address_bar
47 self.pointing_device.click_object(address_bar)54 self.pointing_device.click_object(address_bar)
@@ -51,5 +58,8 @@
51 # button.58 # button.
52 address_bar.clear()59 address_bar.clear()
53 address_bar.write(self.url)60 address_bar.write(self.url)
61 chrome = self.main_window.chrome
62 drawer_button = chrome.get_drawer_button()
63 self.assertThat(drawer_button.opacity, Eventually(Equals(1)))
54 address_bar.click_action_button()64 address_bar.click_action_button()
55 address_bar.activeFocus.wait_for(False)65 address_bar.activeFocus.wait_for(False)
5666
=== modified file 'tests/autopilot/webbrowser_app/tests/test_suggestions.py'
--- tests/autopilot/webbrowser_app/tests/test_suggestions.py 2015-05-20 05:45:29 +0000
+++ tests/autopilot/webbrowser_app/tests/test_suggestions.py 2015-06-12 09:40:41 +0000
@@ -285,6 +285,8 @@
285 self.assertThat(webview.url, Eventually(Contains(url)))285 self.assertThat(webview.url, Eventually(Contains(url)))
286286
287 def test_special_characters(self):287 def test_special_characters(self):
288 self.address_bar.focus()
289 self.assert_suggestions_eventually_shown()
288 self.address_bar.clear()290 self.address_bar.clear()
289 self.address_bar.write('(phil')291 self.address_bar.write('(phil')
290 self.assert_suggestions_eventually_shown()292 self.assert_suggestions_eventually_shown()

Subscribers

People subscribed via source and target branches

to status/vote changes: