clients icon indicating copy to clipboard operation
clients copied to clipboard

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

Open patrickhlauke opened this issue 1 year ago • 3 comments

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) https://github.com/bitwarden/clients/pull/2662 / https://github.com/bitwarden/clients/pull/2654

Code changes

  • removed appBlurClick throughout the entirety of the codebase (too many files to list explicitly here)

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)

patrickhlauke avatar Jul 31 '22 11:07 patrickhlauke

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

bitwarden-bot avatar Jul 31 '22 11:07 bitwarden-bot

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 avatar Jul 31 '22 11:07 patrickhlauke

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 avatar Aug 06 '22 00:08 patrickhlauke

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)

patrickhlauke avatar Aug 16 '22 20:08 patrickhlauke

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

djsmith85 avatar Aug 16 '22 20:08 djsmith85

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

patrickhlauke avatar Aug 16 '22 20:08 patrickhlauke

@djsmith85 done I think

patrickhlauke avatar Aug 16 '22 22:08 patrickhlauke

@djsmith85 done I think

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

djsmith85 avatar Aug 16 '22 22:08 djsmith85

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

patrickhlauke avatar Aug 16 '22 23:08 patrickhlauke

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

patrickhlauke avatar Aug 17 '22 08:08 patrickhlauke

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 😉

djsmith85 avatar Aug 17 '22 09:08 djsmith85