During the initial sync, the Tab moves the focus to the widgets of the main window, even when the ModalOverlay is visible. This creates some weird rectangular selections on the screen.
This PR fixes this by keeping the focus on the “Hide” button while the ModalOverlay is visible.
Fixes #783
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | pablomartin4btc |
| Concept ACK | BrandonOdiwuor |
| Stale ACK | hebasto |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
To reproduce the issue (tested on Mac & KDE):
58@@ -58,6 +59,10 @@ bool ModalOverlay::eventFilter(QObject * obj, QEvent * ev) {
59 raise();
60 }
61 }
62+
63+ if (obj == ui->closeButton && ev->type() == QEvent::FocusOut && layerIsVisible)
64+ ui->closeButton->setFocus(Qt::OtherFocusReason);
189@@ -185,6 +190,9 @@ void ModalOverlay::showHide(bool hide, bool userRequested)
190 m_animation.setEndValue(QPoint(0, hide ? height() : 0));
191 m_animation.start(QAbstractAnimation::KeepWhenStopped);
192 layerIsVisible = !hide;
193+
194+ if (!hide)
195+ ui->closeButton->setFocus(Qt::OtherFocusReason);
… keeping the focus on the “Hide” button while the ModalOverlay is visible.
That looks like a better commit message / PR description.
Why does the issue happen only on certain platforms?
Not sure about other platforms. I had a mac and a KDE machine and happened on both of them. Have not checked on other platforms.
190@@ -185,6 +191,10 @@ void ModalOverlay::showHide(bool hide, bool userRequested)
191 m_animation.setEndValue(QPoint(0, hide ? height() : 0));
192 m_animation.start(QAbstractAnimation::KeepWhenStopped);
193 layerIsVisible = !hide;
194+
195+ if (!hide) {
Using the same variable across all conditions for the same action improves readability:
0 if (layerIsVisible) {
I can actually see two distinct issues here:
The “Hide” button doesn’t have focus when the overlay shows for the first time. This can be resolved with ui->closeButton->setFocus(); in the ModalOverlay constructor.
The other issue is a bit convoluted. As a child widget, the overlay also follows its parent’s tab order when processing “Tab” or “Shift+Tab” keys. Isolating the overlay’s tab order might also be a solution to the issue.
This PR indeed forces focus on the “Hide” button as long as the overlay is visible. This approach is acceptable as the “Hide” button is the only one in the overlay.
Concept and approach ACK 1b34e0deb273bacf0198bb232650b0a324346434. Also tested on Ubuntu 22.04.
During the initial sync, the Tab moves the focus to the widgets
of the main window, even when the ModalOverlay is visible. This
creates some weird rectangular *selections on the screen*.
This PR fixes this by keeping the focus on the "Hide" button while
the ModalOverlay is visible.
Fixes #783
I can actually see two distinct issues here:
- The “Hide” button doesn’t have focus when the overlay shows for the first time. This can be resolved with
ui->closeButton->setFocus();in theModalOverlayconstructor.- The other issue is a bit convoluted. As a child widget, the overlay also follows its parent’s tab order when processing “Tab” or “Shift+Tab” keys. Isolating the overlay’s tab order might also be a solution to the issue.
This PR indeed forces focus on the “Hide” button as long as the overlay is visible. This approach is acceptable as the “Hide” button is the only one in the overlay.
Concept and approach ACK 1b34e0d. Also tested on Ubuntu 22.04.
Thanks for the ACK. Algo agree with your point on better readability of layerIsVisible. Added that to the commit & squashed.
Are you still working on this? If so, do you mind addressing #795 (comment)?
Are you still working on this? If so, do you mind addressing #795 (comment)?
sorry I have missed that comment. Will check it and come back to you soon.
Wouldn’t it be better to hide the covered widgets instead?
Thanks for the suggestion @luke-jr . This is the path I tried first but moved to the current solution because of two issues:
parent()->findChildren<QWidget *>() to find the widgets “behind” this modal and call their hide(), the current modal and its closeButton will be disabled too. To prevent this we have to enable all the direct parents of the closeButton and this needs another loop or a few IMO ugly if (child != ui->closeButton....) clauses.Because of these two, I chose the method I implemented but I will be more than happy to hear your comments and update the PR accordingly.
Concept & approach ACK 992b1bbd5da95ee782515fb0f5674bb7a02684b2
Tested this PR on Ubuntu 22.04 with GNOME desktop, not seen any issues with it but I couldn’t reproduce the original issue, perhaps it depends on the platform or it’s KDE’s specific (?).
I agree with the reasoning on the author’s taken approach, unless there’s another problem that @luke-jr still sees with it.