Fix scrollbars on pattern transforms #55069
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
talldan
added
[Type] Bug
|
Size Change: +54 B (0%) Total Size: 1.65 MB
ℹ️ View Unchanged
|
talldan
commented
Oct 5, 2023
| @@ -123,7 +125,6 @@ | |||
| margin: $grid-unit-20 0; | |||
| // Use padding to prevent the pattern previews focus style from being cut-off. | |||
| padding: 0 $grid-unit-20; | |||
| overflow: hidden; | |||
There was a problem hiding this comment.
Looks like I may be undoing my own fix by removing this - #44079.
I think the root issue is that there's a sharing of classnames between two components that do very different things.
There was a problem hiding this comment.
Have resolved this by using a modifier classname unset to remove the overflow only for the patterns menu.
Unpicking the tech debt is unfortunately going to be time consuming, and might not be the best use of time.
|
Thanks for the PR!
I think so too. I think that major refactoring is necessary to fundamentally solve this problem, so I think that using a class with an When I tried to solve this problem, I was thinking of fixing the "Preview" label to the top. This is to match the pattern of the main block inserter. I think this PR needs design feedback, so I'd like to ping @richtabor, who reported this issue. Below is a recording of how I temporarily changed the style from the browser console and the Preview label is stuck on top. 52836d5d5abd84820fe21144768f64e6.mp4 |
|
Good point about the title, not sure I even noticed it. I wonder if we should revisit the copy? It being 'Preview' singular doesn't really seem to fit, given there can be multiple patterns. Perhaps it should say 'Patterns', or maybe it's not needed at all 🤔 |
That's the direction I lean. |
t-hamano
approved these changes
Oct 9, 2023
There was a problem hiding this comment.
I would like to approve this PR because I think it originally fixes the issue with scrolling itself and should be backported to WP6.4.
Looking at the GIF image #30469 where this feature was implemented, it looks like it was scrolling correctly at first. Changes to the block editor or components might have caused this issue.
Another issue I discovered was that I was unable to properly exit the pattern preview using the keyboard. If you access the Pattern popover preview with just the keyboard and press the Escape key, the block will lose focus.
713c4235d26d5cd92094e62f67c62544.mp4
In any case, this component may need to be refactored with accessibility in mind. We can also consider what the title should be at that time.
If you want to keep the title, "Patterns List" might be an idea. This is also specified as aria-label in the pattern list.
I don't think it's a problem to remove the title. As mentioned earlier, the pattern list has an aria-label, so the screen reader is reading out what it is, as you can see in the screencast below.
cdb2d4830bf579e148c8c053e9ba051c.mp4
|
@t-hamano Thanks for the thorough review. I think I broke it in #44079, quite a while ago. I've removed the title text as suggested. There are a few other issues documented in #50589, but I don't think the problem with exiting the popover was mentioned, so it might be good to add to it or make a new issue. Noting that it does seem like a fairly non-standard implementation, the Patterns menu item should probably also have |
|
Flaky tests detected in 17940e995582047482c74f22bba6d5f002800346. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6464187887
|
talldan
force-pushed
the
fix/pattern-transform-scrollbars
branch
from
17940e9
to
7002c18
Compare
mikachan
pushed a commit
that referenced
this pull request
Oct 10, 2023
* Fix scrollbars on pattern transforms * Fix single pattern previews * Improve classname semantics * Remove modal title
|
I just cherry-picked this PR to the 6.4-beta3-2 branch to get it included in the next release: 9468195 |
mikachan
removed
the
Backport to WP 6.7 Beta/RC
karmatosed
pushed a commit
that referenced
this pull request
Oct 10, 2023
* Site Editor Styles Screen: Fix dancing styles previews (#55183) * Pulling across changes from WordPress/wordpress-develop#5441 (#55181) Removed var * Add `aria-label` attribute to navigation block only when it is open (#54816) * Add `aria-label` only when is open * Remove unnecessary `label` property in edit * Escape translation * Move navigation context to `wp_json_encode` * Add `wp_json_encode` flags * Paste: fix MS Word list paste (#55127) * Paste: fix MS Word list paste * Match mso-list:Ignore * Fix inline paste * Fix scrollbars on pattern transforms (#55069) * Fix scrollbars on pattern transforms * Fix single pattern previews * Improve classname semantics * Remove modal title * Reset styles on window resize (#55077) Co-authored-by: Ricardo Artemio Morales <ric.morales22@gmail.com> --------- Co-authored-by: Andrew Serong <14988353+andrewserong@users.noreply.github.com> Co-authored-by: Ramon <ramonjd@users.noreply.github.com> Co-authored-by: Mario Santos <34552881+SantosGuillamot@users.noreply.github.com> Co-authored-by: Ella <4710635+ellatrix@users.noreply.github.com> Co-authored-by: Daniel Richards <daniel.richards@automattic.com> Co-authored-by: Ricardo Artemio Morales <ric.morales22@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Fixes #49289
The popover for pattern transforms had some unusual behaviour when scrolling.
Why?
The CSS overflow rules were not quite right, and that caused a scrollbar to show in the main viewport rather than in the popover. Scrolling would cause the popover to struggle to position itself.
How?
Fix the overflow rules. The popover itself should have no overflow, while the content is scrollable so has
overflow: auto.As an aside, having a big list of these huge previews in a small popover isn't really the best UX, so it might be something to improve upon.
Testing Instructions
In trunk: The popover isn't scrollable and the popover bounces around
In this branch: The popover is scrollable and doesn't bounce
It's also worth testing that there are no knock-on effects with other previews, I had to make sure #44079 didn't regress by solving this. To test that:
Testing Instructions for Keyboard
The original issue isn't reproducible using only a keyboard, it's more of a mouse user bug.
Screenshots or screencast
Before
Kapture.2023-10-05.at.11.30.04.mp4
After
Kapture.2023-10-05.at.11.27.28.mp4