Closed
Conversation
tellthemachines
requested review from
andrewserong,
jasmussen,
ramonjd and
talldan
tellthemachines
added
the
[Type] Bug
tellthemachines
added
Backport to WP 7.0 Beta/RC
|
Size Change: +8 B (0%) Total Size: 6.89 MB
ℹ️ View Unchanged
|
|
Flaky tests detected in 1e41bac. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/22983720690
|
Contributor
andrewserong
left a comment
andrewserong
left a comment
There was a problem hiding this comment.
That's fixing it up nicely for me in testing! My main question was whether we should account for all the breakpoints that useViewportMatch supports.
What do you think?
|
|
||
| // Keep simulated viewport width stable at the same ranges as block visibility: | ||
| // mobile < 480, tablet < 782, desktop => 782. | ||
| const DEVICE_BREAKPOINTS = [ 480, 782 ]; // mobile, tablet, desktop |
Contributor
There was a problem hiding this comment.
useViewportMatch internally uses these breakpoints:
gutenberg/packages/compose/src/hooks/use-viewport-match/index.js
Lines 22 to 31 in 651bade
Would it be worth including all of them, in case any plugins are using useViewportMatch within the editor canvas?
Contributor
Author
There was a problem hiding this comment.
I doubt any plugins would be using useViewportMatch in the canvas because it was broken before #76420 (it always output "desktop") and we never heard any complaints about it 😅
I think here it makes sense to match the breakpoints offered in the preview dropdown, and once we enable extending those breakpoints there should be a single source of truth for available breakpoints somewhere so that we can just use that list everywhere.
Also from a performance point of view, the more breakpoints we have the more potential for jankiness in the resizer 😬
| iframeDocument, | ||
| } ); | ||
|
|
||
| const viewportMatchWidth = getBucketedViewportWidth( scaleContainerWidth ); |
Contributor
There was a problem hiding this comment.
What's the difference between using scaleContainerWidth vs containerWidth? I.e. if zoomed out, would we expect the useViewportMatch to use the scale container or the container? 🤔
Contributor
Author
There was a problem hiding this comment.
They seem to be mostly the same except that when actively resizing scaleContainerWidth lags behind containerWidth a little. So I thought for this purpose scaleContainerWidth should work well and we needn't return the extra value.
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
talldan
reviewed
Mar 12, 2026
Comment on lines
+384
to
+386
| <ViewportWidthProvider | ||
| value={ viewportMatchWidth } | ||
| > |
Contributor
There was a problem hiding this comment.
I think the change means its no longer a ViewportWidthProvider. I can't use it in a component to get the specific viewport width.
Instead it's a BreakpointMatchProvider or something.
Contributor
Author
There was a problem hiding this comment.
Hmm, ViewportWidthProvider is context that is only read by useViewportMatch I don't know if giving it a different name won't just make things more confusing. (useViewportMatch itself deals with breakpoints too)
Contributor
There was a problem hiding this comment.
useViewportMatch seems to read ViewportMatchWidthContext. It gets renamed for some reason, which makes it a bit confusing.
Contributor
Author
There was a problem hiding this comment.
ViewportMatchWidthContext.Provider is exported as __experimentalWidthProvider. i don't know if it was ever used for anything but it doesn't seem to be now, at least in Gutenberg. Maybe plugins use it.
Contributor
There was a problem hiding this comment.
ViewportMatchProvider is much better naming, it's a shame it got conflated with width in that export.
The __experimental prefixed version should probably be deprecated at some point and stabilized, so maybe it can be renamed then.
Contributor
|
I wonder if an alternative could be debouncing the function that updates the value passed to Perhaps the consumers of the context should only be updated when the resizing process comes to a rest. |
Member
|
Just wanted to say that this reduces the jankiness a lot!! BeforeKapture.2026-03-12.at.16.41.06.mp4AfterKapture.2026-03-12.at.16.40.46.mp4I don't have strong opinion about the breakpoints we use (that is, 2 vs the entire set), but the ones used are aligned with block visibility. Maybe we could document that only block-visibility breakpoints are supported for now. |
Contributor
Author
I'd rather stick to the current approach because performance will be better the less the provider is updated and also we should be able to see things change while resizing, as we go past the different breakpoints, and with updating on resize end we'd lose that. |
Contributor
|
Thanks for working on this! In testing this branch, it appears to solve my issue. With the small caveat that it feels like there's a tiny delay after clicking, before animation starts. Here's this branch: this.one.mp4I tried also reverting the older PR to see if that made any difference. I can't actually tell: revert.mp4I'm only comparing with my personal website, where clicking feels much more instantaneous. That's on 6.9 and latest GB officially released. But that's also a simpler theme, that can play a role too. So absent anyone able to spot a difference in the two videos above, this looks like it addresses it for me. |
Contributor
|
Considering where we are in the 7.0 release cycle and the fact that #75156 also introduced another regression (#76424), we have to be careful here. Is there potential for more things to subtly break? Is it possible that a better solution in the first place would altogether avoid setting Stream-of-consciousness idea: what if |
Contributor
Looks promising: #76446 |
Contributor
Author
|
Closing this in favour of #76446 |
t-hamano
removed
the
Backport to WP 7.0 Beta/RC
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
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 issue described in this comment.
#75156 added a ViewportWidthProvider to the editor Iframe and passed it the current frame width value, so that anything inside the editor canvas can have access to the frame width via
useViewportMatch. The problem is that value updates repeatedly when the frame is resizing, causing the choppiness that @jasmussen spotted.This PR updates the value passed to ViewportWidthProvider to only change when certain breakpoints are reached (currently matching the breakpoints used for block visibility, which in the future may be extended to accept theme-defined breakpoints - when that happens we should adjust this logic accordingly)
Testing Instructions
resizing-editor.mp4