Skip to content

Commit

Permalink
Explicitly handle focus before hiding UI elements
Browse files Browse the repository at this point in the history
Almost 7 years ago, it was observed that hiding the status bar causes some
websites being scrolled to the top: #2236.

Back then, it never really was clear why this happens. However, with the v3.0.0
release, we had a regression causing the same thing to happen when leaving
prompt mode: #7885.

Thanks to "git bisect", the culprit was found to be 8e152aa, "Don't give
keyboard focus to tab bar", which was a fix for #7820. However, it still wasn't
clear why this phenomenon happens.

What made things clearer to me was a combination of debugging and an old comment
by pevu: #2236 (comment)

    Chromium-browser has the same issue. When you open lipsum.com, scroll down,
    then focus the location bar (url box), then press Tab, it will jump to the
    top of the page and focus the first link. This doesn't happen when you
    switch focus using the mouse.

    It seems to be an issue of how the view containing the website is focused
    when all qutebrowser ui elements disappear.

And indeed, tabbing into the web contents from the UI elements via the tab key
in Chromium causes the website to start at the top, presumably as an
accessibility feature?

Essentially, this is also what happens in qutebrowser when an UI element is
hidden while it still has focus: In QWidget::hide() (or, rather,
QWidgetPrivate::hide_helper()), Qt moves the focus to the next widget by
calling focusPrevNextChild(true):
https://github.com/qt/qtbase/blob/v6.6.1/src/widgets/kernel/qwidget.cpp#L8259-L8271

And apparently, focusPrevNextChild() basically does the same thing as pressing
the tab key, to the point that there is some code in Qt Declarative actually
making tab keypresses out of it (which I'm still not sure is related, or maybe
just the cause of #4579):
https://github.com/qt/qtdeclarative/blob/v6.6.1/src/quickwidgets/qquickwidget.cpp#L1415-L1429

jome debugging confirms that this is exactly what happening:

1) We hide the status bar (or prompt) which has keyboard focus
2) Qt focuses the web view, which triggers the Chromium feature (?) scrolling it
   to the very top.
3) Only then, in TabbedBrowser.on_mod_left(), we noticed that the command or
   prompt mode was left, and reassign focus to the web view properly.

In step 2), before this change, Qt happened to focus the tab bar (before we set
the focus manually to the web contents), and thus this didn't happen.
Not sure why it didn't focus the tab bar when we hid the status bar (maybe
because how our widget hierarchy works with TabbedBrowser?).

Python stacktrace of hiding prompt:

    Traceback (most recent call first):
    <built-in method hide of DownloadFilenamePrompt object at remote 0x7fffb8bc65f0>
    File ".../qutebrowser/mainwindow/prompt.py", line 204, in _on_mode_left
        self.show_prompts.emit(None)
    File ".../qutebrowser/keyinput/modeman.py", line 434, in leave
        self.left.emit(mode, self.mode, self._win_id)
    File ".../qutebrowser/keyinput/modeman.py", line 445, in mode_leave
        self.leave(self.mode, 'leave current')

