Skip to content

Conversation

@iumehara
Copy link

References #35113

  • Overload toHaveCSS to take styles property (React.CSSProperties) type.
  • Extend Matchers interface in overrides-test.d.ts to include new signature


CSS property value.

### param: LocatorAssertions.toHaveCSS.styles
Copy link
Member

Choose a reason for hiding this comment

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

You need to update value parameter's type to accept React.CSSProperties rather than add a new parameter.

Copy link
Author

Choose a reason for hiding this comment

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

What makes this a little awkward is that this feature requires less parameters than before. ie. one parameter for the pojo CSS Properties, as opposed to the two parameters for the name and the value.

The current PR handles multiple possible inputs for the first argument (either the name string, or the React.CSSProperties), albeit with some questionable naming.

Should I just delete the overload signatures, and use the main function definition that has union parameters?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry, I was not clear. What I meant is you can use function overload with #1 and #2 suffixes. See ## method: TestInfo.fixme#1 for example. Then first overload will stay as is and the second one will have CSSProperty as the only parameter. Then in the documentation we'd have 2 entries, one per overload (see e.g. https://playwright.dev/docs/api/class-testinfo#test-info-fixme-1), you'd need to make sure that there is proper usage section that presents both signatures. Support of overloaded methods is a bit messy in our API, but CSSPropery is not much different than other overloads, so it should work.

Copy link
Author

Choose a reason for hiding this comment

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

Got it. I kept the existing implementation untouched as the base method, and added the new method definition as #2. This way, the other documentation that references toHaveCSS can remain unchanged.

A couple questions:

  1. I did not add any usage related to non-JS APIs. Is that okay? Given the new CSSProperties parameter type, I wasn't sure how to translate them to different languages.

  2. I set since to v1.58. Is that right?

Copy link
Member

Choose a reason for hiding this comment

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

  1. I did not add any usage related to non-JS APIs. Is that okay? Given the new CSSProperties parameter type, I wasn't sure how to translate them to different languages.

Let's add langs: js to filter it out from other ports. I've added a comment.

  1. I set since to v1.58. Is that right?

Yes.

@github-actions

This comment has been minimized.

- revert existing method definition to be the base definition. Add definition#2 that takes CSSProperties
- Define CSSProperties type that is React.CSSProperties if react is installed. If not, defaults to an object with string keys
@iumehara iumehara force-pushed the 35113-toHaveCSS-reactCSS-overload branch from 2d3bb1a to 02b8374 Compare January 4, 2026 05:36
@github-actions

This comment has been minimized.

@github-actions

This comment has been minimized.

Copy link
Member

Choose a reason for hiding this comment

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

Remove this, we normally rely on the e2e tests with very rare exceptions for unit tests.


// @ts-ignore this will be any if react is not installed
type ReactCSSProperties = import('react').CSSProperties;
type FallbackCSSProperties = { [name: string]: string | number | undefined };
Copy link
Member

Choose a reason for hiding this comment

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

This should be something like never when react is missing to indicate the error. We don't want this signature to be used without react.

* since: v1.18

## async method: LocatorAssertions.toHaveCSS#2
* since: v1.20
Copy link
Member

Choose a reason for hiding this comment

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

let's add * langs: js to not have in language ports.

### option: LocatorAssertions.toHaveCSS.timeout = %%-js-assertions-timeout-%%
* since: v1.18

### option: LocatorAssertions.toHaveCSS.timeout = %%-csharp-java-python-assertions-timeout-%%
Copy link
Member

Choose a reason for hiding this comment

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

You need to add #1 to the existing toHaveCSS declaration.

Copy link
Author

Choose a reason for hiding this comment

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

Sounds good. I had originally left the original implementation as the base method without the #1 from referring to how toHaveAttribute is implemented.

I updated the referring documentation to point to #1.


CSS property value.

### param: LocatorAssertions.toHaveCSS.styles
Copy link
Member

Choose a reason for hiding this comment

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

  1. I did not add any usage related to non-JS APIs. Is that okay? Given the new CSSProperties parameter type, I wasn't sure how to translate them to different languages.

Let's add langs: js to filter it out from other ports. I've added a comment.

  1. I set since to v1.58. Is that right?

Yes.

- give the base implementation a microsoft#1 overload and update referencing documentation
- CSSProperties type defaults to never if react is not installed
@iumehara iumehara requested a review from yury-s January 8, 2026 03:36
Co-authored-by: Yury Semikhatsky <[email protected]>
Signed-off-by: iumehara <[email protected]>
@github-actions
Copy link
Contributor

Test results for "tests 1"

1 failed
❌ [playwright-test] › playwright.spec.ts:877 › page.pause() should disable test timeout @macos-latest-node20-1

3 flaky ⚠️ [firefox-library] › library/inspector/cli-codegen-1.spec.ts:1082 › cli codegen › should not throw csp directive violation errors `@firefox-ubuntu-22.04-node20`
⚠️ [firefox-page] › page/page-aria-snapshot-ai.spec.ts:422 › return empty snapshot when iframe is not loaded `@firefox-ubuntu-22.04-node20`
⚠️ [firefox-page] › page/page-wait-for-function.spec.ts:104 › should work with strict CSP policy `@firefox-ubuntu-22.04-node20`

34431 passed, 691 skipped


Merge workflow run.

@github-actions
Copy link
Contributor

Test results for "MCP"

1 failed
❌ [chromium] › mcp/test-debug.spec.ts:167 › test_debug (pause/snapshot/resume) @mcp-macos-15

2821 passed, 116 skipped


Merge workflow run.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants