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

fix(app): remove nav focus state on app load #11068

Merged
merged 1 commit into from
Jul 11, 2022

Conversation

vabruzzo
Copy link
Contributor

@vabruzzo vabruzzo commented Jul 11, 2022

Overview

This will remove the focus state in the nav on app load. According to the following issue in the w3c css working group, the focus-visible state on page load is expected behavior: w3c/csswg-drafts#5885 (i think, this discussion is not always easy to follow)
This is because on page load the app is programmatically moving focus to this element via react router and for accessibility reasons if focus is moved programmatically when no prior element is focused (such as on page load) the focus-visible selector is active. At least that's how I understand the discussion there.

We can keep the focus-visible on load, or implement some solution such as I've done here, which is to remove the focus-visible from the currently active item. A better fix would be to prevent react router from focusing on the nav element but i havent found a way to do that yet.

Changelog

  • removes focus-visible from active nav item

Review requests

Check that on app load nav item is not in focus-visible state

Risk assessment

Low

@vabruzzo vabruzzo requested a review from a team as a code owner July 11, 2022 13:29
@vabruzzo vabruzzo requested review from mcous and removed request for a team July 11, 2022 13:29
@codecov
Copy link

codecov bot commented Jul 11, 2022

Codecov Report

Merging #11068 (e48f6eb) into release_6.0.0 (61feb78) will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@                Coverage Diff                @@
##           release_6.0.0   #11068      +/-   ##
=================================================
+ Coverage          73.78%   73.79%   +0.01%     
=================================================
  Files               2082     2084       +2     
  Lines              57535    58089     +554     
  Branches            5806     6003     +197     
=================================================
+ Hits               42452    42867     +415     
- Misses             13800    13910     +110     
- Partials            1283     1312      +29     
Flag Coverage Δ
app 71.20% <ø> (+0.11%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
app/src/App/Navbar.tsx 100.00% <ø> (ø)
app/src/atoms/MenuList/DropdownMenu.tsx 94.11% <0.00%> (-5.89%) ⬇️
app/src/organisms/ChooseProtocolSlideout/index.tsx 65.71% <0.00%> (-4.29%) ⬇️
app/src/molecules/DeckThumbnail/index.tsx 84.61% <0.00%> (-3.62%) ⬇️
...ganisms/Devices/ProtocolRun/LabwareInfoOverlay.tsx 84.61% <0.00%> (-2.06%) ⬇️
app/src/organisms/ModuleCard/TestShakeSlideout.tsx 75.00% <0.00%> (-1.20%) ⬇️
...src/organisms/Devices/ProtocolRun/SetupModules.tsx 85.18% <0.00%> (-1.03%) ⬇️
app/src/organisms/LabwareDetails/index.tsx 2.70% <0.00%> (-1.01%) ⬇️
...src/organisms/Devices/ProtocolRun/SetupLabware.tsx 89.07% <0.00%> (-0.93%) ⬇️
app/src/organisms/Devices/RobotOverflowMenu.tsx 86.20% <0.00%> (-0.89%) ⬇️
... and 22 more

@vabruzzo vabruzzo requested review from koji, b-cooper, brenthagen and jerader and removed request for mcous July 11, 2022 13:34
Copy link
Collaborator

@jerader jerader left a comment

Choose a reason for hiding this comment

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

Does this also remove the ability to focus on the current Navbar selection? (Protocols doesn't have a focus-visible state now)

Screen.Recording.2022-07-11.at.9.36.31.AM.mov

@vabruzzo
Copy link
Contributor Author

vabruzzo commented Jul 11, 2022

Does this also remove the ability to focus on the current Navbar selection? (Protocols doesn't have a focus-visible state now)

Screen.Recording.2022-07-11.at.9.36.31.AM.mov

yeah it does unfortunately. i havent been able to come up with an actual fix.

@vabruzzo
Copy link
Contributor Author

Does this also remove the ability to focus on the current Navbar selection? (Protocols doesn't have a focus-visible state now)
Screen.Recording.2022-07-11.at.9.36.31.AM.mov

yeah it does unfortunately. i havent been able to come up with an actual fix.

Correction, it doesnt remove the ability to focus on the current item, it removes the focus state when one does focus on the current item. this might be an acceptable tradeoff since there really isnt much of a reason to click on the currently active tab

Copy link
Contributor

@b-cooper b-cooper left a comment

Choose a reason for hiding this comment

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

This is an acceptable compromise for now. Thanks for investigating!

@vabruzzo vabruzzo merged commit ace77fd into release_6.0.0 Jul 11, 2022
@vabruzzo vabruzzo deleted the app_nav-focus-on-startup-bug branch August 1, 2022 14:04
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