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

[Credentials Import] Add "Don't Show Again" button to import prompt #676

Merged
merged 13 commits into from
Oct 14, 2024
35 changes: 30 additions & 5 deletions dist/autofill-debug.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion dist/autofill.css

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

34 changes: 29 additions & 5 deletions dist/autofill.js

Large diffs are not rendered by default.

6 changes: 4 additions & 2 deletions integration-test/helpers/mocks.webkit.js
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,8 @@ export const macosWithoutOverlay = () => {
'setIncontextSignupPermanentlyDismissedAt',
'startEmailProtectionSignup',
'closeEmailProtectionTab',
'startCredentialsImportFlow'
'startCredentialsImportFlow',
'credentialsImportFlowPermanentlyDismissed'
]
}
}
Expand Down Expand Up @@ -265,7 +266,8 @@ export function createWebkitMocks (platform = 'macos') {
setIncontextSignupPermanentlyDismissedAt: null,
startEmailProtectionSignup: null,
closeEmailProtectionTab: null,
startCredentialsImportFlow: {}
startCredentialsImportFlow: {},
credentialsImportFlowPermanentlyDismissed: null
}

/** @type {MockBuilder<any, webkitBase>} */
Expand Down
30 changes: 30 additions & 0 deletions integration-test/tests/credentials-import.macos.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -115,5 +115,35 @@ test.describe('Import credentials prompt', () => {
await expect(webkitCalls.length).toBeGreaterThanOrEqual(1)
await overlay.assertCloseAutofillParent()
})

test('when dismiss prompt is clicked, native API call is made', async ({ page }) => {
await forwardConsoleMessages(page)
await createWebkitMocks()
.withAvailableInputTypes(createAvailableInputTypes({
credentials: {
username: false,
password: false
},
credentialsImport: true
}))
.withCredentialsImport?.('credentials.username')
.applyTo(page)

// Pretend we're running in a top-frame scenario
await createAutofillScript()
.replaceAll(macosContentScopeReplacements())
.replace('isTopFrame', true)
.replace('supportsTopFrame', true)
.platform('macos')
.applyTo(page)

const overlay = overlayPage(page)
await overlay.navigate()
await overlay.clickButtonWithText("Don't Show Again")

const webkitCalls = await mockedCalls(page, { names: ['credentialsImportFlowPermanentlyDismissed'], minCount: 1 })
await expect(webkitCalls.length).toBeGreaterThanOrEqual(1)
await overlay.assertCloseAutofillParent()
})
})
})
7 changes: 6 additions & 1 deletion src/CredentialsImport.js
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { CloseAutofillParentCall, StartCredentialsImportFlowCall } from './deviceApiCalls/__generated__/deviceApiCalls.js'
import { CloseAutofillParentCall, CredentialsImportFlowPermanentlyDismissedCall, StartCredentialsImportFlowCall } from './deviceApiCalls/__generated__/deviceApiCalls.js'

