-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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
Style Book: Move iframe to root of content area to support styles that overflow block previews #48664
Style Book: Move iframe to root of content area to support styles that overflow block previews #48664
Conversation
Size Change: +3.16 kB (0%) Total Size: 1.34 MB
ℹ️ View Unchanged
|
Flaky tests detected in f1a2c35. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4359383899
|
…ecting the height of the preview
@joedolson just adding you as a reviewer for this one, too, as we've been looking at accessibility aspects of the Style Book lately. This PR seeks to swap the |
A quick test shows this working well to solve the box-shadow problem! It's also loading the stylebook noticeably faster than on trunk 🎉
Is it just the default UA styles we're trying to avoid? I wonder if it would be easier to do something like If we do go the div route, we'll also need a handler for the Space key. I'll do a proper review next week, but this is definitely looking like the way to go! |
Thanks for tackling this one, it feels like a much better solution. 🙏 From the visuals it looks right. |
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.
This is working really well in my testing. Button shadows now looking exactly as expected. I also confirmed that the duotone filter is working as expected on images:
I tested two themes with more interesting fonts, Sassify and Tortoise. Both showed their fonts in the style book as expected and I saw no difference between trunk and this branch.
I didn't check the code, but looks like @kevin940726 is giving that a look over.
The I'm not totally clear on why this was necessary, however; why were button styles bleeding between those blocks and the button? The button block creates an I'm reluctant to embrace using a faux button unless we really have to, and right now I don't see the reasoning. It'll need to be thoroughly tested on all standard screen reader/browser/os combinations, since there can be a lot of variation in how they handle custom events. |
Update: the suggestion from @tellthemachines of using I've also updated the e2e tests based on the suggestions from @kevin940726, so this PR should be ready for another review now 🙂 |
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.
Code's looking pretty good to me, aside from one minor comment below!
Testing on Chrome, Firefox and Safari on macOS didn't reveal any issues ✅
It's also pretty exciting that theme gradient and pattern backgrounds now work seamlessly! E.g. this is the Pilgrimage TT3 variation on trunk:
vs this branch:
Note how on trunk the dots only appear in a little window around each block. It's looking much better with this change!
background: none; | ||
border-radius: 2px; | ||
border: none; | ||
color: inherit; |
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.
We shouldn't need to set background and border to none
, or color to inherit
here because we've already unset everything above.
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.
Oh, good catch, I'll tighten these up 👍
Thanks again @tellthemachines and @kevin940726, I've removed the unneeded button styles resets already covered by |
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.
I didn't look into the code, but the testing changes look good to me 👍 .
I'm wrapping up for the day now, but just added the backport to WP Beta/RC label in case folks are hoping to get this in for the next beta/RC. |
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.
This tests very well!
Verified with various themes and shadow definitions. It doesn't show any issues. 🚢
Update: based on the suggestion from @ntsekouras (#48664 (comment)) I've swapped out the This appears to give us the best of both worlds in that we avoid the invalid HTML of rendering buttons within buttons (since 3rd party blocks are likely to contain them), while also getting the keyboard behaviour from I'm not too sure if I've gotten the On balance, I think |
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.
It's still working well for me!
A quick test with VoiceOver yielded the same results as trunk, but it might be good to test on Windows too if anyone has the setup for it. What I'm unsure about is to what extent having a div with a button role is any better than having an actual button, especially as regards its possible interactive content. I strongly suspect none of these changes will make things any worse than they already are on trunk though 😅
className={ classnames( 'edit-site-style-book__example', { | ||
'is-selected': isSelected, | ||
} ) } | ||
id={ id } |
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.
Is the id being used for anything?
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.
Yep! It's a little weird, but I followed the suggestions from the reakit docs here that recommend it as a performance enhancement when using CompositeItem
components when you're concerned about performance. I figured since we're dealing with previews and we want to avoid re-renders where we can, it'd be a good one to include. It's the first time I've used it, though, so very happy for feedback if I've missed anything 😄
I've done a couple of tiny updates, thanks to suggestions from @Mamaduka. Also, I've included an I think this is in a good shape to land now, and it's an improvement over what we have in trunk as @tellthemachines mentions. If there aren't any objections, I'll aim to merge this PR in tomorrow my time, but feel free to merge @Mamaduka or @ntsekouras if this looks okay and if it makes the backporting process easier to have it in sooner. Thanks again for all the reviews and feedback, everybody! 🙇 |
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.
Thank you @andrewserong, this looks good! It would be good to see if we can find a better solution for the extra iframe tab stop, but for now this is an improvement against trunk.
…t overflow block previews (#48664) * Style Book: Try moving iframe to root of content area * Try to match styles inside the iframe to the existing text styles from Gutenberg * Restore margin rules to prevent vertical margins on previews from affecting the height of the preview * Fix svg filters, attempt to fix tabbing behaviour * Add Enter key handling, button role, update e2e tests * Tidy hard-coded styling rules * Switch back to button element, add additional focus styles, update e2e tests * Remove unneeded CSS rules * Update e2e test * Improve caching of blocks * Try swapping out button element for Composite and CompositeItem * Update id key, remove unnecessary memo call
I just cherry-picked this PR to the wp/6.2 branch to get it included in the next release: 3082500 |
What?
Fixes: #48392
Alternative to #48470, this PR looks at moving the iframe for the Style Book up to the root of the content area, and uses editor context / block list at the example level, so that the styles affect and are displayed across the whole content area of the Style Book rather than just within each particular block example.
Why?
Styles need to affect an area beyond just their preview — the idea in this PR is that (hopefully) we can get the styles to affect the entire content area so that it more closely resembles the layout of the editor.
How?
Note / caveat: because of lifting the iframe up to the root of the content area, I had to set the iframe to
tabIndex=0
so that the buttons within it could be tabbed to. This seems to work pretty well, but means that the iframe itself now sits within the tab sequence of the style book. Happy for any suggestions on how to improve this.Testing Instructions
trunk
, and that the block headings still match the styling in GutenbergFor code review: double-check whether or not I've included all the bits required from the block previews over to the examples here.
Testing Instructions for Keyboard
Screenshots or screencast
Button block with a large shadow applied in a site with a gradient set as the site's background color: