gui
gui copied to clipboard
Keep focus on "Hide" while ModalOverlay is visible
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.
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.
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
Why does the issue happen only on certain platforms?
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.
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.
cc @jarolrod @furszy @Sjors
Wouldn't it be better to hide the covered widgets instead?
@jadijadi
Are you still working on this? If so, do you mind addressing https://github.com/bitcoin-core/gui/pull/795#issuecomment-1955232548?
@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.
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 theirhide(), 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 uglyif (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.
Please do not hesitate to ask me if any other update is needed on this from my side. Thanks.