/**
* Use this as place to store any state or functionality related to password import promotion
Expand Down Expand Up @@ -58,6 +58,11 @@ class CredentialsImport {
this.device.deviceApi.notify(new CloseAutofillParentCall(null))
this.device.deviceApi.notify(new StartCredentialsImportFlowCall({}))
}

async dismissed () {
this.device.deviceApi.notify(new CloseAutofillParentCall(null))
this.device.deviceApi.notify(new CredentialsImportFlowPermanentlyDismissedCall(null))
}
Copy link
Member

Choose a reason for hiding this comment

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

Just a thought. Here we call close, then the other call (same as we do above in line 58). Is there a race condition where the first call's side effect is so quick that the second call never happens because then the webview gets closed and the js context is destroyed? Should we flip their order?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That's a good point! I think it's safer to do flipped order. Changed it in 79c1941

}

export { CredentialsImport }
14 changes: 12 additions & 2 deletions src/UI/CredentialsImportTooltip.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import HTMLTooltip from './HTMLTooltip.js'
class CredentialsImportTooltip extends HTMLTooltip {
/**
* @param {import("../DeviceInterface/InterfacePrototype.js").default} device
* @param {{ onStarted(): void }} callbacks
* @param {{ onStarted(): void, onDismissed(): void }} callbacks
*/
render (device, callbacks) {
this.device = device
Expand All @@ -19,16 +19,26 @@ ${this.options.css}
<span class="label label--small">${t('autofill:credentialsImportText')}</span>
</span>
</button>
<hr />
<button class="tooltip__button tooltip__button--secondary js-dismiss">
<span class="tooltip__button__text-container">
<span class="label label--medium">${t('autofill:dontShowAgain')}</span>
</span>
</button>
</div>
</div>
`

this.tooltip = this.shadow.querySelector('.tooltip')

this.buttonWrapper = this.shadow.querySelector('.js-promo-wrapper')
this.dismissWrapper = this.shadow.querySelector('.js-dismiss')

this.registerClickableButton(this.buttonWrapper, () => {
callbacks.onStarted()
})
this.registerClickableButton(this.dismissWrapper, () => {
callbacks.onDismissed()
})

this.init()
return this
Expand Down
2 changes: 1 addition & 1 deletion src/UI/DataHTMLTooltip.js
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ ${css}
: ''}
${shouldShowManageButton ? `
<hr />
<button id="manage-button" class="tooltip__button tooltip__button--manage" type="button">
<button id="manage-button" class="tooltip__button tooltip__button--secondary" type="button">
<span class="tooltip__button__text-container">
<span class="label label--medium">
${t(manageItemStringIds[config.type] ?? 'autofill:manageSavedItems')}
Expand Down
3 changes: 3 additions & 0 deletions src/UI/controllers/HTMLTooltipUIController.js
Original file line number Diff line number Diff line change
Expand Up @@ -116,6 +116,9 @@ export class HTMLTooltipUIController extends UIController {
.render(this._options.device, {
onStarted: () => {
this._options.device.credentialsImport.started()
},
onDismissed: () => {
this._options.device.credentialsImport.dismissed()
}
})
}
Expand Down
2 changes: 1 addition & 1 deletion src/UI/styles/autofill-tooltip-styles.css
Original file line number Diff line number Diff line change
Expand Up @@ -215,7 +215,7 @@
color: #FFFFFF;
}

.tooltip__button--manage {
.tooltip__button--secondary {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Just renaming it to sound more "reusable", just because we have the same scenario (manage password + don't show again) kind of like a secondary button.

Copy link
Member

Choose a reason for hiding this comment

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

Nice.

font-size: 13px;
padding: 5px 9px;
border-radius: 3px;
Expand Down
6 changes: 6 additions & 0 deletions src/deviceApiCalls/__generated__/deviceApiCalls.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 6 additions & 0 deletions src/deviceApiCalls/__generated__/validators-ts.ts

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions src/deviceApiCalls/__generated__/validators.zod.js

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 4 additions & 0 deletions src/deviceApiCalls/api.json
Original file line number Diff line number Diff line change
Expand Up @@ -148,6 +148,10 @@
"type": "object",
"description": "(macOS/Windows) Opens the native password import flow UI"
},
"credentialsImportFlowPermanentlyDismissed": {
"type": "object",
"description": "(macOS/Windows) User clicked on the password import flow prompt"
},
"emailProtectionGetAlias": {
"type": "object",
"description": "Used to prompt email protection autofill on mobile.",
Expand Down
35 changes: 30 additions & 5 deletions swift-package/Resources/assets/autofill-debug.js

Large diffs are not rendered by default.

2 changes: 1 addition & 1 deletion swift-package/Resources/assets/autofill.css

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

34 changes: 29 additions & 5 deletions swift-package/Resources/assets/autofill.js

Large diffs are not rendered by default.

Loading