Keep focus on "Hide" while ModalOverlay is visible #795

jadijadi · 2024-02-14T13:58:18Z

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

DrahtBot · 2024-02-14T13:58:21Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Reviews

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.

jadijadi · 2024-02-14T14:00:47Z

To reproduce the issue (tested on Mac & KDE):

open bitcoin gui with wallet
Hide the sync modal
Go to send tab
Highlight the Label
Click on the progress bar to open the sync modal
press TAB and a misplaced rectangular highlight should appear

hebasto · 2024-02-15T10:51:59Z

Why does the issue happen only on certain platforms?

hebasto

... keeping the focus on the "Hide" button while the ModalOverlay is visible.

That looks like a better commit message / PR description.

src/qt/modaloverlay.cpp

jadijadi · 2024-02-15T11:26:37Z

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.

hebasto

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 1b34e0d. Also tested on Ubuntu 22.04.

src/qt/modaloverlay.cpp

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 bitcoin-core#783

jadijadi · 2024-02-17T14:24:46Z

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 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.

hebasto · 2024-02-17T14:55:15Z

cc @jarolrod @furszy @Sjors

luke-jr · 2024-02-20T22:34:12Z

Wouldn't it be better to hide the covered widgets instead?

hebasto · 2024-03-04T10:19:49Z

@jadijadi

Are you still working on this? If so, do you mind addressing #795 (comment)?

jadijadi · 2024-03-04T13:54:56Z

@jadijadi

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.

jadijadi · 2024-03-05T06:32:42Z

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:

If I used 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.
The above method might interfere with other sourced whom are "hiding" widget in the parent gui and can make future bugs. Imagine this situation:
a. an element is disabled on the gui for any reason
b. user opens the modal
c. I will disable all widgets on the parent (and enable all parents on my CloseButton to enable it)
d. user closes the modal
e. I enable all widgets on the main window
As you can see in this situation the initial state is lost. I have to keep track of the status of all widgets to prevent this.

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.

BrandonOdiwuor

Concept ACK

pablomartin4btc

Concept & approach ACK 992b1bb

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.

jadijadi · 2024-05-08T15:53:13Z

Please do not hesitate to ask me if any other update is needed on this from my side. Thanks.

hebasto

re-ACK 992b1bb

…s visible 992b1bb qt: keep focus on "Hide" while ModalOverlay is visible (Jadi) Pull request description: 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 dashpay#783 ACKs for top commit: pablomartin4btc: Concept & approach ACK 992b1bb hebasto: re-ACK 992b1bb Tree-SHA512: f702a3fd51db4bc10780bccf76394e35a6b5fb45db72c9c23cd10d777106b08c61077d2d989003838921e76d2cb44f809399f31df76448e4305a6c2a71b5c6a3

