Handle clicks via createWindow with QtWebEngine

Before, we used the same logic for QtWebKit and QtWebEngine, where we
simply set an attribute on the tab on a mousePressEvent and then handled
opening links in acceptNavigationRequest.

However, this caused random links to pop up in new tabs (probably to
things being much more async?) on QtWebEngine, so we now handle those
cases in createWindow and only use override_target from the tab there.

Fixes #2102.

qutebrowser/browser/browsertab.py

@@ -30,7 +30,7 @@ from qutebrowser.config import config
 from qutebrowser.utils import (utils, objreg, usertypes, message, log, qtutils,
                                urlutils)
 from qutebrowser.misc import miscwidgets
-from qutebrowser.browser import mouse, hints
+from qutebrowser.browser import mouse, hints, shared
 
 
 tab_id_gen = itertools.count(0)
@@ -84,7 +84,6 @@ class TabData:
                    load.
         inspector: The QWebInspector used for this webview.
         viewing_source: Set if we're currently showing a source view.
-        open_target: How the next clicked link should be opened.
         override_target: Override for open_target for fake clicks (like hints).
     """
 
@@ -92,15 +91,8 @@ class TabData:
         self.keep_icon = False
         self.viewing_source = False
         self.inspector = None
-        self.open_target = usertypes.ClickTarget.normal
         self.override_target = None
 
-    def combined_target(self):
-        if self.override_target is not None:
-            return self.override_target
-        else:
-            return self.open_target
-
 
 class AbstractPrinting:
 
@@ -610,44 +602,6 @@ class AbstractTab(QWidget):
         evt.posted = True
         QApplication.postEvent(recipient, evt)
 
-    @pyqtSlot(QUrl)
-    def _on_link_clicked(self, url):
-        log.webview.debug("link clicked: url {}, override target {}, "
-                          "open_target {}".format(
-                              url.toDisplayString(),
-                              self.data.override_target,
-                              self.data.open_target))
-
-        if not url.isValid():
-            msg = urlutils.get_errstring(url, "Invalid link clicked")
-            message.error(msg)
-            self.data.open_target = usertypes.ClickTarget.normal
-            return False
-
-        target = self.data.combined_target()
-
-        if target == usertypes.ClickTarget.normal:
-            return
-        elif target == usertypes.ClickTarget.tab:
-            win_id = self.win_id
-            bg_tab = False
-        elif target == usertypes.ClickTarget.tab_bg:
-            win_id = self.win_id
-            bg_tab = True
-        elif target == usertypes.ClickTarget.window:
-            from qutebrowser.mainwindow import mainwindow
-            window = mainwindow.MainWindow()
-            window.show()
-            win_id = window.win_id
-            bg_tab = False
-        else:
-            raise ValueError("Invalid ClickTarget {}".format(target))
-
-        tabbed_browser = objreg.get('tabbed-browser', scope='window',
-                                    window=win_id)
-        tabbed_browser.tabopen(url, background=bg_tab)
-        self.data.open_target = usertypes.ClickTarget.normal
-
     @pyqtSlot(QUrl)
     def _on_url_changed(self, url):
         """Update title when URL has changed and no title is available."""

qutebrowser/browser/mouse.py

@@ -96,7 +96,6 @@ class MouseEventFilter(QObject):
             return True
 
         self._ignore_wheel_event = True
-        self._mousepress_opentarget(e)
         self._tab.elements.find_at_pos(e.pos(), self._mousepress_insertmode_cb)
 
         return False
@@ -197,27 +196,6 @@ class MouseEventFilter(QObject):
             else:
                 message.error("At end of history.")
 
-    def _mousepress_opentarget(self, e):
-        """Set the open target when something was clicked.
-
-        Args:
-            e: The QMouseEvent.
-        """
-        if e.button() == Qt.MidButton or e.modifiers() & Qt.ControlModifier:
-            background_tabs = config.get('tabs', 'background-tabs')
-            if e.modifiers() & Qt.ShiftModifier:
-                background_tabs = not background_tabs
-            if background_tabs:
-                target = usertypes.ClickTarget.tab_bg
-            else:
-                target = usertypes.ClickTarget.tab
-            self._tab.data.open_target = target
-            log.mouse.debug("Ctrl/Middle click, setting target: {}".format(
-                target))
-        else:
-            self._tab.data.open_target = usertypes.ClickTarget.normal
-            log.mouse.debug("Normal click, setting normal target")
-
     def eventFilter(self, obj, event):
         """Filter events going to a QWeb(Engine)View."""
         evtype = event.type()

qutebrowser/browser/shared.py

@@ -24,7 +24,7 @@ import html
 import jinja2
 
 from qutebrowser.config import config
-from qutebrowser.utils import usertypes, message, log
+from qutebrowser.utils import usertypes, message, log, objreg
 
 
 class CallSuper(Exception):
@@ -195,3 +195,30 @@ def feature_permission(url, option, msg, yes_action, no_action, abort_on):
     else:
         no_action()
         return None
+
+
+def get_tab(win_id, target):
+    """Get a tab widget for the given usertypes.ClickTarget.
+
+    Args:
+        win_id: The window ID to open new tabs in
+        target: An usertypes.ClickTarget
+    """
+    if target == usertypes.ClickTarget.tab:
+        win_id = win_id
+        bg_tab = False
+    elif target == usertypes.ClickTarget.tab_bg:
+        win_id = win_id
+        bg_tab = True
+    elif target == usertypes.ClickTarget.window:
+        from qutebrowser.mainwindow import mainwindow
+        window = mainwindow.MainWindow()
+        window.show()
+        win_id = window.win_id
+        bg_tab = False
+    else:
+        raise ValueError("Invalid ClickTarget {}".format(target))
+
+    tabbed_browser = objreg.get('tabbed-browser', scope='window',
+                                window=win_id)
+    return tabbed_browser.tabopen(url=None, background=bg_tab)

qutebrowser/browser/webengine/webenginetab.py

@@ -616,7 +616,6 @@ class WebEngineTab(browsertab.AbstractTab):
         view.urlChanged.connect(self._on_url_changed)
         page.loadFinished.connect(self._on_load_finished)
         page.certificate_error.connect(self._on_ssl_errors)
-        page.link_clicked.connect(self._on_link_clicked)
         page.authenticationRequired.connect(self._on_authentication_required)
         try:
             view.iconChanged.connect(self.icon_changed)

qutebrowser/browser/webengine/webview.py

@@ -30,7 +30,8 @@ from PyQt5.QtWebEngineWidgets import QWebEngineView, QWebEnginePage
 from qutebrowser.browser import shared
 from qutebrowser.browser.webengine import certificateerror
 from qutebrowser.config import config
-from qutebrowser.utils import log, debug, usertypes, objreg, qtutils, jinja
+from qutebrowser.utils import (log, debug, usertypes, objreg, qtutils, jinja,
+                               urlutils, message)
 
 
 class WebEngineView(QWebEngineView):
@@ -40,7 +41,8 @@ class WebEngineView(QWebEngineView):
     def __init__(self, tabdata, win_id, parent=None):
         super().__init__(parent)
         self._win_id = win_id
-        self.setPage(WebEnginePage(tabdata, parent=self))
+        self._tabdata = tabdata
+        self.setPage(WebEnginePage(parent=self))
 
     def shutdown(self):
         self.page().shutdown()
@@ -71,24 +73,40 @@ class WebEngineView(QWebEngineView):
             The new QWebEngineView object.
         """
         debug_type = debug.qenum_key(QWebEnginePage, wintype)
