Merge lp:~fboucault/webbrowser-app/switch_to_parent_tab_on_close into lp:webbrowser-app/staging

[Florian Boucault]

Restore focus on parent tab when closing child tab

Restore focus on parent tab when closing child tab

[Olivier Tilloy (osomon)]

flake8 unit test is failing.

review: Needs Fixing

[lp:~fboucault/webbrowser-app/switch_to_parent_tab_on_close]

1624. By Florian Boucault on 2017-03-03

Fixed flake8

[Florian Boucault (fboucault)]

> flake8 unit test is failing.

Fixed

Unmerged revisions

1624. By Florian Boucault on 2017-03-03

Fixed flake8

1623. By Florian Boucault on 2017-02-22

Added corresponding AP tests

1622. By Florian Boucault on 2017-02-21

Restore focus on parent tab when closing child tab

1621. By Florian Boucault on 2017-02-21

AP test: simplified tab closing

=== modified file 'src/app/webbrowser/Browser.qml'
--- src/app/webbrowser/Browser.qml	2017-02-16 17:25:49 +0000
+++ src/app/webbrowser/Browser.qml	2017-03-03 14:26:48 +0000
@@ -87,6 +87,7 @@
     function serializeTabState(tab) {
         var state = {}
         state.uniqueId = tab.uniqueId
+        state.parentUniqueId = tab.parentUniqueId
         state.url = tab.url.toString()
         state.title = tab.title
         state.icon = tab.icon.toString()
@@ -98,6 +99,7 @@
     function restoreTabState(state) {
         var properties = {'initialUrl': state.url, 'initialTitle': state.title,
                           'uniqueId': state.uniqueId, 'initialIcon': state.icon,
+                          'parentUniqueId': state.parentUniqueId,
                           'preview': state.preview, 'restoreState': state.savedState,
                           'restoreType': Oxide.WebView.RestoreLastSessionExitedCleanly}
         return createTab(properties)
@@ -1047,9 +1049,10 @@
             if (share) share.shareText(text)
         }
 
-        function openUrlInNewTab(url, setCurrent, load, index) {
+        function openUrlInNewTab(url, setCurrent, load, index, parentUniqueId) {
             load = typeof load !== 'undefined' ? load : true
-            var tab = internal.createTabHelper({"initialUrl": url})
+            var tab = internal.createTabHelper({"initialUrl": url,
+                                                "parentUniqueId": parentUniqueId})
             addTab(tab, setCurrent, index)
             if (load) {
                 tab.load()
@@ -1096,6 +1099,13 @@
             moving = moving === undefined ? false : moving;
 
             var tab = tabsModel.get(index)
+            if (tab.parentUniqueId) {
+                var parentIndex = -1
+                parentIndex = tabsModel.match("uniqueId", tab.parentUniqueId)
+                if (parentIndex != -1) {
+                    internal.switchToTab(parentIndex)
+                }
+            }
             tabsModel.remove(index)
 
             if (tab) {

=== modified file 'src/app/webbrowser/BrowserTab.qml'
--- src/app/webbrowser/BrowserTab.qml	2017-02-16 17:25:49 +0000
+++ src/app/webbrowser/BrowserTab.qml	2017-03-03 14:26:48 +0000
@@ -28,6 +28,7 @@
     id: tab
 
     property string uniqueId: this.toString() + "-" + Date.now()
+    property string parentUniqueId
     property url initialUrl
     property string initialTitle
     property url initialIcon

=== modified file 'src/app/webbrowser/TabComponent.qml'
--- src/app/webbrowser/TabComponent.qml	2017-02-15 11:22:05 +0000
+++ src/app/webbrowser/TabComponent.qml	2017-03-03 14:26:48 +0000
@@ -86,13 +86,15 @@
                     objectName: "OpenLinkInNewTabContextualAction"
                     enabled: contextModel && contextModel.linkUrl.toString()
                     onTriggered: internal.openUrlInNewTab(contextModel.linkUrl, true,
-                                                          true, tabsModel.indexOf(browserTab) + 1)
+                                                          true, tabsModel.indexOf(browserTab) + 1,
+                                                          tab.uniqueId)
                 }
                 Actions.OpenLinkInNewBackgroundTab {
                     objectName: "OpenLinkInNewBackgroundTabContextualAction"
                     enabled: contextModel && contextModel.linkUrl.toString()
                     onTriggered: internal.openUrlInNewTab(contextModel.linkUrl, false,
-                                                          true, tabsModel.indexOf(browserTab) + 1)
+                                                          true, tabsModel.indexOf(browserTab) + 1,
+                                                          tab.uniqueId)
                 }
                 Actions.OpenLinkInNewWindow {
                     objectName: "OpenLinkInNewWindowContextualAction"
@@ -146,7 +148,8 @@
                              (contextModel.mediaType === Oxide.WebView.MediaTypeImage) &&
                              contextModel.srcUrl.toString()
                     onTriggered: internal.openUrlInNewTab(contextModel.srcUrl, true,
-                                                          true, tabsModel.indexOf(browserTab) + 1)
+                                                          true, tabsModel.indexOf(browserTab) + 1,
+                                                          tab.uniqueId)
                 }
                 Actions.CopyImage {
                     objectName: "CopyImageContextualAction"
@@ -170,7 +173,8 @@
                              (contextModel.mediaType === Oxide.WebView.MediaTypeVideo) &&
                              contextModel.srcUrl.toString()
                     onTriggered: internal.openUrlInNewTab(contextModel.srcUrl, true,
-                                                          true, tabsModel.indexOf(browserTab) + 1)
+                                                          true, tabsModel.indexOf(browserTab) + 1,
+                                                          tab.uniqueId)
                 }
                 Actions.SaveVideo {
                     objectName: "SaveVideoContextualAction"
@@ -258,7 +262,8 @@
             contextMenu: browser && browser.wide ? contextMenuWideComponent : contextMenuNarrowComponent
 
             onNewViewRequested: {
-                var newTab = browser.createTab({"request": request})
+                var newTab = browser.createTab({"parentUniqueId": tab.uniqueId,
+                                                "request": request})
                 var setCurrent = (request.disposition == Oxide.NewViewRequest.DispositionNewForegroundTab)
                 internal.addTab(newTab, setCurrent, tabsModel.indexOf(browserTab) + 1)
                 if (setCurrent) tabContainer.forceActiveFocus()

=== modified file 'src/app/webbrowser/tabs-model.cpp'
--- src/app/webbrowser/tabs-model.cpp	2017-02-15 19:41:01 +0000
+++ src/app/webbrowser/tabs-model.cpp	2017-03-03 14:26:48 +0000
@@ -212,6 +212,22 @@
     return m_tabs.indexOf(tab);
 }
 
+/*!
+    Returns the index position of the first occurrence of tab for which
+    property is set to value.
+    Returns -1 if no item matched.
+*/
+int TabsModel::match(const QString& property, const QVariant &value) const
+{
+    for (int i = 0; i < m_tabs.size(); ++i) {
+        QVariant propertyValue = m_tabs[i]->property(property.toLocal8Bit().constData());
+        if (propertyValue == value) {
+            return i;
+        }
+    }
+    return -1;
+}
+
 void TabsModel::move(int from, int to)
 {
     if ((from == to) || !checkValidTabIndex(from) || !checkValidTabIndex(to)) {

=== modified file 'src/app/webbrowser/tabs-model.h'
--- src/app/webbrowser/tabs-model.h	2017-02-15 11:22:05 +0000
+++ src/app/webbrowser/tabs-model.h	2017-03-03 14:26:48 +0000
@@ -61,6 +61,7 @@
     Q_INVOKABLE QObject* remove(int index);
     Q_INVOKABLE QObject* get(int index) const;
     Q_INVOKABLE int indexOf(QObject* tab) const;
+    Q_INVOKABLE int match(const QString& property, const QVariant &value) const;
     Q_INVOKABLE void move(int from, int to);
 
 Q_SIGNALS:

=== modified file 'tests/autopilot/webbrowser_app/tests/__init__.py'
--- tests/autopilot/webbrowser_app/tests/__init__.py	2017-02-15 11:22:05 +0000
+++ tests/autopilot/webbrowser_app/tests/__init__.py	2017-03-03 14:26:48 +0000
@@ -185,6 +185,16 @@
             tabs_view.get_previews()[index].select()
             tabs_view.visible.wait_for(False)
 
+    def close_tab(self, index):
+        if self.main_window.wide:
+            self.main_window.chrome.get_tabs_bar().close_tab(index)
+        else:
+            tabs_view = self.open_tabs_view()
+            tabs_view.get_previews()[index].close()
+            toolbar = self.main_window.get_recent_view_toolbar()
+            toolbar.click_button("doneButton")
+            tabs_view.visible.wait_for(False)
+
     def open_new_window(self):
         windows = self.app.get_windows(incognito=False)
         chrome = self.main_window.chrome

=== modified file 'tests/autopilot/webbrowser_app/tests/http_server.py'
--- tests/autopilot/webbrowser_app/tests/http_server.py	2016-10-13 14:36:24 +0000
+++ tests/autopilot/webbrowser_app/tests/http_server.py	2017-03-03 14:26:48 +0000
@@ -68,6 +68,12 @@
             html += '<a href="/test1"><div style="height: 100%"></div></a>'
             html += '</body></html>'
             self.send_html(html)
+        elif self.path == "/linkcloseself":
+            self.send_response(200)
+            html = '<html><body style="margin: 0">'
+            html += '<a href="/closeself"><div style="height: 100%"></div></a>'
+            html += '</body></html>'
+            self.send_html(html)
         elif self.path.startswith("/wait/"):
             delay = int(self.path[6:])
             self.send_response(200)

=== modified file 'tests/autopilot/webbrowser_app/tests/test_new_tab_view.py'
--- tests/autopilot/webbrowser_app/tests/test_new_tab_view.py	2016-11-03 11:23:18 +0000
+++ tests/autopilot/webbrowser_app/tests/test_new_tab_view.py	2017-03-03 14:26:48 +0000
@@ -36,13 +36,7 @@
 
     def test_new_tab_view_destroyed_when_closing_tab(self):
         new_tab_view = self.open_new_tab(open_tabs_view=True)
-        if self.main_window.wide:
-            self.main_window.chrome.get_tabs_bar().close_tab(1)
-        else:
-            tabs_view = self.open_tabs_view()
-            tabs_view.get_previews()[0].close()
-            toolbar = self.main_window.get_recent_view_toolbar()
-            toolbar.click_button("doneButton")
+        self.close_tab(1 if self.main_window.wide else 0)
         new_tab_view.wait_until_destroyed()
 
     def test_new_tab_view_is_shared_between_tabs(self):
@@ -54,44 +48,21 @@
         self.assertThat(new_tab_view_2.id, Equals(new_tab_view.id))
         # Close the second new tab, and verify that the NewTabView instance
         # is still there
-        if self.main_window.wide:
-            self.main_window.chrome.get_tabs_bar().close_tab(2)
-        else:
-            tabs_view = self.open_tabs_view()
-            tabs_view.get_previews()[0].close()
-            toolbar = self.main_window.get_recent_view_toolbar()
-            toolbar.click_button("doneButton")
-            tabs_view.visible.wait_for(False)
+        self.close_tab(2 if self.main_window.wide else 0)
         self.assertThat(new_tab_view.visible, Equals(True))
         # Close the first new tab, and verify that the NewTabView instance
         # is destroyed
-        if self.main_window.wide:
-            self.main_window.chrome.get_tabs_bar().close_tab(1)
-        else:
-            tabs_view = self.open_tabs_view()
-            tabs_view.get_previews()[0].close()
-            toolbar = self.main_window.get_recent_view_toolbar()
-            toolbar.click_button("doneButton")
+        self.close_tab(1 if self.main_window.wide else 0)
         new_tab_view.wait_until_destroyed()
 
     @testtools.skipIf(model() == "Desktop",
                       "Closing the last open tab on desktop quits the app")
     def test_new_tab_view_not_destroyed_when_closing_last_open_tab(self):
-        if self.main_window.wide:
-            self.main_window.chrome.get_tabs_bar().close_tab(0)
-        else:
-            tabs_view = self.open_tabs_view()
-            tabs_view.get_previews()[0].close()
-            tabs_view.visible.wait_for(False)
+        self.close_tab(0)
         new_tab_view = self.main_window.get_new_tab_view()
         # Verify that the new tab view is not destroyed and then re-created
         # when closing the last open tab if it was a blank one
-        if self.main_window.wide:
-            self.main_window.chrome.get_tabs_bar().close_tab(0)
-        else:
-            tabs_view = self.open_tabs_view()
-            tabs_view.get_previews()[0].close()
-            tabs_view.visible.wait_for(False)
+        self.close_tab(0)
         self.assertThat(new_tab_view.visible, Equals(True))
 
 

=== modified file 'tests/autopilot/webbrowser_app/tests/test_site_previews.py'
--- tests/autopilot/webbrowser_app/tests/test_site_previews.py	2016-11-03 11:23:18 +0000
+++ tests/autopilot/webbrowser_app/tests/test_site_previews.py	2017-03-03 14:26:48 +0000
@@ -107,15 +107,6 @@
         super(TestSitePreviews, self).setUp()
         self.launch_and_wait_for_page_loaded()
 
-    def close_tab(self, index):
-        if self.main_window.wide:
-            self.main_window.chrome.get_tabs_bar().close_tab(index)
-        else:
-            tabs_view = self.open_tabs_view()
-            tabs_view.get_previews()[index].close()
-            toolbar = self.main_window.get_recent_view_toolbar()
-            toolbar.click_button("doneButton")
-
     def get_captures(self):
         if not path.exists(self.captures_dir):
             return []

=== modified file 'tests/autopilot/webbrowser_app/tests/test_tabs.py'
--- tests/autopilot/webbrowser_app/tests/test_tabs.py	2017-02-15 11:22:05 +0000
+++ tests/autopilot/webbrowser_app/tests/test_tabs.py	2017-03-03 14:26:48 +0000
@@ -280,14 +280,7 @@
         url = self.base_url + "/closeself"
         self.main_window.go_to_url(url)
         self.main_window.wait_until_page_loaded(url)
-        if self.main_window.wide:
-            self.main_window.chrome.get_tabs_bar().close_tab(0)
-        else:
-            tabs_view = self.open_tabs_view()
-            tabs_view.get_previews()[1].close()
-            toolbar = self.main_window.get_recent_view_toolbar()
-            toolbar.click_button("doneButton")
-            tabs_view.visible.wait_for(False)
+        self.close_tab(0 if self.main_window.wide else 1)
         webview = self.main_window.get_current_webview()
         wide = self.main_window.wide
         self.pointing_device.click_object(webview)
@@ -339,3 +332,67 @@
         self.main_window.press_key('Ctrl+Shift+t')
         self.assert_number_webviews_eventually(3)
         self.check_current_tab(url2)
+
+
+class TestCloseTabsSelectParent(StartOpenRemotePageTestCaseBase,
+                                TestTabsMixin):
+
+    def setup_tabs(self, url):
+        # create 3 tabs, the second one being a child of the first one
+
+        # first tab with a link
+        self.main_window.go_to_url(url)
+        self.main_window.wait_until_page_loaded(url)
+
+        # second tab by ctrl-clicking the link
+        webview = self.main_window.get_current_webview()
+        self.keyboard.press('Ctrl')
+        self.pointing_device.click_object(webview)
+        self.keyboard.release('Ctrl')
+        self.assert_number_webviews_eventually(2)
+
+        # third empty tab
+        self.open_new_tab(open_tabs_view=not self.main_window.wide)
+        self.assert_number_webviews_eventually(3)
+
+    @testtools.skipIf(model() != "Desktop", "on desktop only")
+    def test_click_close(self):
+        if not self.main_window.wide:
+            self.skipTest("Only on wide form factors")
+        self.setup_tabs(self.base_url + "/link")
+
+        self.switch_to_tab(1 if self.main_window.wide else 2)
+        self.check_current_tab(self.base_url + "/test1")
+        self.close_tab(1 if self.main_window.wide else 0)
+
+        # verify that the parent tab that created the 'test1' tab is reselected
+        self.check_current_tab(self.base_url + "/link")
+
+    @testtools.skipIf(model() != "Desktop", "on desktop only")
+    def test_close_shortcut(self):
+        if not self.main_window.wide:
+            self.skipTest("Only on wide form factors")
+        self.setup_tabs(self.base_url + "/link")
+
+        self.switch_to_tab(1 if self.main_window.wide else 2)
+        self.check_current_tab(self.base_url + "/test1")
+        self.main_window.press_key('Ctrl+w')
+
+        # verify that the parent tab that created the 'test1' tab is reselected
+        self.check_current_tab(self.base_url + "/link")
+
+    @testtools.skipIf(model() != "Desktop", "on desktop only")
+    def test_webview_requests_close(self):
+        if not self.main_window.wide:
+            self.skipTest("Only on wide form factors")
+        self.setup_tabs(self.base_url + "/linkcloseself")
+
+        self.switch_to_tab(1)
+        self.check_current_tab(self.base_url + "/closeself")
+        webview = self.main_window.get_current_webview()
+        self.pointing_device.click_object(webview)
+        webview.wait_until_destroyed()
+
+        # verify that the parent tab that created the 'closeself' tab is
+        # not reselected
+        self.check_current_tab("")

=== modified file 'tests/unittests/tabs-model/tst_TabsModelTests.cpp'
--- tests/unittests/tabs-model/tst_TabsModelTests.cpp	2017-02-15 19:41:01 +0000
+++ tests/unittests/tabs-model/tst_TabsModelTests.cpp	2017-03-03 14:26:48 +0000
@@ -499,6 +499,20 @@
         delete nonAddedTab;
     }
 
+    void shouldReturnTabWithPropertyValueIndex()
+    {
+        QQuickItem* tab1 = createTab();
+        tab1->setProperty("testProperty", 38);
+        model->add(tab1);
+        QQuickItem* tab2 = createTab();
+        tab2->setProperty("testProperty", 42);
+        model->add(tab2);
+        QCOMPARE(model->match("testProperty", 38), 0);
+        QCOMPARE(model->match("testProperty", 42), 1);
+        QCOMPARE(model->match("invalidProperty", 42), -1);
+        QCOMPARE(model->match("testProperty", -256), -1);
+    }
+
 private:
     void moveTabs(int from, int to, bool moved, bool indexChanged, int newIndex)
     {