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

feat: remove click handler setting onTabContentFocusIn #1263

Merged
merged 2 commits into from
May 5, 2023

Conversation

dsmmcken
Copy link
Contributor

@dsmmcken dsmmcken commented May 4, 2023

OnTabContentFocusIn is used to set a class on the tab to highlight which tab currently has focus so the user knows where any keyboard input or shortcuts used will be applied. This was being set by both focusin and any clicks inside the tab, with clicks triggering an addtional call to focus.

Both handlers were commited at the same time, but I can't think of any reason we were actually setting the focus on click. The click itsellf should do that. My best guess is tabindex=-1 wasn't on the parent at the time (during development as it was also added in the same commit) or maybe browser support at the time? It all came in under one commit.

I have tested in chrome (win & mac), firefox (win) and safari (mac) and clicking on or anywher within any panel (tables, plots, one clicks, console, notebooks, log panel, command history, file exporer) all still set the focus as expected.

Additionally I addressed a firefox edge case around focus and an item being removed from the dom.

OnTabContentFocusIn is used to set a class on the tab to highlight which tab currently has focus so the user knows where any keyboard input or shortcuts used will be applied. This was being set by both focusin and any clicks inside the tab, with clicks triggering an addtional call to focus.

Both handlers were commited at the same time, but I can't think of any reason we were actually setting the focus on click. The click itsellf should do that. My best guess is tabindex=-1 wasn't on the parent at the time (during development as it was also added in the same commit) or maybe browser support at the time? It all came in under one commit.

I have tested in chrome (win & mac), firefox (win) and safari (mac) and clicking on or anywher within any panel (tables, plots, one clicks, console, notebooks, log panel, command history, file exporer) all still set the focus as expected.

Additionally I addressed a firefox edge case around focus and an item being removed from the dom.
@dsmmcken dsmmcken self-assigned this May 4, 2023
@dsmmcken dsmmcken requested review from bmingles and mofojed May 4, 2023 19:55
@codecov
Copy link

codecov bot commented May 4, 2023

Codecov Report

Merging #1263 (67281f2) into main (1b29af1) will decrease coverage by 0.21%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##             main    #1263      +/-   ##
==========================================
- Coverage   45.57%   45.37%   -0.21%     
==========================================
  Files         486      502      +16     
  Lines       34101    34666     +565     
  Branches     8538     8675     +137     
==========================================
+ Hits        15543    15728     +185     
- Misses      18507    18887     +380     
  Partials       51       51              
Flag Coverage Δ
unit 45.37% <ø> (-0.21%) ⬇️

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

see 35 files with indirect coverage changes

bmingles
bmingles previously approved these changes May 4, 2023
Copy link
Contributor

@bmingles bmingles left a comment

Choose a reason for hiding this comment

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

Changes look good to me

@dsmmcken dsmmcken merged commit 7d56f97 into main May 5, 2023
@dsmmcken dsmmcken deleted the dmckenzie_focus branch May 5, 2023 13:05
@github-actions github-actions bot locked and limited conversation to collaborators May 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants