-
Notifications
You must be signed in to change notification settings - Fork 11
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
Changes from 13 commits
71bc2d4
332b981
62f0775
2e42c64
ce44870
d7cd42e
cd4b7e7
59ec0c1
f175489
57416c8
3fd6200
09fc6e9
65759ae
740ca7b
5c64f07
aa286c1
10e1780
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,12 +3,12 @@ import { constants } from '../constants.js' | |
import { matchingConfiguration } from './matching-configuration.js' | ||
import { getText, isLikelyASubmitButton } from '../autofill-utils.js' | ||
|
||
const negativeRegex = new RegExp(/sign(ing)?.?in(?!g)|log.?in|unsubscri|(forgot(ten)?|reset) (your )?password|password (forgotten|lost)/i) | ||
const positiveRegex = new RegExp( | ||
const loginRegex = new RegExp(/sign(ing)?.?in(?!g)|log.?in|unsubscri|(forgot(ten)?|reset) (your )?password|password (forgotten|lost)/i) | ||
const signupRegex = new RegExp( | ||
GioSensation marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/sign(ing)?.?up|join|\bregist(er|ration)|newsletter|\bsubscri(be|ption)|contact|create|start|enroll|settings|preferences|profile|update|checkout|guest|purchase|buy|order|schedule|estimate|request|new.?customer|(confirm|retype|repeat) password|password confirm?/i | ||
) | ||
const conservativePositiveRegex = new RegExp(/sign.?up|join|register|enroll|newsletter|subscri(be|ption)|settings|preferences|profile|update/i) | ||
const strictPositiveRegex = new RegExp(/sign.?up|join|register|enroll|settings|preferences|profile|update/i) | ||
const conservativeSignupRegex = new RegExp(/sign.?up|join|register|enroll|newsletter|subscri(be|ption)|settings|preferences|profile|update/i) | ||
const strictSignupRegex = new RegExp(/sign.?up|join|register|(create|new).+account|enroll|settings|preferences|profile|update/i) | ||
|
||
class FormAnalyzer { | ||
/** @type HTMLElement */ | ||
|
@@ -23,9 +23,23 @@ class FormAnalyzer { | |
constructor (form, input, matching) { | ||
this.form = form | ||
this.matching = matching || new Matching(matchingConfiguration) | ||
/** | ||
* The signal is a continuum where negative values imply login and positive imply signup | ||
* @type {number} | ||
*/ | ||
this.autofillSignal = 0 | ||
/** | ||
* Collects the signals for debugging purposes | ||
* @type {string[]} | ||
*/ | ||
this.signals = [] | ||
|
||
/** | ||
* A hybrid form can be either a login or a signup, the site uses a single form for both | ||
* @type {boolean} | ||
*/ | ||
this.isHybrid = false | ||
|
||
// Avoid autofill on our signup page | ||
if (window.location.href.match(/^https:\/\/(.+\.)?duckduckgo\.com\/email\/choose-address/i)) { | ||
return this | ||
|
@@ -37,27 +51,43 @@ class FormAnalyzer { | |
} | ||
|
||
get isLogin () { | ||
if (this.isHybrid) return false | ||
|
||
return this.autofillSignal < 0 | ||
} | ||
|
||
get isSignup () { | ||
if (this.isHybrid) return false | ||
|
||
return this.autofillSignal >= 0 | ||
} | ||
|
||
/** | ||
* Tilts the scoring towards Signup | ||
* @param {number} strength | ||
* @param {string} signal | ||
* @returns {FormAnalyzer} | ||
*/ | ||
increaseSignalBy (strength, signal) { | ||
this.autofillSignal += strength | ||
this.signals.push(`${signal}: +${strength}`) | ||
return this | ||
} | ||
|
||
/** | ||
* Tilts the scoring towards Login | ||
* @param {number} strength | ||
* @param {string} signal | ||
* @returns {FormAnalyzer} | ||
*/ | ||
GioSensation marked this conversation as resolved.
Show resolved
Hide resolved
|
||
decreaseSignalBy (strength, signal) { | ||
this.autofillSignal -= strength | ||
this.signals.push(`${signal}: -${strength}`) | ||
return this | ||
} | ||
|
||
/** | ||
* | ||
* Updates the Login<->Signup signal according to the provided parameters | ||
* @param {object} p | ||
* @param {string} p.string - The string to check | ||
* @param {number} p.strength - Strength of the signal | ||
|
@@ -75,24 +105,25 @@ class FormAnalyzer { | |
shouldCheckUnifiedForm = false, | ||
shouldBeConservative = false | ||
}) { | ||
const matchesNegative = string === 'current-password' || negativeRegex.test(string) | ||
const matchesLogin = string === 'current-password' || loginRegex.test(string) | ||
|
||
// Check explicitly for unified login/signup forms. They should always be negative, so we increase signal | ||
if (shouldCheckUnifiedForm && matchesNegative && strictPositiveRegex.test(string)) { | ||
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 commentThe 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. |
||
return this | ||
} | ||
|
||
const positiveRegexToUse = shouldBeConservative ? conservativePositiveRegex : positiveRegex | ||
const matchesPositive = string === 'new-password' || positiveRegexToUse.test(string) | ||
const signupRegexToUse = shouldBeConservative ? conservativeSignupRegex : signupRegex | ||
const matchesSignup = string === 'new-password' || signupRegexToUse.test(string) | ||
|
||
// In some cases a login match means the login is somewhere else, i.e. when a link points outside | ||
if (shouldFlip) { | ||
if (matchesNegative) this.increaseSignalBy(strength, signalType) | ||
if (matchesPositive) this.decreaseSignalBy(strength, signalType) | ||
if (matchesLogin) this.increaseSignalBy(strength, signalType) | ||
if (matchesSignup) this.decreaseSignalBy(strength, signalType) | ||
} else { | ||
if (matchesNegative) this.decreaseSignalBy(strength, signalType) | ||
if (matchesPositive) this.increaseSignalBy(strength, signalType) | ||
if (matchesLogin) this.decreaseSignalBy(strength, signalType) | ||
if (matchesSignup) this.increaseSignalBy(strength, signalType) | ||
} | ||
return this | ||
} | ||
|
@@ -113,23 +144,21 @@ class FormAnalyzer { | |
|
||
evaluatePageTitle () { | ||
const pageTitle = document.title | ||
this.updateSignal({string: pageTitle, strength: 2, signalType: `page title: ${pageTitle}`}) | ||
this.updateSignal({string: pageTitle, strength: 2, signalType: `page title: ${pageTitle}`, shouldCheckUnifiedForm: true}) | ||
GioSensation marked this conversation as resolved.
Show resolved
Hide resolved
|
||
} | ||
|
||
evaluatePageHeadings () { | ||
const headings = document.querySelectorAll('h1, h2, h3, [class*="title"], [id*="title"]') | ||
if (headings) { | ||
headings.forEach(({textContent}) => { | ||
textContent = removeExcessWhitespace(textContent || '') | ||
this.updateSignal({ | ||
string: textContent, | ||
strength: 0.5, | ||
signalType: `heading: ${textContent}`, | ||
shouldCheckUnifiedForm: true, | ||
shouldBeConservative: true | ||
}) | ||
headings.forEach(({textContent}) => { | ||
textContent = removeExcessWhitespace(textContent || '') | ||
this.updateSignal({ | ||
string: textContent, | ||
strength: 0.5, | ||
signalType: `heading: ${textContent}`, | ||
shouldCheckUnifiedForm: true, | ||
shouldBeConservative: true | ||
}) | ||
} | ||
}) | ||
} | ||
|
||
evaluatePage () { | ||
|
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 usesimport
. 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.