gui icon indicating copy to clipboard operation
gui copied to clipboard

Keep focus on "Hide" while ModalOverlay is visible

Open jadijadi opened this issue 1 year ago • 11 comments

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

jadijadi avatar Feb 14 '24 13:02 jadijadi

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.

DrahtBot avatar Feb 14 '24 13:02 DrahtBot

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

jadijadi avatar Feb 14 '24 14:02 jadijadi

Why does the issue happen only on certain platforms?

hebasto avatar Feb 15 '24 10:02 hebasto

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.

jadijadi avatar Feb 15 '24 11:02 jadijadi

I can actually see two distinct issues here:

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

jadijadi avatar Feb 17 '24 14:02 jadijadi

cc @jarolrod @furszy @Sjors

hebasto avatar Feb 17 '24 14:02 hebasto

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

luke-jr avatar Feb 20 '24 22:02 luke-jr

@jadijadi

Are you still working on this? If so, do you mind addressing https://github.com/bitcoin-core/gui/pull/795#issuecomment-1955232548?

hebasto avatar Mar 04 '24 10:03 hebasto

@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 avatar Mar 04 '24 13:03 jadijadi

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:

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

jadijadi avatar Mar 05 '24 06:03 jadijadi

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

jadijadi avatar May 08 '24 15:05 jadijadi