-
Notifications
You must be signed in to change notification settings - Fork 693
feat(pass-extension): add autofill keyboard shortcut command #481
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
base: main
Are you sure you want to change the base?
Changes from all commits
e178fcb
e564905
49e4a80
2d42985
e833567
e2560a4
d639fe2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,6 +29,7 @@ export const DROPDOWN_FOCUS_TRAP_TIMEOUT = 500; | |
| export interface DropdownFocusController { | ||
| focused: boolean; | ||
| willFocus: boolean; | ||
| requestFocus: () => Promise<void>; | ||
| disconnect: () => void; | ||
| } | ||
|
|
||
|
|
@@ -92,7 +93,12 @@ export const createDropdownFocusController = ({ | |
| }; | ||
|
|
||
| const onWillFocus = () => { | ||
| if (anchor.current?.type === 'field') anchor.current.field.preventAction(); | ||
| /** Only arm the field action-trap when the anchor field is the active element — i.e. the | ||
| * focus-recovery scenario. The shortcut flow opens the dropdown without focusing the field, | ||
| * so arming it there would needlessly suppress the field's normal autofocus dropdown. */ | ||
| if (anchor.current?.type === 'field' && isActiveElement(anchor.current.field.element)) { | ||
|
Contributor
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. For your use-case : could we rather drill a boolean flag down |
||
| anchor.current.field.preventAction(); | ||
| } | ||
| clearTimeout(state.willFocusTimer); | ||
| state.willFocus = true; | ||
| state.willFocusTimer = setTimeout(disconnect, DROPDOWN_FOCUS_TRAP_TIMEOUT); | ||
|
|
@@ -165,6 +171,7 @@ export const createDropdownFocusController = ({ | |
| get willFocus() { | ||
| return state.willFocus; | ||
| }, | ||
| requestFocus: onFocusRequest, | ||
| disconnect, | ||
| }; | ||
| }; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| import { handleExtensionCommand } from './commands'; | ||
| import browser from '@proton/pass/lib/globals/browser'; | ||
| import { WorkerMessageType } from 'proton-pass-extension/types/messages'; | ||
|
|
||
| jest.mock('@proton/pass/lib/globals/browser', () => ({ | ||
| tabs: { | ||
|
Contributor
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. You can use (and/or augment) the |
||
| create: jest.fn(() => Promise.resolve()), | ||
| query: jest.fn(() => Promise.resolve([{ id: 42 }])), | ||
| sendMessage: jest.fn(() => Promise.resolve()), | ||
| }, | ||
| runtime: { | ||
| getURL: jest.fn((path: string) => `chrome-extension://abc/${path}`), | ||
| }, | ||
| commands: { | ||
| getAll: jest.fn(() => Promise.resolve([])), | ||
| }, | ||
| })); | ||
|
|
||
| describe('handleExtensionCommand', () => { | ||
| beforeEach(() => jest.clearAllMocks()); | ||
|
|
||
| it('should open larger window for open-larger-window command', async () => { | ||
| await handleExtensionCommand('open-larger-window'); | ||
| expect(browser.tabs.create).toHaveBeenCalledWith({ | ||
| url: 'chrome-extension://abc/popup.html#', | ||
| }); | ||
| }); | ||
|
|
||
| it('should send AUTOFILL_TRIGGER to active tab for autofill command', async () => { | ||
| await handleExtensionCommand('autofill'); | ||
| expect(browser.tabs.query).toHaveBeenCalledWith({ active: true, currentWindow: true }); | ||
| expect(browser.tabs.sendMessage).toHaveBeenCalledWith( | ||
| 42, | ||
| expect.objectContaining({ type: WorkerMessageType.AUTOFILL_TRIGGER }) | ||
| ); | ||
| }); | ||
|
|
||
| it('should send message when tab id is 0', async () => { | ||
| (browser.tabs.query as jest.Mock).mockResolvedValueOnce([{ id: 0 }]); | ||
| await handleExtensionCommand('autofill'); | ||
| expect(browser.tabs.sendMessage).toHaveBeenCalledWith( | ||
| 0, | ||
| expect.objectContaining({ type: WorkerMessageType.AUTOFILL_TRIGGER }) | ||
| ); | ||
| }); | ||
|
|
||
| it('should not send message when no active tab', async () => { | ||
| (browser.tabs.query as jest.Mock).mockResolvedValueOnce([]); | ||
| await handleExtensionCommand('autofill'); | ||
| expect(browser.tabs.sendMessage).not.toHaveBeenCalled(); | ||
| }); | ||
|
|
||
| it('should do nothing for unknown commands', async () => { | ||
| await handleExtensionCommand('unknown-command'); | ||
| expect(browser.tabs.create).not.toHaveBeenCalled(); | ||
| expect(browser.tabs.sendMessage).not.toHaveBeenCalled(); | ||
| }); | ||
| }); | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,9 @@ | ||
| import browser from '@proton/pass/lib/globals/browser'; | ||
| import noop from '@proton/utils/noop'; | ||
|
|
||
| import { backgroundMessage } from 'proton-pass-extension/lib/message/send-message'; | ||
| import { WorkerMessageType } from 'proton-pass-extension/types/messages'; | ||
|
|
||
| export type Shortcut = { name: string; description: string; shortcut: string }; | ||
|
|
||
| type BrowserCommand = Awaited<ReturnType<typeof browser.commands.getAll>>[number]; | ||
|
|
@@ -10,12 +13,19 @@ export const resolveShortcuts = (commands: BrowserCommand[], supported: Record<s | |
| .filter((cmd): cmd is BrowserCommand & { name: string } => Boolean(cmd.name && cmd.name in supported)) | ||
| .map(({ name, shortcut }) => ({ name, shortcut: shortcut ?? '', description: supported[name] })); | ||
|
|
||
| export const handleExtensionCommand = (command: string) => { | ||
| export const handleExtensionCommand = async (command: string) => { | ||
| if (command === 'open-larger-window') { | ||
| browser.tabs | ||
| .create({ | ||
| url: browser.runtime.getURL('popup.html#'), | ||
| }) | ||
| .catch(noop); | ||
| } | ||
|
|
||
| if (command === 'autofill') { | ||
| const [tab] = await browser.tabs.query({ active: true, currentWindow: true }); | ||
| if (tab?.id != null) { | ||
| browser.tabs.sendMessage(tab.id, backgroundMessage({ type: WorkerMessageType.AUTOFILL_TRIGGER })).catch(noop); | ||
|
Contributor
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. We recently added support for autofilling iframe fields as well (will be released in 1.38.0). This trigger will end up only broadcasting to the top-frame. We could iterate through all frames (starting from the top-level one) until we have a match and early exit here. It would require |
||
| } | ||
| } | ||
| }; | ||
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.
There is a small type error when running
yarn check-typesinsideapplications/pass-extension, this shouldn't beasync:Note that my suggestion may be changed if you implement #481 (comment)