Pseudo States: Handle :where() wrapping in CSS selector substitution#35030
Pseudo States: Handle :where() wrapping in CSS selector substitution#35030AliMahmoudDev wants to merge 1 commit into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughDetect which pseudo-states were inside ChangesCSS Pseudo-State Selector Rewriting
sequenceDiagram
participant extractPseudoStates
participant replacePseudoStatesWithAncestorSelector
participant Storybook/CSS
extractPseudoStates->>replacePseudoStatesWithAncestorSelector: { withoutPseudoStates, whereWrappedStates }
replacePseudoStatesWithAncestorSelector->>Storybook/CSS: emit ancestor selectors preserving :where() wrapping
🎯 3 (Moderate) | ⏱️ ~20 minutes
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/addons/pseudo-states/src/preview/rewriteStyleSheet.ts`:
- Around line 49-62: The code currently uses a single hadWhereWrapper flag to
decide wrapping all pseudo-state classes in :where(...) when building
ancestorPrefix (in the block computing ancestorPrefix using forShadowDOM,
hadWhereWrapper, selectors), which incorrectly collapses mixed wrapped/unwrapped
pseudo-states; change the extraction/concat logic to track hadWhereWrapper per
matched pseudo-state/group (e.g., store an array of {selectorClass,
hadWhereWrapper} from the extraction step) and then build the ancestor selector
by joining wrapped classes inside a single :where(...) and appending unwrapped
classes outside it (preserving order), so that when you compute ancestorPrefix
(and when handling withoutPseudoStates starting with :host-context(...)) you
only wrap the specific pseudo-state tokens that originally had a :where wrapper
rather than applying hadWhereWrapper globally to selectors or to the entire
selectors string.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0ec2c777-c714-4e83-ab65-310a211b7037
📒 Files selected for processing (2)
code/addons/pseudo-states/src/preview/rewriteStyleSheet.test.tscode/addons/pseudo-states/src/preview/rewriteStyleSheet.ts
This comment was marked as abuse.
This comment was marked as abuse.
🧩 Analysis chain🏁 Script executed: #!/bin/bash
# Check available labels in the repository
gh label list --repo storybookjs/storybook --limit 100 | grep -E "bug|ci:normal|qa:needed"Length of output: 203 🏁 Script executed: #!/bin/bash
# Search more broadly
gh label list --repo storybookjs/storybook --limit 200 | grep -iE "ci|qa"Length of output: 916 🏁 Script executed: #!/bin/bash
# Add the available labels to the PR
gh pr edit 35030 --repo storybookjs/storybook --add-label "bug,ci:normal"Length of output: 420
Here's what I confirmed about the labels:
Regarding the fix you pushed — thank you for addressing the per-pseudo-state ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/addons/pseudo-states/src/preview/rewriteStyleSheet.test.ts`:
- Around line 562-571: The test expectation is out of sync with
rewriteStyleSheet's output for `.textLink:hover:where(:focus-visible)`; update
the assertions in rewriteStyleSheet.test.ts to expect the implementation's
ordering (`:where(.pseudo-focus-visible-all).pseudo-hover-all`) instead of
`.pseudo-hover-all:where(.pseudo-focus-visible-all)` — locate the test case that
calls new Sheet(...) and rewriteStyleSheet(...) and change the selector
expectation lines (the expectations that currently reference
`.pseudo-hover-all:where(.pseudo-focus-visible-all)` and
`:where(.pseudo-hover-all.pseudo-focus-visible-all)`) to match the produced
selectors from rewriteStyleSheet/Sheet.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6cf6aa62-3b27-4042-a4fd-1b9e0b13617b
📒 Files selected for processing (2)
code/addons/pseudo-states/src/preview/rewriteStyleSheet.test.tscode/addons/pseudo-states/src/preview/rewriteStyleSheet.ts
🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh pr edit 35030 --repo storybookjs/storybook --add-label "bug,ci:normal"Length of output: 383
You'll need to ask a Storybook maintainer (e.g., via a ping in their Discord or by tagging a maintainer directly on the PR) to apply the following labels:
Regarding the per-pseudo-state ✅ Actions performedReview triggered.
|
|
Hi @AliMahmoudDev, Due to a recent high volume of unreviewed AI-generated PRs, we are requesting verification and proof that the implemented fix actually works. Please provide a simple GIF/Video or image of how the fix works, optimally with before-and-after comparisons. Thank you for your understanding! |
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
Sidnioulz
left a comment
Sidnioulz
left a comment
There was a problem hiding this comment.
Thanks for the PR @AliMahmoudDev! This is great stuff!
Could I please ask you to add a story in code/addons/pseudo-states/src/stories/ which matches the reproduction in the original issue? Then, you can either force the addon state in a particular state using parameters/globals, and use a play function to verify the computed CSS properties for the component, or write an E2E test that selects the right value in the addon tool, and the verifies what's on screen?
Doing this will also make it possible to add a test case for components that don't use where but that also rely on CSS ordering.
If you're not sure how to do this, feel free to come on http://discord.com/invite/storybook in the #contributing channel, and we'll be able to help onboard you. We have an office hour every Monday at 3PM CET which you can join too.
Thanks!
| if (whereWrappedStates.size === 0) { | ||
| return `${selectors} `; | ||
| } |
There was a problem hiding this comment.
Genuine question: is there a reason we don't want to also use :where for our pseudostate selector when the existing component selector doesn't use :where?
Why do we need to preserve a higher specificity in that case? Wouldn't we run into the same class of bugs if a user relied on CSS file ordering without where to assign styles?
| const wrappedParts = extracted.states | ||
| .filter((s) => whereWrappedStates.has(s)) | ||
| .map((s) => `.pseudo-${s}-all`) | ||
| .join(''); |
There was a problem hiding this comment.
nit: please double check the join content, I'm not sure what extracted contains and can't decide if this is what we want, or if we want a space.
| // Find all :where() block ranges to track per-state wrapping | ||
| const whereRanges: Array<{ start: number; end: number }> = []; | ||
| const wherePattern = /:where\(/g; | ||
| let whereMatch; | ||
| while ((whereMatch = wherePattern.exec(selector)) !== null) { | ||
| const start = whereMatch.index; | ||
| let depth = 1; | ||
| let i = start + whereMatch[0].length; | ||
| while (i < selector.length && depth > 0) { | ||
| if (selector[i] === '(') depth++; | ||
| if (selector[i] === ')') depth--; | ||
| i++; | ||
| } | ||
| whereRanges.push({ start, end: i }); | ||
| } |
There was a problem hiding this comment.
nit, maintainability: This is fairly complex. Let's please take a look at whether we can apply where more generally so we can maybe avoid this regexp matching and parsing.
This comment was marked as abuse.
This comment was marked as abuse.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
code/addons/pseudo-states/src/stories/WhereSelector.stories.tsx (2)
4-4: ⚡ Quick winRemove unused imports.
The imports
fnanduserEventare not used anywhere in this file.🧹 Proposed cleanup
-import { expect, fn, userEvent, waitFor, within } from 'storybook/test'; +import { expect, waitFor, within } from 'storybook/test';🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/addons/pseudo-states/src/stories/WhereSelector.stories.tsx` at line 4, The import line in WhereSelector.stories.tsx imports unused symbols `fn` and `userEvent` from 'storybook/test'; remove `fn` and `userEvent` from that import so only used symbols (`expect`, `waitFor`, `within`) remain, updating the import statement accordingly to eliminate the unused imports and satisfy linting.
18-23: 💤 Low valueConsider using
waitForfor consistency.The Default story doesn't use
waitFor, while all other stories do. Although the default state may not require waiting for pseudo-state transformations, there could still be a race condition if stylesheets haven't fully loaded yet. UsingwaitForconsistently across all stories would make the test suite more robust.📝 Suggested enhancement
export const Default: Story = { play: async ({ canvas }) => { const link = within(canvas).getByRole('link'); - await expect(link).toHaveComputedStyle({ color: 'rgb(0, 0, 0)' }); + await waitFor(async () => { + await expect(link).toHaveComputedStyle({ color: 'rgb(0, 0, 0)' }); + }); }, };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@code/addons/pseudo-states/src/stories/WhereSelector.stories.tsx` around lines 18 - 23, Wrap the existing assertion inside a waitFor to match other stories and avoid potential stylesheet race conditions: in the Default story's play function (the play block on the Default Story object) import and use waitFor and change the assertion to await waitFor(() => { const link = within(canvas).getByRole('link'); expect(link).toHaveComputedStyle({ color: 'rgb(0, 0, 0)' }); }); — keep the same selector (within(canvas).getByRole('link')) and expectation but run it inside waitFor for consistency and stability.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@code/addons/pseudo-states/src/stories/WhereSelector.stories.tsx`:
- Around line 18-65: The stories call within(canvas).getByRole('link') but
WhereSelector renders an <a> without href so it has no implicit link role;
update the exported Story objects (Default, Hover, FocusVisible,
FocusVisibleAndHover) to include args: { href: '#', children: 'Link' } (or
equivalent props) so the rendered <a> has the link role and the play() queries
succeed; alternatively, if you prefer not to add href, change the queries in the
play() functions to target the actual rendered element (e.g.,
getByText('Link')).
---
Nitpick comments:
In `@code/addons/pseudo-states/src/stories/WhereSelector.stories.tsx`:
- Line 4: The import line in WhereSelector.stories.tsx imports unused symbols
`fn` and `userEvent` from 'storybook/test'; remove `fn` and `userEvent` from
that import so only used symbols (`expect`, `waitFor`, `within`) remain,
updating the import statement accordingly to eliminate the unused imports and
satisfy linting.
- Around line 18-23: Wrap the existing assertion inside a waitFor to match other
stories and avoid potential stylesheet race conditions: in the Default story's
play function (the play block on the Default Story object) import and use
waitFor and change the assertion to await waitFor(() => { const link =
within(canvas).getByRole('link'); expect(link).toHaveComputedStyle({ color:
'rgb(0, 0, 0)' }); }); — keep the same selector
(within(canvas).getByRole('link')) and expectation but run it inside waitFor for
consistency and stability.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 08e63b68-48cb-48f3-92b7-1c16223adeb6
📒 Files selected for processing (3)
code/addons/pseudo-states/src/stories/WhereSelector.stories.tsxcode/addons/pseudo-states/src/stories/WhereSelector.tsxcode/addons/pseudo-states/src/stories/whereselector.css
✅ Files skipped from review due to trivial changes (2)
- code/addons/pseudo-states/src/stories/WhereSelector.tsx
- code/addons/pseudo-states/src/stories/whereselector.css
| export const Default: Story = { | ||
| play: async ({ canvas }) => { | ||
| const link = within(canvas).getByRole('link'); | ||
| await expect(link).toHaveComputedStyle({ color: 'rgb(0, 0, 0)' }); | ||
| }, | ||
| }; | ||
|
|
||
| export const Hover: Story = { | ||
| parameters: { | ||
| pseudo: { hover: true }, | ||
| }, | ||
| play: async ({ canvas }) => { | ||
| const link = within(canvas).getByRole('link'); | ||
| await waitFor(async () => { | ||
| await expect(link).toHaveComputedStyle({ color: 'rgb(255, 0, 0)' }); | ||
| }); | ||
| }, | ||
| }; | ||
|
|
||
| export const FocusVisible: Story = { | ||
| parameters: { | ||
| pseudo: { focusVisible: true }, | ||
| }, | ||
| play: async ({ canvas }) => { | ||
| const link = within(canvas).getByRole('link'); | ||
| await waitFor(async () => { | ||
| // The :where(:focus-visible) rule has specificity 0-1-0, same as :hover. | ||
| // With the fix, the ancestor selector also has specificity 0-1-0, | ||
| // so focus-visible correctly applies blue (comes after hover in the stylesheet). | ||
| await expect(link).toHaveComputedStyle({ color: 'rgb(0, 0, 255)' }); | ||
| }); | ||
| }, | ||
| }; | ||
|
|
||
| export const FocusVisibleAndHover: Story = { | ||
| parameters: { | ||
| pseudo: { focusVisible: true, hover: true }, | ||
| }, | ||
| play: async ({ canvas }) => { | ||
| const link = within(canvas).getByRole('link'); | ||
| await waitFor(async () => { | ||
| // Both states active: mixed :where() wrapping means :focus-visible is wrapped | ||
| // but :hover is not, so both selectors contribute. The combined rule | ||
| // .textLink:where(:focus-visible):hover should make the text bold. | ||
| await expect(link).toHaveComputedStyle({ fontWeight: '700' }); | ||
| }); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check WhereSelector component implementation for default content or children
echo "=== WhereSelector component implementation ==="
cat code/addons/pseudo-states/src/stories/WhereSelector.tsxRepository: storybookjs/storybook
Length of output: 331
🏁 Script executed:
#!/bin/bash
# Show how WhereSelector is rendered/what props are passed in the stories
sed -n '1,140p' code/addons/pseudo-states/src/stories/WhereSelector.stories.tsxRepository: storybookjs/storybook
Length of output: 2055
🌐 Web query:
In dom-accessibility-api / Testing Library, does an <a> element without an href get exposed with accessible role "link"? Provide the rule/source.
💡 Result:
No, an element without an href attribute is not exposed with the accessible role "link" [1][2]. According to the HTML standard, an element represents a hyperlink only if it has an href attribute [2]. If the href attribute is absent, the element is considered a placeholder link [3][2] and does not possess the implicit "link" role [1]. In the context of Testing Library, which queries the accessibility tree (via libraries like dom-accessibility-api/aria-query), the getByRole('link') selector will not return an element that lacks an href attribute because it does not have the "link" role [4][5]. Under these conditions, the element typically defaults to a "generic" role [1] or is not treated as an interactive link by assistive technology [6]. Rule/Source: - HTML Standard (WHATWG): Specifies that an element has an href attribute to represent a hyperlink [2]. - ARIA in HTML (W3C): Defines implicit ARIA roles for HTML elements, where without an href does not map to "link" [7]. - Testing Library Behavior: Confirmed in issues and documentation that getByRole('link') requires the presence of an href attribute to identify the element as a link [4][8].
Citations:
- 1: https://stackoverflow.com/questions/78036048/shadow-dom-role-behaviour-a-tag-without-href
- 2: https://html.spec.whatwg.org/multipage/text-level-semantics.html
- 3: https://dev.w3.org/html5/spec-LC/text-level-semantics.html
- 4: getByRole ignores role "link" on achors without href attribute or falsy value testing-library/react-testing-library#1306
- 5: https://stackoverflow.com/questions/69725673/react-testing-library-cant-find-anchor-element-by-role
- 6: https://www.scottohara.me/note/2019/07/17/placeholder-link.html
- 7: https://www.w3.org/TR/2024/REC-html-aria-20241213/
- 8: Get/Log Roles helpers doesn't return anchors when querying link roles testing-library/dom-testing-library#313
🏁 Script executed:
#!/bin/bash
# Search for href/children/args in this story folder
rg -n "href|children|args:" code/addons/pseudo-states/src/stories/WhereSelector.stories.tsx code/addons/pseudo-states/src/stories/WhereSelector.tsx code/addons/pseudo-states/src/stories/whereselector.cssRepository: storybookjs/storybook
Length of output: 47
🌐 Web query:
dom-accessibility-api aria-query implicit role link for <a> without href getByRole('link')
💡 Result:
In the context of the DOM accessibility API and testing frameworks like Testing Library, an <a> element without an href attribute does not have the implicit link role [1][2][3]. According to the HTML Accessibility API Mappings (HTML-AAM) specification, the accessibility role of an <a> element is determined by the presence of the href attribute [2][4]: * With href: The element is assigned the implicit link role [1][5][2]. * Without href: The element is considered a placeholder and is assigned a generic role (or no specific semantic role) [2][6][3]. Because Testing Library's getByRole query relies on the accessibility tree—which is generated based on these standards—it will not find an <a> tag that lacks an href attribute when queried for the link role [7][8][9]. This behavior is intentional and reflects how browsers and assistive technologies treat these elements; an anchor without an href is generally not navigable via keyboard as a link would be [9]. If you need to select such an element, you would need to use a different selector (such as getByRole('generic') or a CSS selector like querySelector('a')) or, ideally, ensure the element is semantically correct for its intended use (e.g., using a <button> if it triggers an action) [7][8][9].
Citations:
- 1: eps1lon/dom-accessibility-api@fe4fab5
- 2: https://www.w3.org/TR/2022/REC-html-aria-20220927/
- 3: https://stackoverflow.com/questions/78036048/shadow-dom-role-behaviour-a-tag-without-href
- 4: https://bugzilla.mozilla.org/show_bug.cgi?id=854796
- 5: https://zenn.dev/yusukehirao/articles/acba4a2a3dd909?locale=en
- 6: https://html.spec.whatwg.org/multipage/text-level-semantics.html
- 7: getByRole ignores role "link" on achors without href attribute or falsy value testing-library/react-testing-library#1306
- 8: should getByRole('textbox') and getByRole('link') return elements with no role testing-library/dom-testing-library#779
- 9: https://stackoverflow.com/questions/68969275/getbyrole-for-text-under-li-a-on-test-library
Fix getByRole('link')—WhereSelector renders an <a> without href in these stories
WhereSelector returns <a {...props} ... /> and none of the stories pass href (or children) via args, so the element won’t have the implicit accessible “link” role; within(canvas).getByRole('link') can’t reliably find it.
- Add
args: { href: '#', children: 'Link' }(or otherwise ensurehrefis present), or switch the query to the actual role you’re rendering.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@code/addons/pseudo-states/src/stories/WhereSelector.stories.tsx` around lines
18 - 65, The stories call within(canvas).getByRole('link') but WhereSelector
renders an <a> without href so it has no implicit link role; update the exported
Story objects (Default, Hover, FocusVisible, FocusVisibleAndHover) to include
args: { href: '#', children: 'Link' } (or equivalent props) so the rendered <a>
has the link role and the play() queries succeed; alternatively, if you prefer
not to add href, change the queries in the play() functions to target the actual
rendered element (e.g., getByText('Link')).
There was a problem hiding this comment.
@AliMahmoudDev FYI.
Have you run the test locally? You can run it by doing this
- Create a sandbox (yarn task sandbox) and select react-vite/default-ts
- Run that sandbox on port 6006
- Uncomment lines 7 and 8 in code/playwright.config.ts
- Run the test suite
This comment was marked as abuse.
This comment was marked as abuse.
Sidnioulz
added
the
ci:normal
Sidnioulz
changed the title
Let's see how they go in CI. I just noticed this PR was missing the label to run end-to-end tests. If CodeRabbit is right, there should be a failure for you to fix. |
Package BenchmarksCommit: The following packages have significant changes to their size or dependencies:
|
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 184 | 184 | 0 |
| Self size | 79 KB | 79 KB | 0 B |
| Dependency size | 33.90 MB | 33.95 MB | 🚨 +53 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 74 | 72 | 🎉 -2 🎉 |
| Self size | 20.44 MB | 20.41 MB | 🎉 -30 KB 🎉 |
| Dependency size | 36.65 MB | 36.11 MB | 🎉 -539 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/angular
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 185 | 185 | 0 |
| Self size | 160 KB | 160 KB | 0 B |
| Dependency size | 31.24 MB | 31.30 MB | 🚨 +53 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/ember
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 188 | 188 | 0 |
| Self size | 15 KB | 15 KB | 🎉 -18 B 🎉 |
| Dependency size | 30.62 MB | 30.67 MB | 🚨 +53 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 534 | 534 | 0 |
| Self size | 662 KB | 662 KB | 🚨 +120 B 🚨 |
| Dependency size | 62.08 MB | 62.13 MB | 🚨 +50 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/nextjs-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 93 | 93 | 0 |
| Self size | 1.38 MB | 1.38 MB | 0 B |
| Dependency size | 24.85 MB | 24.90 MB | 🚨 +50 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-native-web-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 122 | 122 | 0 |
| Self size | 30 KB | 30 KB | 🚨 +18 B 🚨 |
| Dependency size | 25.92 MB | 25.97 MB | 🚨 +50 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-vite
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 83 | 83 | 0 |
| Self size | 36 KB | 36 KB | 0 B |
| Dependency size | 22.62 MB | 22.67 MB | 🚨 +50 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 271 | 271 | 0 |
| Self size | 23 KB | 23 KB | 🎉 -12 B 🎉 |
| Dependency size | 46.61 MB | 46.66 MB | 🚨 +50 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/server-webpack5
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 196 | 196 | 0 |
| Self size | 16 KB | 16 KB | 0 B |
| Dependency size | 35.16 MB | 35.22 MB | 🚨 +53 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/tanstack-react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 84 | 84 | 0 |
| Self size | 109 KB | 108 KB | 🎉 -1 KB 🎉 |
| Dependency size | 22.66 MB | 22.71 MB | 🚨 +50 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/cli
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 205 | 203 | 🎉 -2 🎉 |
| Self size | 908 KB | 908 KB | 🚨 +144 B 🚨 |
| Dependency size | 89.17 MB | 88.65 MB | 🎉 -515 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
@storybook/codemod
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 198 | 196 | 🎉 -2 🎉 |
| Self size | 32 KB | 32 KB | 🎉 -36 B 🎉 |
| Dependency size | 87.66 MB | 87.14 MB | 🎉 -515 KB 🎉 |
| Bundle Size Analyzer | Link | Link |
create-storybook
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 75 | 73 | 🎉 -2 🎉 |
| Self size | 1.08 MB | 1.08 MB | 🚨 +66 B 🚨 |
| Dependency size | 57.08 MB | 56.52 MB | 🎉 -569 KB 🎉 |
| Bundle Size Analyzer | node | node |
@storybook/preset-react-webpack
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 164 | 164 | 0 |
| Self size | 18 KB | 18 KB | 0 B |
| Dependency size | 32.43 MB | 32.48 MB | 🚨 +53 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
@storybook/react
| Before | After | Difference | |
|---|---|---|---|
| Dependency count | 59 | 59 | 0 |
| Self size | 1.51 MB | 1.51 MB | 🎉 -3 KB 🎉 |
| Dependency size | 13.32 MB | 13.37 MB | 🚨 +53 KB 🚨 |
| Bundle Size Analyzer | Link | Link |
|
@AliMahmoudDev see the CI errors! You can see that some of your tests are failing. Please make sure you run them locally before committing! |
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
This comment was marked as abuse.
|
@AliMahmoudDev could you please go through the review comments and CI failures? Thanks. |
This comment was marked as abuse.
This comment was marked as abuse.
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
gh pr edit 35030 --repo storybookjs/storybook --add-label "bug,ci:normal"Length of output: 383
Your best bet is to ping a maintainer directly on this PR. Since Here's a quick summary for the maintainer:
🧠 Learnings used |
github-project-automation
Bot
moved this from Human verification
to Done
in Core Team Projects
3 participants
References
Fixes #31411
Code changes
In
code/addons/pseudo-states/src/preview/rewriteStyleSheet.ts:When a CSS selector uses
:where()to wrap a pseudo-state (e.g..textLink:where(:focus-visible)), the ancestor selector was generated without:where(), resulting in higher specificity (0-2-0 instead of 0-1-0) which changed the resultant styles unexpectedly.The bug
Input:
.textLink:where(:focus-visible) { outline: 1px solid red; }Before (wrong):
After (fixed):
Changes
extractPseudoStates()now tracks per-pseudo-state:where()wrapping by finding:where()block ranges and checking each pseudo-state's match position against those ranges. This correctly handles mixed cases like.textLink:where(:focus-visible):hoverwhere only:focus-visibleis inside:where().replacePseudoStatesWithAncestorSelector()uses per-state:where()information to build the ancestor prefix::where()get wrapped::where(.pseudo-focus-visible-all):where()stay unwrapped:.pseudo-hover-all:where(.pseudo-focus-visible-all).pseudo-hover-all .textLinkTests added
5 new tests in
rewriteStyleSheet.test.ts::where(:focus-visible)preserves wrapper in ancestor selector:where(:hover, :focus-visible)combined selectors preserve wrapper:where()remain unchanged (no regression):where(:focus-visible):hovertracks per-state wrapping:hover:where(:focus-visible)tracks per-state wrappingUser-facing changes
Users who wrap pseudo-states in
:where()(e.g., for specificity control in design systems) will no longer see unexpected style changes when the pseudo-states addon is active.Backwards-incompatible changes
None. The fix only affects the generated ancestor selector and brings it in line with the expected specificity behavior.
AI usage
Manual testing
:where()to wrap a pseudo-state, e.g.:Summary by CodeRabbit
Bug Fixes
New Examples
Tests