-        log.webview.debug("createWindow with type {}".format(debug_type))
+        background_tabs = config.get('tabs', 'background-tabs')
+        override_target = self._tabdata.override_target
 
-        background = False
+        log.webview.debug("createWindow with type {}, background_tabs "
-        if wintype in [QWebEnginePage.WebBrowserWindow,
+                          "{}, override_target {}".format(
-                       QWebEnginePage.WebDialog]:
+                              debug_type, background_tabs, override_target))
+
+        if override_target is not None:
+            target = override_target
+        elif wintype == QWebEnginePage.WebBrowserWindow:
+            log.webview.debug("createWindow with WebBrowserWindow - when does "
+                              "this happen?!")
+            target = usertypes.ClickTarget.window
+        elif wintype == QWebEnginePage.WebDialog:
             log.webview.warning("{} requested, but we don't support "
                                 "that!".format(debug_type))
+            target = usertypes.ClickTarget.tab
         elif wintype == QWebEnginePage.WebBrowserTab:
-            pass
+            # Middle-click / Ctrl-Click with Shift
+            if background_tabs:
+                target = usertypes.ClickTarget.tab
+            else:
+                target = usertypes.ClickTarget.tab_bg
         elif (hasattr(QWebEnginePage, 'WebBrowserBackgroundTab') and
               wintype == QWebEnginePage.WebBrowserBackgroundTab):
-            background = True
+            # Middle-click / Ctrl-Click
+            if background_tabs:
+                target = usertypes.ClickTarget.tab_bg
+            else:
+                target = usertypes.ClickTarget.tab
         else:
             raise ValueError("Invalid wintype {}".format(debug_type))
 
-        tabbed_browser = objreg.get('tabbed-browser', scope='window',
+        tab = shared.get_tab(self._win_id, target)
-                                    window=self._win_id)
-        tab = tabbed_browser.tabopen(background=background)
 
         # WORKAROUND for https://bugreports.qt.io/browse/QTBUG-54419
         vercheck = qtutils.version_check
@@ -110,17 +128,14 @@ class WebEnginePage(QWebEnginePage):
 
     Signals:
         certificate_error: Emitted on certificate errors.
-        link_clicked: Emitted when a link was clicked on a page.
         shutting_down: Emitted when the page is shutting down.
     """
 
     certificate_error = pyqtSignal()
-    link_clicked = pyqtSignal(QUrl)
     shutting_down = pyqtSignal()
 
-    def __init__(self, tabdata, parent=None):
+    def __init__(self, parent=None):
         super().__init__(parent)
-        self._tabdata = tabdata
         self._is_shutting_down = False
         self.featurePermissionRequested.connect(
             self._on_feature_permission_requested)
@@ -277,24 +292,17 @@ class WebEnginePage(QWebEnginePage):
                                 is_main_frame: bool):
         """Override acceptNavigationRequest to handle clicked links.
 
-        Setting linkDelegationPolicy to DelegateAllLinks and using a slot bound
+        This only show an error on invalid links - everything else is handled
-        to linkClicked won't work correctly, because when in a frameset, we
+        in createWindow.
-        have no idea in which frame the link should be opened.
-
-        Checks if it should open it in a tab (middle-click or control) or not,
-        and then conditionally opens the URL. Opening it in a new tab/window
-        is handled in the slot connected to link_clicked.
         """
-        target = self._tabdata.combined_target()
+        log.webview.debug("navigation request: url {}, type {}, is_main_frame "
-        log.webview.debug("navigation request: url {}, type {}, "
+                          "{}".format(url.toDisplayString(),
-                          "target {}, is_main_frame {}".format(
+                                      debug.qenum_key(QWebEnginePage, typ),
-                              url.toDisplayString(),
+                                      is_main_frame))
-                              debug.qenum_key(QWebEnginePage, typ),
+        if (typ == QWebEnginePage.NavigationTypeLinkClicked and
-                              target, is_main_frame))
+                not url.isValid()):
-
+            msg = urlutils.get_errstring(url, "Invalid link clicked")
-        if typ != QWebEnginePage.NavigationTypeLinkClicked:
+            message.error(msg)
-            return True
+            self.page().open_target = usertypes.ClickTarget.normal
-
+            return False
-        self.link_clicked.emit(url)
+        return True
-
-        return url.isValid() and target == usertypes.ClickTarget.normal

qutebrowser/browser/webkit/webkittab.py

@@ -329,7 +329,7 @@ class WebKitCaret(browsertab.AbstractCaret):
         if QWebSettings.globalSettings().testAttribute(
                 QWebSettings.JavascriptEnabled):
             if tab:
-                self._tab.data.open_target = usertypes.ClickTarget.tab
+                self._tab.data.override_target = usertypes.ClickTarget.tab
             self._tab.run_js_async(
                 'window.getSelection().anchorNode.parentNode.click()')
         else:
@@ -714,7 +714,6 @@ class WebKitTab(browsertab.AbstractTab):
         page.frameCreated.connect(self._on_frame_created)
         frame.contentsSizeChanged.connect(self._on_contents_size_changed)
         frame.initialLayoutCompleted.connect(self._on_history_trigger)
-        page.link_clicked.connect(self._on_link_clicked)
 
     def _event_target(self):
         return self._widget

qutebrowser/browser/webkit/webpage.py

@@ -34,7 +34,7 @@ from qutebrowser.browser import pdfjs, shared
 from qutebrowser.browser.webkit import http
 from qutebrowser.browser.webkit.network import networkmanager
 from qutebrowser.utils import (message, usertypes, log, jinja, qtutils, utils,
-                               objreg, debug)
+                               objreg, debug, urlutils)
 
 
 class BrowserPage(QWebPage):
@@ -54,12 +54,10 @@ class BrowserPage(QWebPage):
         shutting_down: Emitted when the page is currently shutting down.
         reloading: Emitted before a web page reloads.
                    arg: The URL which gets reloaded.
-        link_clicked: Emitted when a link was clicked on a page.
     """
 
     shutting_down = pyqtSignal()
     reloading = pyqtSignal(QUrl)
