Fixed: Issue 25818 Preview Area Weird Boarder Accessibility Addon #28844
Conversation
Changed how accessibility filters are applied within Storybook. The filters are no longer applied to the iframe rather we apply filters to iframe contents by targeting the 'storybook-root' div within the iframe. This fixes the weird border bug caused by the blur filter and will also prevent any future unwanted interactions of accessibility filters with the iframe's css. Co-Authored-By: RuxueJ <[email protected]> Co-Authored-By: allybc1124 <[email protected]>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM
1 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
I really appreciate the time and effort put into this PR.
Visually this solves the problem it aims to fix, however it doesn't work in all use-cases, but I understand you didn't know about all.
If you have the time and energy to change this PR somewhat significantly, I'd like to ask you to change it.
Main take-away is that accessing the iframe's contentDocument is something we avoid, due to a feature called storybook composition (1 storybook embedding others).
I'm thinking perhaps we upgrade the addon to work more like the background addon does, where a global is used for state, which can be read in the preview as well (in a decorator).
WDYT?
|
|
||
| useEffect(() => { | ||
| const iframe = document.getElementById(iframeId) as HTMLIFrameElement; | ||
| if (iframe && iframe.contentDocument) { |
There was a problem hiding this comment.
Accessing the contentDocument will only work if the iframe is on the same origin, which is not always the case.
We should encapsulate this behavior in a storybook decorator instead, similar to how the backgrounds addon works.
Syncing state via a global.
There was a problem hiding this comment.
Hi @ndelangen thank you for taking a look at our PR and for leaving such a detailed response.
In the issue page, we left a simpler alternate solution which is to remove the seemingly unnecessary boxShadow css in iframe.tsx. The boxShadow has no visual effect on preview window at the moment and since the blur filter only causes the bug with this boxShadow, this would resolve the weird border bug and then upgrading the addon can be a separate issue? You can find this here:
If you still prefer upgrading the addon for this particular issue, could you roughly outline the steps needed and your expectation? Like would the a11y addon need to be structured similarly to the background addon? We would like to understand this approach a bit better before diving right in.
Closes #25818
What I did
We fixed the border issue with the blur filter by:
Checklist for Contributors
Testing
The changes in this PR are covered in the following automated tests:
Manual testing
This section is mandatory for all contributions. If you believe no manual test is necessary, please state so explicitly. Thanks!
We tested every filter with the accessibility addon.
yarn build-storybookandyarn storybookDocumentation
MIGRATION.MD
Checklist for Maintainers
When this PR is ready for testing, make sure to add
ci:normal,ci:mergedorci:dailyGH label to it to run a specific set of sandboxes. The particular set of sandboxes can be found incode/lib/cli/src/sandbox-templates.tsMake sure this PR contains one of the labels below:
Available labels
bug: Internal changes that fixes incorrect behavior.maintenance: User-facing maintenance tasks.dependencies: Upgrading (sometimes downgrading) dependencies.build: Internal-facing build tooling & test updates. Will not show up in release changelog.cleanup: Minor cleanup style change. Will not show up in release changelog.documentation: Documentation only changes. Will not show up in release changelog.feature request: Introducing a new feature.BREAKING CHANGE: Changes that break compatibility in some way with current major version.other: Changes that don't fit in the above categories.🦋 Canary release
This PR does not have a canary release associated. You can request a canary release of this pull request by mentioning the
@storybookjs/coreteam here.core team members can create a canary release here or locally with
gh workflow run --repo storybookjs/storybook canary-release-pr.yml --field pr=<PR_NUMBER>Greptile Summary
The changes in
VisionSimulator.tsxresolve the issue of applying filters to iframe content instead of the iframe itself, ensuring proper filter application without affecting the iframe's border.useEffectto apply filters to iframe content dynamically.injectFiltersfunction to inject customized SVG filters into the iframe.Hiddenstyled component and theGlobalcomponent that applied filters directly to the iframe.Thorough testing is recommended to ensure all filters work as expected.