Skip to content
This repository has been archived by the owner on Jun 17, 2022. It is now read-only.

Fix cursor location changing issue on toggle password #561

Merged
merged 6 commits into from
Nov 28, 2021

Conversation

chao813
Copy link
Contributor

@chao813 chao813 commented Nov 20, 2021

Type of change

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

Objective

When you toggle visibility of the master password, the cursor moves to the start of the input field. Noticed it does this on Chrome web vault and Chrome browser extension. I tried on Safari and can't reproduce this issue.

Code changes

#557 , also 2178
Updated togglePassword() function to work with Chrome browser. Added a setTimeout of 0 ms (to defer the operation until the stack is clear) on focus since Chrome has an odd quirk where the focus event fires before the cursor is moved into the field.

  • login.component.ts: Updated togglePassword function in login.component.ts

Testing requirements

Test toggle password visibility on all platforms.

Before you submit

  • 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)

@CLAassistant
Copy link

CLAassistant commented Nov 20, 2021

CLA assistant check
All committers have signed the CLA.

@cscharf cscharf requested a review from eliykat November 22, 2021 19:04
@chao813
Copy link
Contributor Author

chao813 commented Nov 24, 2021

@eliykat Just saw your comment on the issue I opened, should I move this PR into the browser repo or just leave this here for you to review?

@eliykat
Copy link
Member

eliykat commented Nov 24, 2021

It's fine here - this is the right place for the PR. Because jslib is shared by the clients, it's totally normal to have a PR in jslib that fixes an issue reported in a client.

@chao813
Copy link
Contributor Author

chao813 commented Nov 24, 2021

I figured, thanks!

Copy link
Member

@eliykat eliykat left a comment

Choose a reason for hiding this comment

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

Thanks again for the PR, please see my review comments below.

Once we have the changes settled here, it would be good to replicate them in lock.component.ts as well. There are many places in our app that we have a show/hide button like this, it would be good to encapsulate this into a re-usable component, but that's a task for another day. This would at least solve the problem on the 2 most common pages.

angular/src/components/login.component.ts Outdated Show resolved Hide resolved
angular/src/components/login.component.ts Outdated Show resolved Hide resolved
@chao813 chao813 requested a review from eliykat November 25, 2021 04:59
@eliykat
Copy link
Member

eliykat commented Nov 25, 2021

Does this change (waiting until ngZone is stable) fix the original bug, without having to use setSelectionRange? If so, that's great! I didn't actually realise that it would work that way 😅

Can you please replicate for lock.component.ts, then if it all looks good we can merge.

@chao813
Copy link
Contributor Author

chao813 commented Nov 25, 2021

@eliykat I believe it did, didn't need to use setSelectionRange.
I just replicated it in lock.component.ts as well.

angular/src/components/lock.component.ts Outdated Show resolved Hide resolved
@chao813 chao813 requested a review from eliykat November 26, 2021 04:52
Copy link
Member

@eliykat eliykat left a comment

Choose a reason for hiding this comment

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

Nicely done, thanks for the PR!
This will be in the January release.

@xSepticSidx
Copy link

Many thanks to all for sorting this issue out.

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.

None yet

4 participants