Merge lp:~uriboni/webbrowser-app/larger-close-tab-button into lp:webbrowser-app

[Ugo Riboni]

Extend the clickable area to close a tab on mobile, as taps are less precise and often end up missing it.

Extend the clickable area to close a tab on mobile, as taps are less precise and often end up missing it

[Olivier Tilloy (osomon)]

With this change the area looks correct on mobile, but it’s not on desktop.

To verify that, I added a semi-transparent red rectangle that fills the abstract button, and this is how it looks:

 - on desktop: http://people.canonical.com/~osomon/tab-close-button-desktop.png
 - on mobile: http://people.canonical.com/~osomon/tab-close-button-mobile.png

On desktop, the area shouldn’t cover the entire height of the tab, it should be limited to the height of the X icon. And the horizontal center of the area and that of the X icon should be aligned.

review: Needs Fixing

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1206
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/2288/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/4373
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/1042
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/1042
        deb: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/1042/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/1042/console
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/3563
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/4370
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/4370/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/23697

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

review: Needs Fixing (continuous-integration)

[lp:~uriboni/webbrowser-app/larger-close-tab-button]

1207. By Ugo Riboni on 2015-09-28

Confine the close tab button to be as big as the icon on dekstop, while it covers the whole right hand side of the tab on mobile

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1207
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/2289/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/4376
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/1043/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/1043
        deb: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/1043/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/1043/console
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/3564
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/4373
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/4373/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/23703

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

review: Needs Fixing (continuous-integration)

[Olivier Tilloy (osomon)]

There are non-trivial conflicts when merging into the latest trunk. Can you please address them?

review: Needs Fixing

[lp:~uriboni/webbrowser-app/larger-close-tab-button]

1208. By Ugo Riboni on 2015-09-29

Merge changes from trunk

1209. By Ugo Riboni on 2015-09-29

Merge branch: Allow closing a tab by middle-clicking on its X icon + tests

1210. By Ugo Riboni on 2015-09-29

Revert whitespace change

[Olivier Tilloy (osomon)]

That works well, but:

 - Is the change to the right anchor of mouseArea really needed? On mobile the closeButton mouse area hides mouseArea entirely anyway, no?

 - I find the anchoring code in closeButton hard to read, because the conditions are not consistent. Can you maybe add a read-only boolean property to mouseArea, and use that property consistently in anchors assignments? E.g.:

    readonly property bool mobile: formFactor == "mobile"
    anchors {
        fill: mobile ? undefined : closeIcon
        …
    }

[lp:~uriboni/webbrowser-app/larger-close-tab-button]

1211. By Ugo Riboni on 2015-09-29

Clarify and simplify anchoring

[Olivier Tilloy (osomon)]

LGTM now, thanks!

review: Approve

[PS Jenkins bot (ps-jenkins)]

FAILED: Continuous integration, rev:1209
http://jenkins.qa.ubuntu.com/job/webbrowser-app-ci/2295/
Executed test runs:
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-vivid-touch/4397
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-amd64-ci/1049/console
    SUCCESS: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/1049
        deb: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-armhf-ci/1049/artifact/work/output/*zip*/output.zip
    FAILURE: http://jenkins.qa.ubuntu.com/job/webbrowser-app-vivid-i386-ci/1049/console
    UNSTABLE: http://jenkins.qa.ubuntu.com/job/generic-deb-autopilot-runner-vivid-mako/3579
    SUCCESS: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/4394
        deb: http://jenkins.qa.ubuntu.com/job/generic-mediumtests-builder-vivid-armhf/4394/artifact/work/output/*zip*/output.zip
    SUCCESS: http://s-jenkins.ubuntu-ci:8080/job/touch-flash-device/23740

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

review: Needs Fixing (continuous-integration)

=== modified file 'src/app/webbrowser/TabItem.qml'
--- src/app/webbrowser/TabItem.qml	2015-09-28 08:10:09 +0000
+++ src/app/webbrowser/TabItem.qml	2015-09-29 09:32:02 +0000
@@ -98,7 +98,7 @@
             anchors.left: parent.left
             anchors.top: parent.top
             anchors.bottom: parent.bottom
-            anchors.right: closeButton.left
+            anchors.right: parent.right
             acceptedButtons: Qt.AllButtons
             onPressed: {
                 if (mouse.button === Qt.LeftButton) {
@@ -118,21 +118,29 @@
             id: closeButton
             objectName: "closeButton"
 
-            anchors.top: parent.top
-            anchors.bottom: parent.bottom
-            anchors.right: parent.right
-            width: units.gu(2)
+            acceptedButtons: Qt.LeftButton | Qt.MiddleButton
 
-            Icon {
-                height: units.gu(1.5)
-                width: height
-                anchors.right: parent.right
-                anchors.rightMargin: units.gu(1)
-                anchors.verticalCenter: parent.verticalCenter
-                name: "close"
-            }
+            // On mobile the tap area to close the tab occupies the whole right
+            // hand side of the tab, while it covers only the close icon in
+            // other form factors
+            readonly property bool mobile: formFactor == "mobile"
+            anchors.fill: mobile ? undefined : closeIcon
+            anchors.top: mobile ? parent.top : undefined
+            anchors.bottom: mobile ? parent.bottom : undefined
+            anchors.right: mobile ? parent.right : undefined
+            width: mobile ? units.gu(4) : closeIcon.width
 
             onClicked: closed()
         }
+
+        Icon {
+            id: closeIcon
+            height: units.gu(1.5)
+            width: height
+            anchors.right: parent.right
+            anchors.rightMargin: units.gu(1)
+            anchors.verticalCenter: parent.verticalCenter
+            name: "close"
+        }
     }
 }
 
=== modified file 'tests/unittests/qml/tst_TabsBar.qml'
--- tests/unittests/qml/tst_TabsBar.qml	2015-09-24 05:41:14 +0000
+++ tests/unittests/qml/tst_TabsBar.qml	2015-09-29 09:32:02 +0000
@@ -211,12 +211,19 @@
             compare(tabsModel.currentIndex, 1)
         }
 
-        function test_close_tabs() {
+        function test_close_tabs_data() {
+            return [
+                {button: Qt.LeftButton},
+                {button: Qt.MiddleButton}
+            ]
+        }
+
+        function test_close_tabs(data) {
             populateTabs()
             for (var i = 2; i >= 0; --i) {
                 var tab0 = getTabDelegate(0)
                 var closeButton = findChild(tab0, "closeButton")
-                clickItem(closeButton, Qt.LeftButton)
+                clickItem(closeButton, data.button)
                 compare(tabsModel.count, i)
             }
         }