Skip to content

Conversation

jennifer-shehane
Copy link
Member

@jennifer-shehane jennifer-shehane commented Apr 11, 2025

Additional details

Include additional key support to cy.press. See https://github.com/cypress-io/cypress-documentation/blob/release/15.1.0/docs/api/commands/press.mdx for full list of keys

Steps to test

How has the user experience changed?

PR Tasks

Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (1)

packages/server/lib/automation/commands/key_press.ts:134

  • The code mapping for 'ArrowUp' uses 'U+0010', which is also used for 'Shift' in this mapping. Please verify against the CDP specification and assign a unique key code for 'ArrowUp'.
'ArrowUp': 'U+0010',

Copy link

cypress bot commented Apr 11, 2025

cypress    Run #65076

Run Properties:  status check passed Passed #65076  •  git commit fd8fd803b5: more updates
Project cypress
Branch Review feat-all-keys
Run status status check passed Passed #65076
Run duration 12m 55s
Commit git commit fd8fd803b5: more updates
Committer Cacie Prins
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 0
Tests that did not run due to a developer annotating a test with .skip  Pending 694
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 130
View all changes introduced in this branch ↗︎

@jennifer-shehane jennifer-shehane marked this pull request as draft April 15, 2025 02:52
@jennifer-shehane jennifer-shehane self-assigned this Apr 22, 2025
@brian-mann
Copy link
Member

brian-mann commented May 21, 2025

it seems like we aren't normalizing cy.press() to the same options/behavior/mechanics as cy.type() ... namely I can see in the code that we're always issuing the keydown + up events, instead of giving you the ability to "press and hold" a particular key down before releasing it later. Maybe that was intentional when we first supported only TAB but upon supporting other keys, you can see the need arise.

@cacieprins cacieprins self-assigned this Aug 6, 2025
@cacieprins cacieprins marked this pull request as ready for review August 6, 2025 15:02
@cacieprins
Copy link
Contributor

it seems like we aren't normalizing cy.press() to the same options/behavior/mechanics as cy.type() ... namely I can see in the code that we're always issuing the keydown + up events, instead of giving you the ability to "press and hold" a particular key down before releasing it later. Maybe that was intentional when we first supported only TAB but upon supporting other keys, you can see the need arise.

We have this functionality mapped out! #31052

@cacieprins cacieprins changed the title feat: extend Cypress.Keyboard.Keys and cy.press to support all keyboard keys feat: extend Cypress.Keyboard.Keys and cy.press to support (almost) all keyboard keys Aug 6, 2025
cursor[bot]

This comment was marked as outdated.

Comment on lines 24 to 25
export function isSupportedKey (key: string): key is KeyPressSupportedKeys {
return CDP_KEYCODE[key] && BIDI_VALUE[key]
}

