Back/forward mouse buttons don't work since updating to Qt 5.8

[pkillnine]

qutebrowser v0.9.1
Git commit: v0.8.0-2102-gde15151d0-dirty (2017-01-25 19:31:30 +0000)
Backend: webengine

[lahwaacz]

This is similar to a problem with (not) entering an insert mode when one clicks an input field, see https://bbs.archlinux.org/viewtopic.php?pid=1687352#p1687352

A temporary workaround is to comment out these lines, but I'm pretty sure that will break something eventually...

[The-Compiler]

Indeed. the log confirms that:

16:43:41 DEBUG    mouse      mouse:eventFilter:213 Ignoring QMouseEvent to <PyQt5.QtWidgets.QWidget object at 0x7f4ef55ea3a8> which is not an instance of <class 'PyQt5.QtWidgets.QOpenGLWidget'>

The reason why it was added is described in fe11e25.

[lahwaacz]

So that's what it breaks. Perhaps it would be enough to correct the WIDGET_CLASS (which might be the actual change in 5.8)?

[The-Compiler]

Either that, or find some other way to blacklist or deal with the QMenu events.

[lahwaacz]

If the only problem is with QMenu, then something like this might work:

--- a/qutebrowser/browser/mouse.py
+++ b/qutebrowser/browser/mouse.py
@@ -26,6 +26,7 @@ from qutebrowser.keyinput import modeman
 
 
 from PyQt5.QtCore import QObject, QEvent, Qt, QTimer
+from PyQt5.QtWidgets import QMenu
 
 
 class ChildEventFilter(QObject):
@@ -207,7 +208,7 @@ class MouseEventFilter(QObject):
         evtype = event.type()
         if evtype not in self._handlers:
             return False
-        if not isinstance(obj, self._widget_class):
+        if self._widget_class is not QMenu and isinstance(obj, QMenu):
             log.mouse.debug("Ignoring {} to {} which is not an instance of "
                             "{}".format(event.__class__.__name__, obj,
                                         self._widget_class))

[maxemillianian]

I also have this issue with back//forward keyboard buttons on my Thinkpad, also using webengine. Might @lahwaacz fix work for my computer? And if so, how do I implement it?

Thanks!

[The-Compiler]

@maxemillianian Those aren't mouse buttons - you can probably just do :bind <XF86Back> back and the same with forward.

[maxemillianian]

I have bound and to 'back' and 'forward', but neither is working. Do I need to unbind the previous keybindings for 'back' and 'forward'?

[maxemillianian]

Although I can see other commds which are 'double bound' so I can't see that making a difference...

[lahwaacz]

Use xev to see which keycode these keys have (if any).

[maxemillianian]

They're called XF86Back and XF86Forward, I bound as well as XF86Back but neither is working...

Thanks!

[lahwaacz]

Can you start qutebrowser with --debug --logfilter keyboard and show the log when you press the key?

I suspect it will be the same as in #2166 (comment), in which case you need to bind <back> and <forward>, which are the Qt names for the keys. Note that they are bound by default since v0.9.0, are you running an older version?

[maxemillianian]

This is the log output for the back and forward keyboard keys:

16:15:12 DEBUG keyboard basekeyparser:_debug_log:112 No special binding found for back.
16:15:12 DEBUG keyboard basekeyparser:_debug_log:112 Got key: 0x1000061 / text: ''
16:15:12 DEBUG keyboard basekeyparser:_debug_log:112 Ignoring, no text char
16:15:22 DEBUG keyboard basekeyparser:_debug_log:112 No special binding found for forward.
16:15:22 DEBUG keyboard basekeyparser:_debug_log:112 Got key: 0x1000062 / text: ''
16:15:22 DEBUG keyboard basekeyparser:_debug_log:112 Ignoring, no text char

I'm running qutebrowser 0.9.1-3

[maxemillianian]

Ah, I bound < back > back and now the keys work!

Should I unbind the bindings I added earlier?

Thanks!

[The-Compiler]

Oh, my bad, I guess I was thinking about herbstluftwm bindings there 😆

You can :unbind <XF86Back>, but they don't hurt either FWIW.

[maxemillianian]

NP, Thanks a lot both of you!

[lahwaacz]

I got a fix in lahwaacz@de0151a, though there are some issues that made me not submit a pull request yet:

• This will break stuff with older Qt versions. Maybe we could get the correct WIDGET_CLASS at runtime somehow?
• QWidget, which is now used for the webview in webengine, is a base of QMenu, so I had to use type instead of isinstance. This might break stuff too, e.g. in webkit.

[The-Compiler]

@lahwaacz is there a reason you went for a whitelist approach there again instead of a blacklist one? If #2262 (comment) works, that might be the easier approach. I won't have the time to dig into this until Friday or so though.

[lahwaacz]

Well, in webkit all events are delivered only to the WebView object and things like scrolling with mouse wheel does not work when e.g. a context menu is active - webkit itself takes care of it. In webengine however, events are delivered to the child widgets themseleves (at least since Qt 5.8) - one of them is QMenu, but from what I know there might be others in the future (or already). As I examined the code, I realized that the intention is to have the same behaviour as in webkit, so this seemed like the more futureproof solution.
So far it's also less invasive since I'd reduce the condition in #2262 (comment) to if isinstance(obj, QMenu): and remove the WIDGET_CLASS altogether. That would work since QMenu is never used in WIDGET_CLASS, but sometime more classes might need to be blacklisted. On second thought, this might be easier than solving the above problems... I'll think about it more tomorrow.

[The-Compiler]

Either way it's probably a good idea to test this with things like select-dropdowns on pages too.

[lahwaacz]

Events for selects are delivered to some QWidget in webengine, which might be either the select or webview - I guess there is no way to know. In webkit however, events for selects are not passed to the eventFilter at all. So when some select is active, zooming with Ctrl+MouseWheel actually works in webengine (unlike webkit) and I'd imagine that for gestures or other pointing devices much more things will be broken...

[The-Compiler]

That shouldn't be a problem - I think QtWebEngine (and Chromium?) just grabs the mouse somehow.

As for the filtering issue: An AbstractTab has an _event_target method, which returns the underlying widget (the QWidget::focusProxy for QtWebEngine). Couldn't we just get that in the mouse handler and check against that with is?

[The-Compiler]

I mean this:

diff --git a/qutebrowser/browser/browsertab.py b/qutebrowser/browser/browsertab.py
index 1480efbf0..e868f3261 100644
--- a/qutebrowser/browser/browsertab.py
+++ b/qutebrowser/browser/browsertab.py
@@ -486,10 +486,6 @@ class AbstractTab(QWidget):
 
     We use this to unify QWebView and QWebEngineView.
 
-    Class attributes:
-        WIDGET_CLASS: The class of the main widget recieving events.
-                      Needs to be overridden by subclasses.
-
     Attributes:
         history: The AbstractHistory for the current tab.
         registry: The ObjectRegistry associated with this tab.
@@ -523,8 +519,6 @@ class AbstractTab(QWidget):
     contents_size_changed = pyqtSignal(QSizeF)
     add_history_item = pyqtSignal(QUrl, QUrl, str)  # url, requested url, title
 
-    WIDGET_CLASS = None
-
     def __init__(self, win_id, mode_manager, parent=None):
         self.win_id = win_id
         self.tab_id = next(tab_id_gen)
@@ -553,7 +547,7 @@ class AbstractTab(QWidget):
         self._mode_manager = mode_manager
         self._load_status = usertypes.LoadStatus.none
         self._mouse_event_filter = mouse.MouseEventFilter(
-            self, widget_class=self.WIDGET_CLASS, parent=self)
+            self, parent=self)
         self.backend = None
 
         # FIXME:qtwebengine  Should this be public api via self.hints?
@@ -588,7 +582,7 @@ class AbstractTab(QWidget):
         self._load_status = val
         self.load_status_changed.emit(val.name)
 
-    def _event_target(self):
+    def event_target(self):
         """Return the widget events should be sent to."""
         raise NotImplementedError
 
@@ -603,7 +597,7 @@ class AbstractTab(QWidget):
         if getattr(evt, 'posted', False):
             raise AssertionError("Can't re-use an event which was already "
                                  "posted!")
-        recipient = self._event_target()
+        recipient = self.event_target()
         evt.posted = True
         QApplication.postEvent(recipient, evt)
 