-    link_clicked = pyqtSignal(QUrl)
 
     def __init__(self, win_id, tab_id, tabdata, parent=None):
         super().__init__(parent)
@@ -72,6 +70,7 @@ class BrowserPage(QWebPage):
         }
         self._ignore_load_started = False
         self.error_occurred = False
+        self.open_target = usertypes.ClickTarget.normal
         self._networkmanager = networkmanager.NetworkManager(
             win_id, tab_id, self)
         self.setNetworkAccessManager(self._networkmanager)
@@ -462,16 +461,20 @@ class BrowserPage(QWebPage):
         have no idea in which frame the link should be opened.
 
         Checks if it should open it in a tab (middle-click or control) or not,
-        and then conditionally opens the URL. Opening it in a new tab/window
+        and then conditionally opens the URL here or in another tab/window.
-        is handled in the slot connected to link_clicked.
         """
         url = request.url()
-        target = self._tabdata.combined_target()
         log.webview.debug("navigation request: url {}, type {}, "
-                          "target {}".format(
+                          "target {} override {}".format(
                               url.toDisplayString(),
                               debug.qenum_key(QWebPage, typ),
-                              target))
+                              self.open_target,
+                              self._tabdata.override_target))
+
+        if self._tabdata.override_target is not None:
+            target = self._tabdata.override_target
+        else:
+            target = self.open_target
 
         if typ == QWebPage.NavigationTypeReload:
             self.reloading.emit(url)
@@ -479,6 +482,16 @@ class BrowserPage(QWebPage):
         elif typ != QWebPage.NavigationTypeLinkClicked:
             return True
 
-        self.link_clicked.emit(url)
+        if not url.isValid():
+            msg = urlutils.get_errstring(url, "Invalid link clicked")
+            message.error(msg)
+            self.open_target = usertypes.ClickTarget.normal
+            return False
 
-        return url.isValid() and target == usertypes.ClickTarget.normal
+        if target == usertypes.ClickTarget.normal:
+            return True
+
+        tab = shared.get_tab(self._win_id, target)
+        tab.openurl(url)
+        self.open_target = usertypes.ClickTarget.normal
+        return False

qutebrowser/browser/webkit/webview.py

@@ -64,6 +64,7 @@ class WebView(QWebView):
         # FIXME:qtwebengine this is only used to set the zoom factor from
         # the QWebPage - we should get rid of it somehow (signals?)
         self.tab = tab
+        self._tabdata = tab.data
         self.win_id = win_id
         self.scroll_pos = (-1, -1)
         self._old_scroll_pos = (-1, -1)
@@ -280,3 +281,24 @@ class WebView(QWebView):
             pass
 
         super().hideEvent(e)
+
+    def mousePressEvent(self, e):
+        """Set the tabdata ClickTarget on a mousepress.
+
+        This is implemented here as we don't need it for QtWebEngine.
+        """
+        if e.button() == Qt.MidButton or e.modifiers() & Qt.ControlModifier:
+            background_tabs = config.get('tabs', 'background-tabs')
+            if e.modifiers() & Qt.ShiftModifier:
+                background_tabs = not background_tabs
+            if background_tabs:
+                target = usertypes.ClickTarget.tab_bg
+            else:
+                target = usertypes.ClickTarget.tab
+            self.page().open_target = target
+            log.mouse.debug("Ctrl/Middle click, setting target: {}".format(
+                target))
+        else:
+            self.page().open_target = usertypes.ClickTarget.normal
+            log.mouse.debug("Normal click, setting normal target")
+        super().mousePressEvent(e)