export const CDP_KEYCODE: KeyCodeLookup = {
'Tab': 'U+000009',
return KEY_HEX_TABLE[key as KeyPressSupportedKeys] !== undefined
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason not to narrow the parameter type to KeyPressSupportedKeys? That change would remove the need for the type assertion inside the function.

export function isSupportedKey (key: KeyPressSupportedKeys): key is KeyPressSupportedKeys {
  return typeof KEY_HEX_TABLE[key] !== 'undefined'
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I've since changed how this works, but-- this is a type guard function, which casts the parameter of type T to the predicate type U.

function isCat (creature: Animal): creature is Cat {
  return creature.family === 'Felidae`
}

For the function isCat to be useful, its argument (Animal) must be a supertype of the guarded type (Cat). Otherwise you already know it's a Cat, so why invoke the function? :)

Comment on lines 170 to 172
for (const [key, code] of Object.entries(CDP_KEYCODE)) {
it(`dispatches a keydown followed by a keyup event to the provided send fn with the ${key} keycode`, async () => {
await cdpKeyPress({ key: key as KeyPressSupportedKeys }, sendFn, executionContexts, frameTree)
Copy link
Contributor

Choose a reason for hiding this comment

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

You might benefit from adding utils that return typed keys, values, and entries for objects. With these utils, you shouldn't need to use type assertions when working with Object.keys, Object.values, or Object.entries.

I've added these utils in other projects, and they've improved the TypeScript experience while also helping catch potential issues.

type ObjectKey<T> = keyof T;
type ObjectKeys<T> = ObjectKey<T>[];
type ObjectValue<T> = T[ObjectKey<T>];
type ObjectValues<T> = ObjectValue<T>[];
type ObjectEntries<T> = [ObjectKey<T>, ObjectValue<T>][];

export const getTypedObjectKeys = <T extends object>(obj: T): ObjectKeys<T> => {
  return Object.keys(obj) as ObjectKeys<T>;
};

export const getTypedObjectValues = <T extends object>(obj: T): ObjectValues<T> => {
  return Object.values(obj) as ObjectValues<T>;
};

export const getTypedObjectEntries = <T extends object>(obj: T): ObjectEntries<T> => {
  return Object.entries(obj) as ObjectEntries<T>;
};

@@ -4,6 +4,8 @@ export * from './constants'

export * from './preferences'

export { KeyPressSupportedKeys } from './automation'
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
export { KeyPressSupportedKeys } from './automation'
export type { KeyPressSupportedKeys } from './automation'

Copy link
Contributor

Choose a reason for hiding this comment

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

This has since been updated to export * from './automation', obviating the need to declare it as a type export.

for (const [key, value] of Object.entries(BIDI_VALUE)) {
// special formatting for value; otherwise test output will show the rendered unicode character
it(`dispatches a keydown and keyup action with the value '\\u${(value as string).charCodeAt(0).toString(16).toUpperCase()}' for key '${key}'`, async () => {
await bidiKeyPress({ key: key as KeyPressSupportedKeys }, client as WebdriverClient, autContext, 'idSuffix')
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is the WebdriverClient type assertion necessary? client appears to satisfy the type constraint of the second parameter in the bidiKeyPress function without it.

Copy link
Contributor

Choose a reason for hiding this comment

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

It was at a previous point to this, but is no longer. Thanks, it's been removed!

@cacieprins cacieprins marked this pull request as draft August 6, 2025 17:27
@cacieprins
Copy link
Contributor

cacieprins commented Aug 6, 2025

Converting to draft for now - there are major cross-browser issues with using the unicode codepoints as they are; this implementation (not the public signature) needs to be rethought.

cursor[bot]

This comment was marked as outdated.

@cacieprins cacieprins requested a review from AtofStryker August 25, 2025 17:35
cy.visit('/fixtures/input_events.html')
})

const testKeyDownUp = (key) => {
Copy link
Contributor

@adamalston adamalston Aug 26, 2025

Choose a reason for hiding this comment

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

Should key be typed?

Copy link
Contributor

Choose a reason for hiding this comment

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

It really doesn't matter here.

import $errUtils from '../../../cypress/error_utils'
import $utils from '../../../cypress/utils'

export interface PressCommand {
(key: KeyPressSupportedKeys, userOptions?: Partial<Cypress.Loggable> & Partial<Cypress.Timeoutable>): void
(key: SupportedKey | string, userOptions?: Partial<Cypress.Loggable> & Partial<Cypress.Timeoutable>): void
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the | string redundant given SupportedKey includes it?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, because SupportedKey always resolves to never unless the value is cast with toSupportedKey.


/**
* Union type representing all keys supported by cy.press().
* Includes single-character strings (inluding unicode characters with multiple code points)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* Includes single-character strings (inluding unicode characters with multiple code points)
* Includes single-character strings (including unicode characters with multiple code points)

* Must be cast to via `toSupportedKey` or guarded with `isSupportedKey`
* to ensure it is a valid key.
*/
export type SupportedKey = SupportedKeyType & string
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a reason the type of SupportedKey here doesn't match the type in cypress-automation.d.ts?

type SupportedKey = SupportedNamedKey | string

Copy link
Contributor

Choose a reason for hiding this comment

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

It's so users won't have to .toSupportedKey() whatever value they pass in.

@@ -149,38 +151,94 @@ describe('key:press automation command', () => {
})

it('dispaches a keydown followed by a keyup event to the provided send fn with the tab keycode', async () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
it('dispaches a keydown followed by a keyup event to the provided send fn with the tab keycode', async () => {
it('dispatches a keydown followed by a keyup event to the provided send fn with the tab keycode', async () => {

})

it('dispatches one keydown followed by a keyup event for each codepoint', async () => {
await bidiKeyPress(key, client, autContext, 'idSuffix')
Copy link
Contributor

Choose a reason for hiding this comment

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

I’ve noticed that the fourth parameter in all bidiKeyPress calls in this file is the same. Should there be some variation?

Copy link
Contributor

Choose a reason for hiding this comment

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

i'd prefer the duplication for readability but maybe just me

Comment on lines 156 to 162
.map((value): InputKeySourceAction[] => {
return [
{ type: 'keyDown', value },
{ type: 'keyUp', value },
]
})
.reduce((arr, el) => [...arr, ...el], []),
Copy link
Contributor

Choose a reason for hiding this comment

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

Can the map and reduce be combined to improve performance?

Copy link
Contributor

Choose a reason for hiding this comment

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

The longest the value can be, for the sake of single-pair combination utf-8 characters, is 2. Legibility is more important than performance when n<=2.

const activeWindow = await getActiveWindow(client)
const { contexts: [{ context: topLevelContext }] } = await client.browsingContextGetTree({})

const key: string = BidiOverrideCodepoints[inKey] ?? inKey
Copy link
Contributor

Choose a reason for hiding this comment

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

I was going to suggest removing the string type, since it should be inferred. However, I noticed that the type of inKey is being resolved as never. Could that be an issue?

I also see that variables typed as SupportedKey in packages/server/lib/automation/commands/key_press.ts are being resolved to never as well. I’m wondering if any type errors would appear if these variables were not resolving to never.

Copy link
Contributor

Choose a reason for hiding this comment

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

SupportedKey is a bit of a weird type here. It needs to include:

  • Named Keys
  • Single-codepoint utf-8 characters
  • Multi-codepoint utf-8 characters

but exclude:

  • Multiple characters

Given TS does not have a "character" type, this is a bit of a hack to ensure that the value of the string is programmatically ensured to be either a named key or a single character (allowing for multiple utf-8 codepoints).

Unless toSupportedKey is called (to ensure the check), SupportedKey will always resolve to never.

// multi-codepoint characters must be dispatched as individual codepoints
const isNamedKey = NamedKeys.includes(key)

const chars = [...key].length === 1 || isNamedKey ? [key] : [...key]
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this code be simplified to improve performance?

Suggested change
const chars = [...key].length === 1 || isNamedKey ? [key] : [...key]
const chars = isNamedKey ? [key] : [...key];

Copy link
Contributor

@AtofStryker AtofStryker Aug 26, 2025

Choose a reason for hiding this comment

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

I doubt there would be any measurable performance here. The suggestion also doesn't have the same effect

{ type: 'keyDown', value },
{ type: 'keyUp', value },
],
id: `${autContext}-${inKey}-${idSuffix || Date.now()}`,
Copy link
Contributor

Choose a reason for hiding this comment

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

The logical OR encourages the use of '' as a suffix. Also, will Date.now() collide if multiple calls are made in the same millisecond?

Suggested change
id: `${autContext}-${inKey}-${idSuffix || Date.now()}`,
id: `${autContext}-${inKey}-${idSuffix ?? crypto.randomUUID()}`,

Copy link
Contributor

Choose a reason for hiding this comment

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

Logical OR results in there always being a suffix, even if '' is passed in - an empty string is falsey. A nullish coalescing operator will result in context-key-, which is not quite what we want here.

TBH, the BiDi spec is very vague on what this id needs to be, and given the nature of how Cypress' server works, it's very unlikely to collide. I'm okay with switching it over to a random uuid. However, Date.now() is what was there previously, so that kind of refactor (including test updates) should probably be covered in a separate PR.

cli/CHANGELOG.md Outdated
@@ -3,6 +3,10 @@

_Released 08/26/2025 (PENDING)_

**Features:**

- Expanded `cy.press()` to support more key types. Addresses [#31051](https://github.com/cypress-io/cypress/issues/31051). Addressed in [#31496](https://github.com/cypress-io/cypress/pull/31496).
Copy link
Contributor

Choose a reason for hiding this comment

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

likely need to bump the version to 15.1.0 in the changelog. I can do that in the release PR if needed

return
}
// Non-BiDi firefox is not supported
if (Cypress.browser.family === 'firefox' && Cypress.browserMajorVersion() < 135) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think now that we are on 15 Cypress fails before this because we support bidi only? Likely don't need this check

// multi-codepoint characters must be dispatched as individual codepoints
const isNamedKey = NamedKeys.includes(key)

const chars = [...key].length === 1 || isNamedKey ? [key] : [...key]
Copy link
Contributor

@AtofStryker AtofStryker Aug 26, 2025

Choose a reason for hiding this comment

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

I doubt there would be any measurable performance here. The suggestion also doesn't have the same effect

})

it('dispatches one keydown followed by a keyup event for each codepoint', async () => {
await bidiKeyPress(key, client, autContext, 'idSuffix')
Copy link
Contributor

Choose a reason for hiding this comment

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

i'd prefer the duplication for readability but maybe just me

Copy link
Member Author

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

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

@cacieprins It would be nice to have a docs link here to on.cypress.io/press

Screenshot 2025-08-26 at 1 06 30 PM

Copy link
Member Author

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

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

@cacieprins Is this how a press of the SPACE key is meant to be handled?

cy.press(' ')
Screenshot 2025-08-26 at 1 12 52 PM

Copy link
Member Author

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

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

This actually works, the output is a bit weird though since it prints nothing. Not saying this has to be addressed, just commenting.

      // Test tab character
      cy.press('\t')
      cy.get('#lastKey').should('contain', 'Tab')

Copy link
Member Author

@jennifer-shehane jennifer-shehane left a comment

Choose a reason for hiding this comment

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

@cacieprins Oh, this seems like it should be handled better

cy.press(123)
Screenshot 2025-08-26 at 1 20 14 PM

This also happens on the below, which I think SHOULD work.

      cy.press(0)

cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

cursor[bot]

This comment was marked as outdated.

@cacieprins cacieprins merged commit 90310ae into develop Aug 28, 2025
89 of 91 checks passed
@cacieprins cacieprins deleted the feat-all-keys branch August 28, 2025 14:31
@cypress-bot
Copy link
Contributor

cypress-bot bot commented Sep 2, 2025

Released in 15.1.0.

This comment thread has been locked. If you are still experiencing this issue after upgrading to
Cypress v15.1.0, please open a new issue.

@cypress-bot cypress-bot bot locked as resolved and limited conversation to collaborators Sep 2, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Space key support for cy.press Additional key support for cy.press
5 participants