Skip to content

qt: Enable tabbing through labels #14810

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

qt: Enable tabbing through labels #14810

wants to merge 3 commits into from

Conversation

hebasto
Copy link
Member

@hebasto hebasto commented Nov 26, 2018 •
edited
Loading

This PR:

Also applied a style-fix from #15220:

please just do this the next time you're editing these files.

@hebasto hebasto mentioned this pull request Nov 26, 2018
Closed
@laanwj
Copy link
Member

laanwj commented Nov 26, 2018 via email

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 26, 2018 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@fanquake fanquake added the GUI label Nov 26, 2018
@jonasschnelli
Copy link
Contributor

The only downside with this is, that you can't select parts of the text of a label.

@laanwj
Copy link
Member

laanwj commented Dec 13, 2018

@jonasschnelli Yeah. Possibly we're getting too deep in shed-painting territory here. I'll leave the decision what to do here up to you.

@jonasschnelli
Copy link
Contributor

More opinions, tests would be welcome.

@hebasto
Copy link
Member Author

hebasto commented Dec 15, 2018

@jonasschnelli @laanwj

The only downside with this is, that you can't select parts of the text of a label.

That has been made intentionally.
Rationale: some labels are modified by setText() and any user selected text will vanish.
In such circumstances, only two options are consistent: all-selected or none-selected.
To make a focused label clearly visible to a user the former option has been chosen.

@hebasto hebasto force-pushed the 20181124-tab-through-labels branch from 1223621 to 98a7271 Compare January 20, 2019 23:42
@hebasto hebasto force-pushed the 20181124-tab-through-labels branch from 98a7271 to ba5fc32 Compare January 21, 2019 00:30
@hebasto
Copy link
Member Author

hebasto commented Jan 21, 2019

Rebased.

@hebasto
Copy link
Member Author

hebasto commented Jan 21, 2019

@promag @Sjors would you mind reviewing this PR?

@promag
Copy link
Contributor

promag commented Jan 21, 2019

@hebasto sure, on my list now.

@Sjors
Copy link
Member

Sjors commented Jan 21, 2019

Concept ACK.

Unfortunately this is a bit difficult to fully test on macOS because the menu keyboard shortcuts don't work (#13832), so you need the mouse for most navigation anyway.

I'm confused why forms/overviewpage.ui is modified; what's the point of being able to focus on the amounts? Is it so they're easier to copy-paste? Or is it an accessibility feature?

I was already able to navigate through the peer list. Now with this change I can select and copy individual items.

When I tab through the Send screen, it only goes through Pay to, label, amount, [nothing] and then the balance field.

On the receive screen it skips the BTC dropdown field, the payment request button and the erase button. I can go through the list of historical payment request, but I can't access their view and delete buttons.

On the transactions page I can't access the the three filter dropdowns (on the left), and also not the Export button.

I also can't reach the wallet switcher.

@hebasto
Copy link
Member Author

hebasto commented Jan 21, 2019

@Sjors Thank you for your review and valuable input.

I'm confused why forms/overviewpage.ui is modified; what's the point of being able to focus on the amounts? Is it so they're easier to copy-paste? Or is it an accessibility feature?

Yes, it is an accessibility feature mostly. Ref: #14577 (comment).

All others points will be addressed soon.

@Sjors
Copy link
Member

Sjors commented Jan 22, 2019

In that case, is there a way to trigger the tooltip by keyboard too?

@promag
Copy link
Contributor

promag commented Apr 29, 2019

Just wondering if changing to QLineEdit with readOnly=true makes sense? At least visually it's clear?

@promag
Copy link
Contributor

promag commented Apr 29, 2019

BTW I've noticed that #12616 no longer works.

practicalswift
practicalswift reviewed May 7, 2019
View reviewed changes
@jonasschnelli
Copy link
Contributor

Tested this again and the lack of ability to select parts of an amount label (say the amount without the unit BTC) makes me NACK this as it is now.

@hebasto hebasto closed this Oct 9, 2019
This was referenced Jan 3, 2020
Merged
Closed
@hebasto hebasto reopened this Jan 9, 2020
@hebasto hebasto force-pushed the 20181124-tab-through-labels branch from ba5fc32 to 8b6a472 Compare January 9, 2020 11:23
@hebasto
Copy link
Member Author

hebasto commented Jan 9, 2020

@promag

Just wondering if changing to QLineEdit with readOnly=true makes sense? At least visually it's clear?

This approach does not work for me (background color issues), see #17898.

BTW I've noticed that #12616 no longer works.

Fixed in the latest push.

@jonasschnelli

The only downside with this is, that you can't select parts of the text of a label.

Tested this again and the lack of ability to select parts of an amount label (say the amount without the unit BTC) makes me NACK this as it is now.

Fixed in the latest push.

@promag
Copy link
Contributor

promag commented Jan 9, 2020

@hebasto thanks for trying my suggestion in #17898. Will try and review both.

@hebasto
Copy link
Member Author

hebasto commented Jan 9, 2020

The OP has been updated.

@hebasto
Copy link
Member Author

hebasto commented Jan 9, 2020

@Sjors

In that case, is there a way to trigger the tooltip by keyboard too?

It seems possible. This could be addresses in a follow up PR ;)

@hebasto hebasto force-pushed the 20181124-tab-through-labels branch from 8b6a472 to d356340 Compare January 9, 2020 12:21
@hebasto
Copy link
Member Author

hebasto commented Jan 9, 2020

Invariants are forced by HighlightLabelWidget ctor.

@hebasto
Copy link
Member Author

hebasto commented Apr 11, 2020

@jonasschnelli Mind re-reviewing?

@hebasto hebasto force-pushed the 20181124-tab-through-labels branch from d356340 to 1bd1e60 Compare April 11, 2020 16:08
@hebasto
Copy link
Member Author

hebasto commented Apr 11, 2020

Rebased d356340 -> 1bd1e60 (pr14810.101 -> pr14810.102) due to the conflict with #18402.

@DrahtBot DrahtBot mentioned this pull request May 6, 2020
Closed
@jonasschnelli
Copy link
Contributor

I'm unsure if it is worth customising the label based on mouse events to achieve tabbing thought labels.
Also, on macOS, the selected color is different and almost not seeable.

master:
Bildschirmfoto 2020-05-29 um 09 09 20
Bildschirmfoto 2020-05-29 um 09 09 27

this PR:
Bildschirmfoto 2020-05-29 um 09 10 44
Bildschirmfoto 2020-05-29 um 09 11 07

@DrahtBot DrahtBot mentioned this pull request Jun 7, 2020
Closed
@hebasto hebasto mentioned this pull request Jun 8, 2020
Merged
@hebasto
Copy link
Member Author

hebasto commented Jun 8, 2020

This PR:

This fix is replaced by #19210.

  • improves UX by enabling moving focus through fields using keyboard (Tab and Shift+Tab as usual) on Overview tab of the main window and on Info tab of the Debug window (other tabs leaved untouched as tab order for them seems a bit entangled). Inspired by laanwj's #14577 (comment)

@jonasschnelli

I'm unsure if it is worth customising the label based on mouse events to achieve tabbing thought labels.
Also, on macOS, the selected color is different and almost not seeable.

I have some ideas how to get the keyboard-only UX but not going to work on it right now.
So closing this PR.

Please label it "Up for grabs" :)

@hebasto hebasto closed this Jun 8, 2020
laanwj added a commit that referenced this pull request Jul 15, 2020
@laanwj
Merge #19210: qt: Get rid of cursor in out-of-focus labels
d626a3b 
bd315eb qt: Get rid of cursor in out-of-focus labels (Hennadii Stepanov)

Pull request description:

  After clicking on `QLabel` with selectable text the cursor remains forever:

  ![47532924-65e7b200-d8ba-11e8-9254-7bde658961cb](https://user-images.githubusercontent.com/32963518/84038485-ad945200-a9a8-11ea-89e3-c7c17d02a611.png)

  This PR fixes this visual bug.

  Earlier attempts to fix this issue:
  - #14577
  - #14810 (combined with other UX feature)

ACKs for top commit:
  promag:
    Code review ACK bd315eb.
  laanwj:
    Tested ACK bd315eb

Tree-SHA512: 6bf89362412e5ce9a4dec6944b62fe44fc31ca49cda7f6e2eb37e847fac9dccb68bca7ac6877b19e42add2333e40d0b4265757ead105ac0a5d28f8ab43b322c3
@hebasto hebasto deleted the 20181124-tab-through-labels branch July 15, 2020 15:19
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Jul 16, 2020
@laanwj
Merge bitcoin#19210: qt: Get rid of cursor in out-of-focus labels
6db274e 
bd315eb qt: Get rid of cursor in out-of-focus labels (Hennadii Stepanov)

Pull request description:

  After clicking on `QLabel` with selectable text the cursor remains forever:

  ![47532924-65e7b200-d8ba-11e8-9254-7bde658961cb](https://user-images.githubusercontent.com/32963518/84038485-ad945200-a9a8-11ea-89e3-c7c17d02a611.png)

  This PR fixes this visual bug.

  Earlier attempts to fix this issue:
  - bitcoin#14577
  - bitcoin#14810 (combined with other UX feature)

ACKs for top commit:
  promag:
    Code review ACK bd315eb.
  laanwj:
    Tested ACK bd315eb

Tree-SHA512: 6bf89362412e5ce9a4dec6944b62fe44fc31ca49cda7f6e2eb37e847fac9dccb68bca7ac6877b19e42add2333e40d0b4265757ead105ac0a5d28f8ab43b322c3
@Sjors Sjors mentioned this pull request Oct 17, 2020
Closed
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Sep 21, 2021
@laanwj @PastaPastaPasta
Merge bitcoin#19210: qt: Get rid of cursor in out-of-focus labels
3b0144b 
bd315eb qt: Get rid of cursor in out-of-focus labels (Hennadii Stepanov)

Pull request description:

  After clicking on `QLabel` with selectable text the cursor remains forever:

  ![47532924-65e7b200-d8ba-11e8-9254-7bde658961cb](https://user-images.githubusercontent.com/32963518/84038485-ad945200-a9a8-11ea-89e3-c7c17d02a611.png)

  This PR fixes this visual bug.

  Earlier attempts to fix this issue:
  - bitcoin#14577
  - bitcoin#14810 (combined with other UX feature)

ACKs for top commit:
  promag:
    Code review ACK bd315eb.
  laanwj:
    Tested ACK bd315eb

Tree-SHA512: 6bf89362412e5ce9a4dec6944b62fe44fc31ca49cda7f6e2eb37e847fac9dccb68bca7ac6877b19e42add2333e40d0b4265757ead105ac0a5d28f8ab43b322c3
kwvg pushed a commit to kwvg/dash that referenced this pull request Oct 12, 2021
@laanwj @kwvg
Merge bitcoin#19210: qt: Get rid of cursor in out-of-focus labels
f8470b4 
bd315eb qt: Get rid of cursor in out-of-focus labels (Hennadii Stepanov)

Pull request description:

  After clicking on `QLabel` with selectable text the cursor remains forever:

  ![47532924-65e7b200-d8ba-11e8-9254-7bde658961cb](https://user-images.githubusercontent.com/32963518/84038485-ad945200-a9a8-11ea-89e3-c7c17d02a611.png)

  This PR fixes this visual bug.

  Earlier attempts to fix this issue:
  - bitcoin#14577
  - bitcoin#14810 (combined with other UX feature)

ACKs for top commit:
  promag:
    Code review ACK bd315eb.
  laanwj:
    Tested ACK bd315eb

Tree-SHA512: 6bf89362412e5ce9a4dec6944b62fe44fc31ca49cda7f6e2eb37e847fac9dccb68bca7ac6877b19e42add2333e40d0b4265757ead105ac0a5d28f8ab43b322c3
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
1 more reviewer

@practicalswift practicalswift practicalswift left review comments

Reviewers whose approvals may not affect merge requirements
Assignees
No one assigned
Labels
GUI
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@hebasto @laanwj @DrahtBot @jonasschnelli @promag @Sjors @practicalswift @fanquake