diff --git a/qutebrowser/browser/mouse.py b/qutebrowser/browser/mouse.py
index a1b56b96a..1346a7693 100644
--- a/qutebrowser/browser/mouse.py
+++ b/qutebrowser/browser/mouse.py
@@ -64,8 +64,6 @@ class MouseEventFilter(QObject):
     """Handle mouse events on a tab.
 
     Attributes:
-        _widget_class: The class of the main widget getting the events.
-                       All other events are ignored.
         _tab: The browsertab object this filter is installed on.
         _handlers: A dict of handler functions for the handled events.
         _ignore_wheel_event: Whether to ignore the next wheelEvent.
@@ -73,9 +71,8 @@ class MouseEventFilter(QObject):
                                       done when the mouse is released.
     """
 
-    def __init__(self, tab, *, widget_class, parent=None):
+    def __init__(self, tab, *, parent=None):
         super().__init__(parent)
-        self._widget_class = widget_class
         self._tab = tab
         self._handlers = {
             QEvent.MouseButtonPress: self._handle_mouse_press,
@@ -207,9 +204,7 @@ class MouseEventFilter(QObject):
         evtype = event.type()
         if evtype not in self._handlers:
             return False
-        if not isinstance(obj, self._widget_class):
-            log.mouse.debug("Ignoring {} to {} which is not an instance of "
-                            "{}".format(event.__class__.__name__, obj,
-                                        self._widget_class))
+        if obj is not self._tab.event_target():
+            log.mouse.debug("Ignoring {} to {}".format(event.__class__.__name__, obj))
             return False
         return self._handlers[evtype](event)
diff --git a/qutebrowser/browser/webengine/webenginetab.py b/qutebrowser/browser/webengine/webenginetab.py
index 4c8b0602d..a555a2d15 100644
--- a/qutebrowser/browser/webengine/webenginetab.py
+++ b/qutebrowser/browser/webengine/webenginetab.py
@@ -452,8 +452,6 @@ class WebEngineTab(browsertab.AbstractTab):
 
     """A QtWebEngine tab in the browser."""
 
-    WIDGET_CLASS = QOpenGLWidget
-
     def __init__(self, win_id, mode_manager, parent=None):
         super().__init__(win_id=win_id, mode_manager=mode_manager,
                          parent=parent)
@@ -654,5 +652,5 @@ class WebEngineTab(browsertab.AbstractTab):
         except AttributeError:
             log.stub('contentsSizeChanged, on Qt < 5.7')
 
-    def _event_target(self):
+    def event_target(self):
         return self._widget.focusProxy()
diff --git a/qutebrowser/browser/webkit/webkittab.py b/qutebrowser/browser/webkit/webkittab.py
index 12c643f46..82a25d634 100644
--- a/qutebrowser/browser/webkit/webkittab.py
+++ b/qutebrowser/browser/webkit/webkittab.py
@@ -590,8 +590,6 @@ class WebKitTab(browsertab.AbstractTab):
 
     """A QtWebKit tab in the browser."""
 
-    WIDGET_CLASS = webview.WebView
-
     def __init__(self, win_id, mode_manager, parent=None):
         super().__init__(win_id=win_id, mode_manager=mode_manager,
                          parent=parent)
@@ -717,5 +715,5 @@ class WebKitTab(browsertab.AbstractTab):
         frame.contentsSizeChanged.connect(self._on_contents_size_changed)
         frame.initialLayoutCompleted.connect(self._on_history_trigger)
 
-    def _event_target(self):
+    def event_target(self):
         return self._widget

@lahwaacz can you give this a quick review and a bit of testing too if you have a minute? Then I'll push it to master. It seems to make things better, but I'm a bit in a hurry, so I'd appreciate a second pair of eyes.

[lahwaacz]

I think you cracked it 👍 As far as I can tell, that works perfectly.

[The-Compiler]

Pushed! Now only some hint tests are failing with 5.8 I think, and I don't know why yet. Took a quick look with --no-xvfb, and it seems links are sometimes just not clicked. Also the zoom-level seems very low (i.e. stuff is tiny), but I'm not sure if that was the case before already.

Will look at that later this week, now I should really continue learning for my exam tomorrow 😆

[lahwaacz]

I've also recently encountered couple of links on which hinting did not work, even before this fix. It could have something to do with 5.8 though.