Skip to content
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

Allow navigation via tab key in welcome page #3300

Merged
merged 1 commit into from
Oct 8, 2019
Merged

Allow navigation via tab key in welcome page #3300

merged 1 commit into from
Oct 8, 2019

Conversation

cezaraugusto
Copy link
Contributor

@cezaraugusto cezaraugusto commented Aug 29, 2019

fix brave/brave-browser#5504

Welcome page navigation via tab is impossible without this change since all non-visible content is hidden visually but still accessible via keyboard. This change fixes it and improves other element focus.

Test Plan:

  1. Open welcome screen
  2. Navigate via tab

Reviewer Checklist:

  • New files have MPL-2.0 license header.
  • Request a security/privacy review as needed.
  • Adequate test coverage exists to prevent regressions
  • Verify test plan is specified in PR before merging to source

After-merge Checklist:

  • The associated issue milestone is set to the smallest version that the
    changes has landed on.
  • All relevant documentation has been updated.

Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Keyboard navigation works great! However, having a hard time seeing the outline in dark mode.

Light mode seems fairly easy to see the orange outline, but here's a clip of tabbing around in dark mode:
welcome

  • outlines for the bullets are barely visible
  • Skip welcome tour does not show any kind of focus (this is the last thing I navigate to and press space, which launches into a new tab)

@cezaraugusto
Copy link
Contributor Author

@bsclifton thanks for reviewing, updated


export default interface IThemeWelcomePage {
color: {
outlineColor: string
Copy link
Member

Choose a reason for hiding this comment

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

This seems like something that would be useful for all brave components. Is there a reason outline color would be something that welcome page would have only?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree it would be useful for all brave features but adding this global default here seems outside of this PR's scope. I'm happy to do that in a follow-up task where we move our brave-core themes to this repo

Copy link
Member

Choose a reason for hiding this comment

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

I don't think it's outside of this PR scope since this is the first PR that's adding this value. Either way requires a new brave-ui version - allowing for separate layer of themes (which you've discovered is problematic and requires using the drastic any step, thereby removing all type checking for theme props), or simply adding this new property.

fix brave/brave-browser#5504

Welcome page navigation via tab is impossible without this change
since all non-visible content is hidden visually but still accessible
via keyboard. This change fixes it and improves other element focus.
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

MUCH better - looking good! 😄

@bsclifton bsclifton added this to the 0.72.x - Nightly milestone Oct 8, 2019
@bsclifton bsclifton merged commit 4086c98 into master Oct 8, 2019
@bsclifton bsclifton deleted the ca-5504 branch October 8, 2019 16:56
fmarier pushed a commit that referenced this pull request Oct 29, 2019
Support widevine linux with bundle w/o default shipping
petemill pushed a commit that referenced this pull request Jul 27, 2020
Support widevine linux with bundle w/o default shipping
petemill pushed a commit that referenced this pull request Jul 28, 2020
Support widevine linux with bundle w/o default shipping
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

improve accessibility on welcome experience
3 participants