C++ stacktrace, with the focus change presumably being passed of to Chromium
here: https://github.com/qt/qtwebengine/blob/dev/src/core/render_widget_host_view_qt_delegate_client.cpp#L714

    #0  QtWebEngineCore::RenderWidgetHostViewQtDelegateClient::handleFocusEvent(QFocusEvent*) () at /usr/src/debug/qt6-webengine/qtwebengine-everywhere-src-6.6.0/src/core/render_widget_host_view_qt_delegate_client.cpp:708
    #1  QtWebEngineCore::RenderWidgetHostViewQtDelegateClient::handleFocusEvent(QFocusEvent*) () at /usr/src/debug/qt6-webengine/qtwebengine-everywhere-src-6.6.0/src/core/render_widget_host_view_qt_delegate_client.cpp:705
    #2  0x00007fffe5fea70c in QtWebEngineCore::RenderWidgetHostViewQtDelegateClient::forwardEvent(QEvent*) () at /usr/src/debug/qt6-webengine/qtwebengine-everywhere-src-6.6.0/src/core/render_widget_host_view_qt_delegate_client.cpp:300
    #3  0x00007fffe4dd5c79 in QQuickItem::event(QEvent*) (this=0x555556b6cd20, ev=0x7fffffffa320) at /usr/src/debug/qt6-declarative/qtdeclarative-everywhere-src-6.6.0/src/quick/items/qquickitem.cpp:8871
    #4  0x00007ffff1f7318b in QApplicationPrivate::notify_helper(QObject*, QEvent*) (this=<optimized out>, receiver=0x555556b6cd20, e=0x7fffffffa320)
        at /usr/src/debug/qt6-base/qtbase-everywhere-src-6.6.0/src/widgets/kernel/qapplication.cpp:3290
    #5  0x00007ffff295e4a7 in  () at /usr/lib/python3.11/site-packages/PyQt6/QtWidgets.abi3.so
    #6  0x00007ffff59626d8 in QCoreApplication::notifyInternal2(QObject*, QEvent*) (receiver=0x555556b6cd20, event=0x7fffffffa320) at /usr/src/debug/qt6-base/qtbase-everywhere-src-6.6.0/src/corelib/kernel/qcoreapplication.cpp:1118
    #7  0x00007ffff596271d in QCoreApplication::sendEvent(QObject*, QEvent*) (receiver=<optimized out>, event=<optimized out>) at /usr/src/debug/qt6-base/qtbase-everywhere-src-6.6.0/src/corelib/kernel/qcoreapplication.cpp:1536
    #8  0x00007fffe4f33f15 in QQuickDeliveryAgentPrivate::setFocusInScope(QQuickItem*, QQuickItem*, Qt::FocusReason, QFlags<QQuickDeliveryAgentPrivate::FocusOption>)
        (this=<optimized out>, scope=<optimized out>, item=<optimized out>, reason=<optimized out>, options=...) at /usr/src/debug/qt6-declarative/qtdeclarative-everywhere-src-6.6.0/src/quick/util/qquickdeliveryagent.cpp:439
    #9  0x00007fffe4dd348a in QQuickItem::setFocus(bool, Qt::FocusReason) (this=0x555556b724d0, focus=<optimized out>, reason=Qt::TabFocusReason) at /usr/include/qt6/QtCore/qflags.h:73
    #10 0x00007fffe4e7239b in QQuickWindow::focusInEvent(QFocusEvent*) (this=<optimized out>, ev=<optimized out>) at /usr/src/debug/qt6-declarative/qtdeclarative-everywhere-src-6.6.0/src/quick/items/qquickwindow.cpp:231
    #11 0x00007ffff1fc3a05 in QWidget::event(QEvent*) (this=0x555556457b50, event=0x7fffffffa770) at /usr/src/debug/qt6-base/qtbase-everywhere-src-6.6.0/src/widgets/kernel/qwidget.cpp:9111
    #12 0x00007ffff1f7318b in QApplicationPrivate::notify_helper(QObject*, QEvent*) (this=<optimized out>, receiver=0x555556457b50, e=0x7fffffffa770)
        at /usr/src/debug/qt6-base/qtbase-everywhere-src-6.6.0/src/widgets/kernel/qapplication.cpp:3290
    #13 0x00007ffff295e4a7 in  () at /usr/lib/python3.11/site-packages/PyQt6/QtWidgets.abi3.so
    #14 0x00007ffff59626d8 in QCoreApplication::notifyInternal2(QObject*, QEvent*) (receiver=0x555556457b50, event=0x7fffffffa770) at /usr/src/debug/qt6-base/qtbase-everywhere-src-6.6.0/src/corelib/kernel/qcoreapplication.cpp:1118
    #15 0x00007ffff596271d in QCoreApplication::sendEvent(QObject*, QEvent*) (receiver=<optimized out>, event=<optimized out>) at /usr/src/debug/qt6-base/qtbase-everywhere-src-6.6.0/src/corelib/kernel/qcoreapplication.cpp:1536
    #16 0x00007ffff1f7f1b2 in QApplicationPrivate::setFocusWidget(QWidget*, Qt::FocusReason) (focus=0x555556457b50, reason=<optimized out>) at /usr/src/debug/qt6-base/qtbase-everywhere-src-6.6.0/src/widgets/kernel/qapplication.cpp:1538
    #17 0x00007ffff1fca29d in QWidget::setFocus(Qt::FocusReason) (this=0x555556b1ceb0, reason=<optimized out>) at /usr/src/debug/qt6-base/qtbase-everywhere-src-6.6.0/src/widgets/kernel/qwidget.cpp:6580
    #18 0x00007ffff1fb4f1b in QWidget::focusNextPrevChild(bool) (this=<optimized out>, next=<optimized out>) at /usr/src/debug/qt6-base/qtbase-everywhere-src-6.6.0/src/widgets/kernel/qwidget.cpp:6844
    #19 0x00007ffff298d0ac in  () at /usr/lib/python3.11/site-packages/PyQt6/QtWidgets.abi3.so
    #20 0x00007ffff298d0ac in  () at /usr/lib/python3.11/site-packages/PyQt6/QtWidgets.abi3.so
    #21 0x00007ffff298d0ac in  () at /usr/lib/python3.11/site-packages/PyQt6/QtWidgets.abi3.so
    #22 0x00007ffff1fbdb76 in QWidgetPrivate::hide_helper() (this=this@entry=0x55555646a360) at /usr/src/debug/qt6-base/qtbase-everywhere-src-6.6.0/src/widgets/kernel/qwidget.cpp:8271
    #23 0x00007ffff1fbf158 in QWidgetPrivate::setVisible(bool) (this=0x55555646a360, visible=<optimized out>) at /usr/src/debug/qt6-base/qtbase-everywhere-src-6.6.0/src/widgets/kernel/qwidget.cpp:8447
    [...]