b654479 Merge bitcoin#30705: test: Avoid intermittent block download timeout in p2p_ibd_stalling (merge-script) 745a819 Merge bitcoin#30690: devtools, utxo-snapshot: Fix block height out of range in script (Ava Chow) 01b570e Merge bitcoin#29999: guix: fix suggested fake date for openssl-1.1.1l (Ava Chow) 432f352 Merge bitcoin#30580: doc: Add note about distro's `g++-mingw-w64-x86-64-posix` version (merge-script) 1bd090e Merge bitcoin#30597: doc: Drop no longer needed workaround for WSL (merge-script) 8a12237 Merge bitcoin#30630: doc: Update ccache website link (merge-script) f66547f Merge bitcoin#30588: depends: fix ZMQ CMake getcachesize check (merge-script) ddaec96 Merge bitcoin#30565: depends: Fix `zeromq` build on OpenBSD (merge-script) e4e5605 Merge bitcoin#30552: test: fix constructor of msg_tx (merge-script) df3c239 Merge bitcoin#26950: cleanse: switch to SecureZeroMemory for Windows cross-compile (merge-script) 57945ce Merge bitcoin#30506: depends: Cleanup postprocess commands after switching to CMake (merge-script) e016ffa Merge bitcoin#29878: depends: build expat with CMake (merge-script) 62dcd43 Merge bitcoin#29880: depends: build FreeType with CMake (merge-script) 745addf Merge bitcoin#30245: net: Allow -proxy=[::1] on nodes with IPV6 lo only (Ava Chow) 4e144be Merge bitcoin-core/gui#795: Keep focus on "Hide" while ModalOverlay is visible (Hennadii Stepanov) 69c04b2 Merge bitcoin#30372: util: Use SteadyClock in RandAddSeedPerfmon (merge-script) ebed8af Merge bitcoin#30336: depends: update doc in Qt pwd patch (merge-script) 9793fb1 Merge bitcoin#30340: test: Added coverage to Block not found error using gettxoutsetinfo (Ava Chow) 479cb8b Merge bitcoin#30312: contrib: add R(UN)PATH check to ELF symbol-check (merge-script) ca83773 Merge bitcoin#30283: upnp: fix build with miniupnpc 2.2.8 (merge-script) 63e139d Merge bitcoin#30185: guix: show `*_FLAGS` variables in pre-build output (merge-script) 3be0d3e Merge bitcoin#30097: crypto: disable asan for sha256_sse4 with clang and -O0 (merge-script) 3070c3e Merge bitcoin#30078: depends: set AR & RANLIB for CMake (merge-script) Pull request description: ## Issue being fixed or feature implemented Trivial backports ## What was done? ## How Has This Been Tested? built locally ## Breaking Changes ## Checklist: _Go over all the following points, and put an `x` in all the boxes that apply._ - [ ] I have performed a self-review of my own code - [ ] I have commented my code, particularly in hard-to-understand areas - [ ] I have added or updated relevant unit/integration/functional/e2e tests - [ ] I have made corresponding changes to the documentation - [x] I have assigned this pull request to a milestone _(for repository code-owners and collaborators only)_ ACKs for top commit: UdjinM6: utACK b654479 kwvg: utACK b654479 Tree-SHA512: 10b5af4e92c83fa9d6764b20bf066bba8e4c600402966fd5c1d6dad07b0549d8a42151a33f21e2f8263336c12a810a6f3fc2828d90bc98153e09c165d9e5b043

hebasto changed the title ~~qt: prevent weird focus rect on inital sync~~ Prevent weird focus rect on inital sync Feb 15, 2024

hebasto reviewed Feb 15, 2024

View reviewed changes

src/qt/modaloverlay.cpp Show resolved Hide resolved

src/qt/modaloverlay.cpp Show resolved Hide resolved

jadijadi force-pushed the gui-weird-focus-fix branch from e6418e6 to 1b34e0d Compare February 15, 2024 11:22

hebasto reviewed Feb 17, 2024

View reviewed changes

src/qt/modaloverlay.cpp Outdated Show resolved Hide resolved

hebasto changed the title ~~Prevent weird focus rect on inital sync~~ Keep focus on "Hide" while ModalOverlay is visible Feb 17, 2024

hebasto added this to the 27.0 milestone Feb 17, 2024

jadijadi force-pushed the gui-weird-focus-fix branch from 1b34e0d to 992b1bb Compare February 17, 2024 14:22

hebasto modified the milestones: 27.0, 28.0 Mar 4, 2024

BrandonOdiwuor reviewed Mar 21, 2024

View reviewed changes

pablomartin4btc approved these changes Mar 26, 2024

View reviewed changes

DrahtBot requested review from hebasto and BrandonOdiwuor March 26, 2024 22:15

hebasto approved these changes Jul 15, 2024

View reviewed changes

hebasto merged commit 6ae903e into bitcoin-core:master Jul 15, 2024

bitcoin-core locked and limited conversation to collaborators Jul 15, 2025

Keep focus on "Hide" while ModalOverlay is visible #795

Keep focus on "Hide" while ModalOverlay is visible #795

Uh oh!

Conversation

jadijadi commented Feb 14, 2024

Uh oh!

DrahtBot commented Feb 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Reviews

Uh oh!

jadijadi commented Feb 14, 2024

Uh oh!

hebasto commented Feb 15, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jadijadi commented Feb 15, 2024

Uh oh!

hebasto left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jadijadi commented Feb 17, 2024

Uh oh!

hebasto commented Feb 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr commented Feb 20, 2024

Uh oh!

hebasto commented Mar 4, 2024

Uh oh!

jadijadi commented Mar 4, 2024

Uh oh!

jadijadi commented Mar 5, 2024

Uh oh!

BrandonOdiwuor left a comment

Choose a reason for hiding this comment

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

jadijadi commented May 8, 2024

Uh oh!

hebasto left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Feb 14, 2024 •

edited

Loading

hebasto commented Feb 15, 2024 •

edited

Loading

hebasto left a comment •

edited

Loading

hebasto commented Feb 17, 2024 •

edited

Loading