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

[PS-1222] Remove appBlurClick throughout the popup and web code #3208

Conversation

patrickhlauke
Copy link
Contributor

@patrickhlauke patrickhlauke commented Jul 31, 2022

Type of change

- [ ] Bug fix
- [ ] New feature development
- [x] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

appBlurClick leads to focus being lost/reset for assistive technology users. It should not be necessary in any case - if focus does need to move after an action, explicitly set it somewhere programmatically using focus() rather than relying on browser heuristics. in cases where the view completely changes as a result of an action, focus is reset automatically anyway.

this is a follow-up to similar previous PRs (both in the browser extension and desktop app) #2662 / #2654

Code changes

  • removed appBlurClick throughout the entirety of the codebase (too many files to list explicitly here)
  • removed libs/angular/src/directives/blur-click.directive.ts and the related entry in libs/angular/src/jslib.module.ts (as appBlurClick is now not used anywhere in the code base)

Screenshots

No discernible change in UI, nor - from my testing - in behaviour.

Before you submit

- [x] I have checked for **linting** errors (`npm run lint`) (required)
- [ ] I have added **unit tests** where it makes sense to do so (encouraged but not required)
- [ ] This change requires a **documentation update** (notify the documentation team)
- [ ] This change has particular **deployment requirements** (notify the DevOps team)

`appBlurClick` leads to focus being lost/reset for assistive technology users. It should not be necessary in any case - if focus does need to move after an action, explicitly set it somewhere programmatically using `focus()` rather than relying on browser heuristics
@bitwarden-bot
Copy link

Thank you for your contribution! We've added this to our internal Community PR board for review.
ID: PS-1222

@bitwarden-bot bitwarden-bot changed the title Remove appBlurClick throughout the popup code [PS-1222] Remove appBlurClick throughout the popup code Jul 31, 2022
@patrickhlauke
Copy link
Contributor Author

patrickhlauke commented Jul 31, 2022

Recommend thoroughly testing all functionality of the popup and web after this change. From my own testing, I have not noticed any negative behaviours.

@patrickhlauke
Copy link
Contributor Author

After merging master branch back into this, it now seems to only be touching on two straggler appBlurClicks ... seems the other files have already been fixed/have removed it, which is nice to see

@patrickhlauke patrickhlauke changed the title [PS-1222] Remove appBlurClick throughout the popup code [PS-1222] Remove appBlurClick throughout the popup and web code Aug 6, 2022
Copy link
Contributor

@djsmith85 djsmith85 left a comment

Choose a reason for hiding this comment

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

Thank you @patrickhlauke
This is looking good and removes the last instances of appBlurClick

As we are no longer using it, we should also remove the directive

@patrickhlauke
Copy link
Contributor Author

As we are no longer using it, we should also remove the directive

Is it enough to remove that file, or are there deeper ties anywhere else for it? (I'll have a little search, as sadly I'm still not really an angular guy, I just poke it from the side and see what happens)

@djsmith85
Copy link
Contributor

As we are no longer using it, we should also remove the directive

Is it enough to remove that file, or are there deeper ties anywhere else for it? (I'll have a little search, as sadly I'm still not really an angular guy, I just poke it from the side and see what happens)

After removing the file with the directive, the registration of that directive needs to be removed from jslib.module.ts

@patrickhlauke
Copy link
Contributor Author

@djsmith85 okidoki, i'll give it a go and add it to this PR

@patrickhlauke
Copy link
Contributor Author

@djsmith85 done I think

@djsmith85
Copy link
Contributor

@djsmith85 done I think

Almost..., you've removed the import for the directive but it's still used in jslib.module.ts (twice)

@patrickhlauke patrickhlauke force-pushed the patrickhlauke-remove-popup-appblurclick branch from fc9576d to a083143 Compare August 16, 2022 23:00
@patrickhlauke
Copy link
Contributor Author

serves me right for not testing...should be fixed now

@patrickhlauke patrickhlauke force-pushed the patrickhlauke-remove-popup-appblurclick branch from a083143 to a598eb4 Compare August 16, 2022 23:52
Copy link
Contributor

@djsmith85 djsmith85 left a comment

Choose a reason for hiding this comment

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

No worries, it's looking good now.

@patrickhlauke Thank you for your work on this 👍

@djsmith85 djsmith85 merged commit babfa30 into bitwarden:master Aug 17, 2022
@patrickhlauke patrickhlauke deleted the patrickhlauke-remove-popup-appblurclick branch August 17, 2022 08:49
@patrickhlauke
Copy link
Contributor Author

wunderbar, thank you @djsmith85 (and sorry for the flood of PRs ... was clearly on a roll/bored recently)

@djsmith85
Copy link
Contributor

No problem at all and every contribution is appreciated. Keep up the great work, seems you'll be leading community efforts again for 2022.09 😉

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.

3 participants