We fix this problem by explicitly handling focus before hiding the UI elements.
This is done with a new TabbedBrowser.on_release_focus() slot, which is bound to
signals emitted just before things are hidden: The existing Command.hide_cmd()
for the status bar, and a new release_focus() signal for prompts.

Additionally, we make sure to not double-handle hiding in the statusbar
code when it's already handled separately for comamnd mode.

Unfortunately, no tests for this, as application window focus is required to
reproduce the issue. In theory, a test in scroll.feature could be added though,
which loads simple.html, scrolls down, shows/hides a prompt or the status bar,
and then checks the vertical scroll position is != 0.

Fixes #2236
Fixes #7885
  • Loading branch information
The-Compiler committed Dec 7, 2023
1 parent c698091 commit 7593d01
Show file tree
Hide file tree
Showing 5 changed files with 50 additions and 18 deletions.
2 changes: 2 additions & 0 deletions doc/changelog.asciidoc
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,8 @@ Changed
Fixed
~~~~~

- Some web pages jumping to the top when the statusbar is hidden or (with
v3.0.x) when a prompt is hidden.
- Compatibility with PDF.js v4
- Added an elaborate workaround for a bug in QtWebEngine 6.6.0 causing crashes
on Google Mail/Meet/Chat, and a bug in QtWebEngine 6.5.0/.1/.2 causing crashes
Expand Down
3 changes: 3 additions & 0 deletions qutebrowser/mainwindow/mainwindow.py
Original file line number Diff line number Diff line change
Expand Up @@ -483,6 +483,8 @@ def _connect_signals(self):
mode_manager = modeman.instance(self.win_id)

# misc
self._prompt_container.release_focus.connect(
self.tabbed_browser.on_release_focus)
self.tabbed_browser.close_window.connect(self.close)
mode_manager.entered.connect(hints.on_mode_entered)

Expand Down Expand Up @@ -559,6 +561,7 @@ def _connect_signals(self):
self._completion.on_clear_completion_selection)
self.status.cmd.hide_completion.connect(
self._completion.hide)
self.status.cmd.hide_cmd.connect(self.tabbed_browser.on_release_focus)

def _set_decoration(self, hidden):
"""Set the visibility of the window decoration via Qt."""
Expand Down
17 changes: 13 additions & 4 deletions qutebrowser/mainwindow/prompt.py
Original file line number Diff line number Diff line change
Expand Up @@ -202,6 +202,7 @@ def _on_mode_left(self, mode):
log.prompt.debug("Left mode {}, hiding {}".format(
mode, self._question))
self.show_prompts.emit(None)

if self._question.answer is None and not self._question.is_aborted:
log.prompt.debug("Cancelling {} because {} was left".format(
self._question, mode))
Expand Down Expand Up @@ -261,6 +262,7 @@ class PromptContainer(QWidget):
}
"""
update_geometry = pyqtSignal()
release_focus = pyqtSignal()

def __init__(self, win_id, parent=None):
super().__init__(parent)
Expand All @@ -287,19 +289,26 @@ def _on_show_prompts(self, question):
Args:
question: A Question object or None.
"""
item = self._layout.takeAt(0)
if item is not None:
item = qtutils.add_optional(self._layout.takeAt(0))
if item is None:
widget = None
else:
widget = item.widget()
assert widget is not None
log.prompt.debug("Deleting old prompt {}".format(widget))
widget.hide()
log.prompt.debug(f"Deleting old prompt {widget!r}")
widget.deleteLater()

if question is None:
log.prompt.debug("No prompts left, hiding prompt container.")
self._prompt = None
self.release_focus.emit()
self.hide()
return
elif widget is not None:
# We have more prompts to show, just hide the old one.
# This needs to happen *after* we possibly hid the entire prompt container,
# so that keyboard focus can be reassigned properly via release_focus.
widget.hide()

classes = {
usertypes.PromptMode.yesno: YesNoPrompt,
Expand Down
12 changes: 8 additions & 4 deletions qutebrowser/mainwindow/statusbar/bar.py
Original file line number Diff line number Diff line change
Expand Up @@ -377,9 +377,11 @@ def set_text(self, text):
@pyqtSlot(usertypes.KeyMode)
def on_mode_entered(self, mode):
"""Mark certain modes in the commandline."""
mode_manager = modeman.instance(self._win_id)
if config.val.statusbar.show == 'in-mode':
if config.val.statusbar.show == 'in-mode' and mode != usertypes.KeyMode.command:
# Showing in command mode is handled via _show_cmd_widget()
self.show()

mode_manager = modeman.instance(self._win_id)
if mode_manager.parsers[mode].passthrough:
self._set_mode_text(mode.name)
if mode in [usertypes.KeyMode.insert,
Expand All @@ -393,9 +395,11 @@ def on_mode_entered(self, mode):
@pyqtSlot(usertypes.KeyMode, usertypes.KeyMode)
def on_mode_left(self, old_mode, new_mode):
"""Clear marked mode."""
mode_manager = modeman.instance(self._win_id)
if config.val.statusbar.show == 'in-mode':
if config.val.statusbar.show == 'in-mode' and old_mode != usertypes.KeyMode.command:
# Hiding in command mode is handled via _hide_cmd_widget()
self.hide()

mode_manager = modeman.instance(self._win_id)
if mode_manager.parsers[old_mode].passthrough:
if mode_manager.parsers[new_mode].passthrough:
self._set_mode_text(new_mode.name)
Expand Down
34 changes: 24 additions & 10 deletions qutebrowser/mainwindow/tabbedbrowser.py
Original file line number Diff line number Diff line change
Expand Up @@ -858,20 +858,34 @@ def on_mode_entered(self, mode):
assert isinstance(tab, browsertab.AbstractTab), tab
tab.data.input_mode = mode

@pyqtSlot(usertypes.KeyMode)
def on_mode_left(self, mode):
"""Give focus to current tab if command mode was left."""
@pyqtSlot()
def on_release_focus(self):
"""Give keyboard focus to the current tab when requested by statusbar/prompt.
This gets emitted by the statusbar and prompt container before they call .hide()
on themselves, with the idea that we can explicitly reassign the focus,
instead of Qt implicitly calling its QWidget::focusNextPrevChild() method,
finding a new widget to give keyboard focus to.
"""
widget = qtutils.add_optional(self.widget.currentWidget())
if widget is None:
return

log.modes.debug(f"Focus released, focusing {widget!r}")
widget.setFocus()

@pyqtSlot()
def on_mode_left(self):
"""Save input mode for restoring if needed."""
if config.val.tabs.mode_on_change != 'restore':
return

widget = qtutils.add_optional(self.widget.currentWidget())
if widget is None:
return

if mode in [usertypes.KeyMode.command] + modeman.PROMPT_MODES:
log.modes.debug("Left status-input mode, focusing {!r}".format(
widget))
widget.setFocus()
if config.val.tabs.mode_on_change == 'restore':
assert isinstance(widget, browsertab.AbstractTab), widget
widget.data.input_mode = usertypes.KeyMode.normal
assert isinstance(widget, browsertab.AbstractTab), widget
widget.data.input_mode = usertypes.KeyMode.normal

@pyqtSlot(int)
def _on_current_changed(self, idx):
Expand Down

0 comments on commit 7593d01

Please sign in to comment.