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 minor regression #320

Merged
merged 5 commits into from
May 16, 2023
Merged

Fix minor regression #320

merged 5 commits into from
May 16, 2023

Conversation

GioSensation
Copy link
Member

@GioSensation GioSensation commented May 16, 2023

Reviewer: @alistairjcbrown
Asana: https://app.asana.com/0/0/1204595906186359/f

Description

This fixes a minor use case of tabbing from field to field when the form overflows the viewport.

Steps to test

Added test case and verified on device. Steps to test:

  1. Visit https://www.roboform.com/filling-test-all-fields and make sure you have an identity with all data types
  2. Shrink your window so that some fields are below the fold
  3. Tab from field to field
  4. The autofill pulldown should open when tabbing to fields below the fold

CleanShot 2023-05-16 at 10 28 51

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
@GioSensation GioSensation self-assigned this May 16, 2023
setTimeout(() => {
this.attach(args)
}, 50)
return
Copy link
Member Author

Choose a reason for hiding this comment

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

Instead of having the side-effect happen after a timeout, now we are calling this same function recursively after the timeout. This ensures that this.#state is consistent across calls. The 500 delay was likely arbitrary and 50 works just as well, so shorter is better.

Copy link
Member

Choose a reason for hiding this comment

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

@GioSensation NAB: Do we want to provide a comment for context on the 50 value to help future us? (i.e. it's just some value to throttle repeated calls)

Copy link
Member Author

Choose a reason for hiding this comment

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

I've added a comment above to explain why we're recursing, plus that the 50 is arbitrary.

@GioSensation GioSensation marked this pull request as ready for review May 16, 2023 09:07
Copy link
Member

@alistairjcbrown alistairjcbrown left a comment

Choose a reason for hiding this comment

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

LGTM :shipit:

setTimeout(() => {
this.attach(args)
}, 50)
return
Copy link
Member

Choose a reason for hiding this comment

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

@GioSensation NAB: Do we want to provide a comment for context on the 50 value to help future us? (i.e. it's just some value to throttle repeated calls)

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
… ema/fix-focus-regression

Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>

# Conflicts:
#	dist/autofill-debug.js
#	dist/autofill.js
#	swift-package/Resources/assets/autofill-debug.js
#	swift-package/Resources/assets/autofill.js
Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
@GioSensation GioSensation merged commit 1f4abc1 into main May 16, 2023
@GioSensation GioSensation deleted the ema/fix-focus-regression branch May 16, 2023 10:59
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.

2 participants