Preload: fix multiple regressions around global styles #66468
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
ellatrix
added
the
[Type] Regression
|
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. |
|
Flaky tests detected in ece965d. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/11517897274
|
ellatrix
force-pushed
the
fix/preload-regression
branch
from
cdd9d9e to
12efb60
Compare
|
Size Change: +14 B (0%) Total Size: 1.81 MB
ℹ️ View Unchanged
|
ellatrix
changed the title
|
Thanks for looking at this. It was something @aaronrobertshaw and I wanted to come back to but we got desperately sidetracked. LGTM Site and post editor (admin role)Before
After
Post editor (editor role)Before
After
|
ramonjd
approved these changes
Oct 25, 2024
ramonjd
added
Global Styles
|
@ramonjd What's in the table? Are these preloaded requests? Or requests the editor is making still? |
|
Are there some specific test instructions I can follow here? On trunk, I can see the problematic requests noted in the PR description within the dev tools network log. After checking out this PR, I no longer see them. I might just be confused as to what's expected in light of Ramon's results above. So far I've checked using:
The behaviour at least appears consistent for me though. |
Sorry, that's embarrassing. I was looking at the network requests in the browser between branches, but now I think my env was kaputt, and so was my brain. 😅 Anyway I nuked and updated my env, and ran the tests again. I am now seeing that the global styles requests in question are being preloaded through string(35) "/wp/v2/global-styles/5?context=edit"
string(57) "/wp/v2/global-styles/themes/twentytwentyfive?context=view"
string(68) "/wp/v2/global-styles/themes/twentytwentyfive/variations?context=view"
string(22) "/wp/v2/global-styles/5"The corresponding clientside network requests are no longer being fired as Aaron notes. ✅ |
|
I think the E2E fails are real. This PR breaks the patterns view for hybrid themes. For example, activate a hybrid theme and visit |
ramonjd
reviewed
Oct 28, 2024
Comment on lines
+58
to
+61
| // We want the global styles ID request to finish before triggering | ||
| // the OPTIONS request for user capabilities, otherwise it will | ||
| // fetch `/wp/v2/global-styles` instead of | ||
| // `/wp/v2/global-styles/{id}`! |
There was a problem hiding this comment.
Suggested change
| // We want the global styles ID request to finish before triggering | |
| // the OPTIONS request for user capabilities, otherwise it will | |
| // fetch `/wp/v2/global-styles` instead of | |
| // `/wp/v2/global-styles/{id}`! | |
| /* | |
| * We want the global styles ID request to finish before triggering | |
| * the OPTIONS request for user capabilities, otherwise it will | |
| * fetch `/wp/v2/global-styles` instead of `/wp/v2/global-styles/{id}`. | |
| */ |
There was a problem hiding this comment.
|
Thanks for the reviews! |
|
Great stuff! Thanks for fixing it 🙇🏻 |
|
Since this fixes a regression, let's have a crack at getting it into 6.7. I can migrate the compat layer to 6.7 if it makes it. |
andrewserong
added
the
Backport to WP 6.7 Beta/RC
|
There was a conflict while trying to cherry-pick the commit to the wp/6.7 branch. Please resolve the conflict manually and create a PR to the wp/6.7 branch. PRs to wp/6.7 are similar to PRs to trunk, but you should base your PR on the wp/6.7 branch instead of trunk. |
ramonjd
added a commit
that referenced
this pull request
Oct 29, 2024
Co-authored-by: ellatrix <[email protected]> Co-authored-by: ramonjd <[email protected]> Co-authored-by: aaronrobertshaw <[email protected]>
|
Cherry picked here: #66542 |
ramonjd
added a commit
that referenced
this pull request
Oct 29, 2024
…tyles (#66468) (#66542) * Sync with #66543 * Move backport log to 6.7 --------- Co-authored-by: ellatrix <[email protected]> Co-authored-by: ramonjd <[email protected]> Co-authored-by: aaronrobertshaw <[email protected]> Co-authored-by: andrewserong <[email protected]>
|
I cherry picked this PR to wp/6.7 in #66542 |
ramonjd
removed
the
Backport to WP 6.7 Beta/RC
kevin940726
added
the
Backported to WP Core
karthick-murugan
pushed a commit
to karthick-murugan/gutenberg
that referenced
this pull request
Nov 13, 2024
Co-authored-by: ellatrix <[email protected]> Co-authored-by: ramonjd <[email protected]> Co-authored-by: aaronrobertshaw <[email protected]>
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?
Avoids 5 blocking requests for Site Editor loading:
There are multiple things going wrong after #65071 (cc @ramonjd):
/wp/v2/global-stylesand one at/wp/v2/global-styles/{id}.editandviewcontext. This shouldn't be happening.editcontext and (2) WITHOUT any context, even though Global Styles: allow read access to users withedit_postscapabilities #65071 changed the request from no context at all toview.view.edit_postscapabilities #65071, but global styles variations are not preloaded.To fix all the issues we should:
vieworeditcontext. If we neededitfor example, we shouldn't be fetching `view unnecessarily.viewcontext to the theme global styles path for preloading and remove the old path.Why?
Optimise site editor loading.
How?
Testing Instructions
Check the network tab for these requests.
Testing Instructions for Keyboard
Screenshots or screencast