-
Notifications
You must be signed in to change notification settings - Fork 13
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
Improve hybrid form UX #248
Conversation
Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
… ema/improve-hybrid-form-ux Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com> # Conflicts: # dist/autofill-debug.js # dist/autofill.js # src/Form/FormAnalyzer.js # swift-package/Resources/assets/autofill-debug.js # swift-package/Resources/assets/autofill.js
Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
… ema/improve-hybrid-form-ux 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>
Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
this.decreaseSignalBy(strength + 2, `Unified detected ${signalType}`) | ||
if (shouldCheckUnifiedForm && matchesLogin && strictSignupRegex.test(string)) { | ||
this.signals.push(`hybrid form: ${signalType}`) | ||
this.isHybrid = true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we detect that it's a hybrid form, short circuit things. Actually, I'm wondering if we should also stop scanning the form at this stage 🤔. Feedback welcome, but as things stand now once this is set we never un-set it so the rest of the form scanning doesn't have any effect.
Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
input[type=email], | ||
input[type=text][aria-label*=email i]:not([aria-label*=search i]), | ||
input:not([type])[aria-label*=email i]:not([aria-label*=search i]), | ||
input[type=text][placeholder*=email i]:not([placeholder*=search i]):not([placeholder*=filter i]):not([placeholder*=subject i]), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These were duplicates 🤦♂️. Previously we discussed about generating these instead of having them hardcoded like this. It could give us easier access to variations, especially on other selectors where we use hyphens, underscores etc. For another day.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added some questions and non-blocking comments. LGTM and the test build works really well
}, | ||
iconMatchers: { | ||
key: 'data:image/svg+xml;base64,PD94bWwgdmVyc2lvbj0iMS4w', | ||
dax: 'data:image/svg+xml;base64,PHN2ZyBmaWxsPSJub25lIiBo' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❓ Is it worth importing the actual string here than duplicating part of it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I tried that I think that there was a build error because the tests use require
and the rest of the code uses import
. I couldn't figure it out quickly and just moved on. If you or @shakyShane have suggestions, please let me know and I'll be happy to improve it, otherwise I'd leave it be.
Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
Signed-off-by: Emanuele Feliziani <feliziani.emanuele@gmail.com>
password: {match: 'password', forceUnknown: 'captcha|mfa|2fa|two factor'}, | ||
username: {match: '(user|account|apple|login|net)((.)?(name|id|login).?)?(.?(or|/).+)?$|benutzername', forceUnknown: 'search|policy'}, | ||
email: {match: '.mail\\b|apple.?id', skip: 'phone|name|reservation number|code', forceUnknown: 'search|filter|subject|title|\btab\b'}, | ||
password: {match: 'password', skip: 'email', forceUnknown: 'captcha|mfa|2fa|two factor'}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added this to fix a bug on substack reported just yesterday 💪. Test case added.
Reviewer: @shakyShane @alistairjcbrown
Asana: https://app.asana.com/0/0/1203752198387942/f
Description
Tweaks the flow to show identities autofill when an hybrid or login form doesn't have saved credentials. See this matrix to understand what we're changing:
This also fixes a few other bugs. Full list of fixes in Asana.
Steps to test
Automated tests added, but there's also a comprehensive list of bugs this is fixing. You can find it in the Asana task. There you can also find the macOS build.