Skip to content

Add focus outlines to controls used during onboarding #214

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.

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

Closed
wants to merge 6 commits into from

Conversation

johnny9
Copy link
Collaborator

@johnny9 johnny9 commented Jan 2, 2023

Changes to the controls within the Onboarding flows to show which control is in active focus. This will be the main indicator when navigating with a keyboard/tabbing.

Windows
Intel macOS
Apple Silicon macOS
ARM64 Android

@jarolrod
Copy link
Member

jarolrod commented Jan 3, 2023

concept ACK, going in the right direction here.

Not a blocker for this PR but: seems we can cycle into components in pages that aren't currently in view, right now this is an issue for proper tab-focus functionality and for accessibility features like a screenreader.

A benefit of doing this is that it exposes some slight padding+layout issues with some components :D

@johnny9
Copy link
Collaborator Author

johnny9 commented Jan 3, 2023

Some of these come out a little strange. TextEdits and external links in particular.

Continue button and top right icon Back and about page TextEdits and switch OptionButtons
Screenshot from 2023-01-02 21-22-00 Screenshot from 2023-01-02 21-22-14 Screenshot from 2023-01-03 07-54-54 Screenshot from 2023-01-03 07-54-13

ContinueButton and the OptionButtons work well. The navigation buttons are close but the alignment needs a little work since the icon makes the widths odd. Looking for thoughts mainly on the TextEdits and Labels for external links/about information.

@johnny9
Copy link
Collaborator Author

johnny9 commented Jan 4, 2023

concept ACK, going in the right direction here.

Not a blocker for this PR but: seems we can cycle into components in pages that aren't currently in view, right now this is an issue for proper tab-focus functionality and for accessibility features like a screenreader.

Yeah, I've been working on the KeyNavigation to prevent the focus from going to outside pages. It's more complicated than I thought it would be but it seems to be workable. Good motivation to get some simple qml testing going too since its something that could be broken fairly easily with changes to the controls.

anchors.leftMargin: -15
anchors.rightMargin: -4
anchors.topMargin: -4
anchors.bottomMargin: -5
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why have different values like here? The border should be nicely defined and uniform, it is the elements themselves that differ in how much space they take up, and we need to address those issues in follow-ups.

I would keep the definitions in all of these to just specify anchors.margins: -4 and address the issues in the elements themselves, instead of changing how the border works to accommodate the size issues.

It might be possible to extrapolate this out into our theme file, but we can leave that for much later.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, I like that approach. I'll try it out and see if it can be adjusted properly with margins/padding values on the elements.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm going to ACK this

this pr doesn't have to address the issues with the elements, only to keep the declaration of the margins of the border to only anchors.margins: -4 for now, alternatively we can merge like this and address the issues in a follow-up and clean up the definition of the border.

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 6f00bc3

@hebasto
Copy link
Member

hebasto commented Jan 17, 2023

Tested on Ubuntu 22.04. Having multiple warnings:

qrc:/qml/controls/ValueInput.qml:22:9: Unable to assign [undefined] to bool

@hebasto
Copy link
Member

hebasto commented Feb 4, 2023

@johnny9

Mind rebasing?

And #214 (comment) remains unaddressed...

@johnny9
Copy link
Collaborator Author

johnny9 commented Feb 5, 2023

Update from 6f00bc3 to 85317d3:

  • Rebase with main
  • Address Jarol's comment by using margins: -4 for all components
  • Make ValueInput a Control so it can receive tab focus and fix the error seen by hebasto

Copy link
Member

@jarolrod jarolrod left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The tab focus for Setting should be the setting row itself, we'd have to move out the separator rectangle from the setting control into the ColumnLayout containing the settings though

Screen Shot 2023-02-04 at 10 47 01 PM

@jarolrod
Copy link
Member

Replaced by #263

@hebasto hebasto closed this Feb 14, 2023
hebasto added a commit that referenced this pull request Feb 14, 2023
5cbf67f qml: use Separator component to separate settings (jarolrod)
09aa701 qml: introduce Separator component (jarolrod)

Pull request description:

  This is required in order to properly support the feature attempted by #214

  ### Master

  | About | Dev | Prune | Connection | Node |
  | ----- | --- | ----- | ---------- | ---- |
  | <img width="752" alt="about-master" src="https://user-images.githubusercontent.com/23396902/218625414-180b5a16-6a54-4c30-9f7f-6fd7ba667a6d.png"> |<img width="752" alt="dev-master" src="https://user-images.githubusercontent.com/23396902/218629878-7a1b1024-5b25-446e-9159-6160f0699a8a.png"> |<img width="752" alt="prune-master" src="https://user-images.githubusercontent.com/23396902/218629926-f61080f2-e20a-42f8-8f1c-7dac9ec494a9.png"> |<img width="752" alt="connection-master" src="https://user-images.githubusercontent.com/23396902/218629963-baebb34c-9615-435a-9f7c-24a1a88c8706.png"> |<img width="752" alt="node-master" src="https://user-images.githubusercontent.com/23396902/218629984-1d6a2b5e-d4a4-4baf-ba28-041f90214036.png"> |

  ### PR

  | About | Dev | Prune | Connection | Node |
  | ----- | --- | ----- | ---------- | ---- |
  | <img width="752" alt="about-pr" src="https://user-images.githubusercontent.com/23396902/218625012-4fb45c2c-4911-422d-a83c-471e64b05fd1.png"> |<img width="752" alt="dev-pr" src="https://user-images.githubusercontent.com/23396902/218625047-78e6ca4d-d900-4bf7-9f69-77c1a40e0920.png"> |<img width="752" alt="prune-pr" src="https://user-images.githubusercontent.com/23396902/218625067-a71ee530-60c9-4c6f-86eb-f3dccac4230e.png"> |<img width="752" alt="connection-pr" src="https://user-images.githubusercontent.com/23396902/218625084-3811d101-c67b-4f8a-89e7-fc19d58ae609.png"> |<img width="752" alt="node-pr" src="https://user-images.githubusercontent.com/23396902/218625114-ec06b0ec-e37c-403c-a9c5-68b919375cba.png"> |

  This also gets us in line with the design file in regards to the height of a setting
  | master | pr | design |
  | ------ | -- | ------ |
  | <img width="483" alt="Screen Shot 2023-02-13 at 7 30 58 PM" src="https://user-images.githubusercontent.com/23396902/218630233-cb861aad-9541-419a-9252-6fbbe697bad8.png"> | <img width="483" alt="Screen Shot 2023-02-13 at 7 21 49 PM" src="https://user-images.githubusercontent.com/23396902/218630255-e428a704-9b8c-41ee-a065-ebef73a9f75e.png"> | <img width="483" alt="Screen Shot 2023-02-13 at 7 31 54 PM" src="https://user-images.githubusercontent.com/23396902/218630286-6bb415a7-eca6-44d3-8d12-faf5dfe283b7.png"> |

  [![Windows](https://img.shields.io/badge/OS-Windows-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/win64/insecure_win_gui.zip?branch=pull/263)
  [![Intel macOS](https://img.shields.io/badge/OS-Intel%20macOS-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/macos/insecure_mac_gui.zip?branch=pull/263)
  [![Apple Silicon macOS](https://img.shields.io/badge/OS-Apple%20Silicon%20macOS-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/macos_arm64/insecure_mac_arm64_gui.zip?branch=pull/263)
  [![ARM64 Android](https://img.shields.io/badge/OS-Android-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/android/insecure_android_apk.zip?branch=pull/263)

ACKs for top commit:
  johnny9:
    ACK 5cbf67f

Tree-SHA512: 45bb90348b780b364a2165aac6f585b1428f60de276c866fe889385b3226726a44e4747b1f1768d62e94bc15343036089df29f209157cb67454cbcf26aece2f3
hebasto added a commit that referenced this pull request Feb 28, 2023
ab0ea57 qml: add FocusBorder to Focus-able components (jarolrod)
c54a2c7 qml: Introduce FocusBorder component (Jarol Rodriguez)
31ec28d qml: inset NavButtons 4px from borders to give space for focus outline (jarolrod)

Pull request description:

  Replaces #214

  Adds the focus outline to the (intended) focusable components:

  | a | b | c | d | e | f | g | h | i | j | k |
  | - | - | - | - | - | - | - | - | - | - | - |
  | <img width="752" alt="Screen Shot 2023-02-13 at 8 48 31 PM" src="https://user-images.githubusercontent.com/23396902/218634844-31045e76-5673-4d74-8606-8c1b27a3cf5e.png"> |  <img width="752" alt="Screen Shot 2023-02-13 at 8 48 40 PM" src="https://user-images.githubusercontent.com/23396902/218634877-f935fbd2-289c-4967-a31e-953daeda08d2.png"> |  <img width="752" alt="Screen Shot 2023-02-13 at 8 48 45 PM" src="https://user-images.githubusercontent.com/23396902/218634925-d42f984e-a58d-43bb-88b3-49de6a273ff0.png"> |  <img width="752" alt="Screen Shot 2023-02-13 at 8 48 53 PM" src="https://user-images.githubusercontent.com/23396902/218634957-7b0d458b-80c2-4e02-938f-20324e2235fa.png"> |  <img width="752" alt="Screen Shot 2023-02-13 at 8 48 55 PM" src="https://user-images.githubusercontent.com/23396902/218634993-ae906fc9-0410-4551-a3a8-890bacea76fc.png"> | <img width="752" alt="Screen Shot 2023-02-13 at 8 48 58 PM" src="https://user-images.githubusercontent.com/23396902/218635019-040cd823-e9f8-4e86-b35b-8f4248b9d6a8.png"> | <img width="752" alt="Screen Shot 2023-02-13 at 8 49 01 PM" src="https://user-images.githubusercontent.com/23396902/218635041-820f0e49-9abc-4b61-8a7b-ec18a927d12f.png"> |  <img width="752" alt="Screen Shot 2023-02-13 at 8 49 19 PM" src="https://user-images.githubusercontent.com/23396902/218635060-3f94b33f-2be2-443b-a681-09ee14e9c467.png"> |  <img width="752" alt="Screen Shot 2023-02-13 at 8 49 21 PM" src="https://user-images.githubusercontent.com/23396902/218635100-1a1aec9b-38c6-4d62-9265-3f9700b07d4e.png"> |  <img width="752" alt="Screen Shot 2023-02-13 at 8 49 26 PM" src="https://user-images.githubusercontent.com/23396902/218635123-ef687083-aee4-4db0-9c1d-7dd8ebfdd75c.png"> | <img width="752" alt="Screen Shot 2023-02-13 at 8 49 31 PM" src="https://user-images.githubusercontent.com/23396902/218635147-f00d6480-337f-4947-88a6-920c9f463d12.png"> |

  #219 is still something we have to fix in order for this to work properly

  Additionally, this insets the navbuttons 4px from the borders of the GUI to allow for the focus outline to show properly. I believe that **GBKS** intended the buttons to be inset a bit from the borders

  | a | b |
  | - | - |
  | <img width="752" alt="Screen Shot 2023-02-13 at 8 36 33 PM" src="https://user-images.githubusercontent.com/23396902/218636059-13ecf932-5e41-4bb7-94bb-264523283438.png"> |  <img width="752" alt="Screen Shot 2023-02-13 at 8 38 57 PM" src="https://user-images.githubusercontent.com/23396902/218636186-499c5549-b2cc-4325-aade-022be3f47e85.png"> |

  [![Windows](https://img.shields.io/badge/OS-Windows-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/win64/insecure_win_gui.zip?branch=pull/264)
  [![Intel macOS](https://img.shields.io/badge/OS-Intel%20macOS-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/macos/insecure_mac_gui.zip?branch=pull/264)
  [![Apple Silicon macOS](https://img.shields.io/badge/OS-Apple%20Silicon%20macOS-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/macos_arm64/insecure_mac_arm64_gui.zip?branch=pull/264)
  [![ARM64 Android](https://img.shields.io/badge/OS-Android-green)](https://api.cirrus-ci.com/v1/artifact/github/bitcoin-core/gui-qml/android/insecure_android_apk.zip?branch=pull/264)

ACKs for top commit:
  johnny9:
    ACK ab0ea57

Tree-SHA512: f26ca941d0d3a8026846f3a86a7867fb437aeb3b3aca2af3054ce08c57f46f16c8536691b49a9d08dba8f14009d771f9dbddfe9e61d886ee2cc59ddcdb25db0a
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants