Site Editor: Prevent unintended actions on the classic theme #54422
Conversation
t-hamano
added
[Feature] Patterns
t-hamano
changed the title
t-hamano
force-pushed
the
patterns/classic-theme-bc
branch
from
bffbae5
to
b252e42
Compare
|
Size Change: +241 B (0%) Total Size: 1.62 MB
ℹ️ View Unchanged
|
t-hamano
changed the title
There was a problem hiding this comment.
Thanks for continuing the work on this one @t-hamano 👍
The correct links and menu items are showing for me depending on the type of theme.
| Classic | Hybrid |
|---|---|
 |
 |
The only odd thing I noticed was when using a hybrid theme, you can access the template part index page via two ways.
- Appearance > Template Parts
- Patterns > Manage all template parts
Regardless of which way you navigate there, hitting back will take you to wp-admin.
Is there a way we can take the user back to the Patterns page if that is how they got to the template parts index page in the site-editor?
My understanding is that this is expected behavior at this point. As discussed in #54066, all Patterns links are In other words, there is only one way to access the template parts index page: "Appearance > Template Parts". Also, I don't think it is expected at this point to return to the patterns page from the template parts index page. |
If it can be accessed, even if only being typed directly, I'd expect users will. After all, that's how some users have been accessing the We could say there is only one official way to access the template parts index page, but there are two possible ways. So if there is a way we could return to the correct location using the history state or something, that would be a proactive step to avoiding a jarring experience. |
I see, it checks if the page just before accessing the template parts index page was a Pattern page (i.e. whether the user accessed directly to the Patterns page) and if so, back to the Pattern page instead of the dashboard. This is the ideal behaviour, right? |
t-hamano
force-pushed
the
patterns/classic-theme-bc
branch
from
4ac57c7
to
c0b57d0
Compare
t-hamano
force-pushed
the
patterns/classic-theme-bc
branch
from
c0b57d0
to
8a78ac2
Compare
|
I was wondering what approach would be best to determine if the Patterns page was accessed directly. At first, I considered adding that information to the URL parameter and making a determination based on that parameter, but the URL parameter would be overwritten in various places. In the end, I took the following approach:
With this logic, a classic theme with block-template-parts should behave like this:
Screencast86aee6b69ef6f7a72066e19129311674.mp4 |
|
Flaky tests detected in 8a78ac2. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/6205258274
|
I was expecting we'd do something along these lines so that reloading the page didn't interfere with going back to the correct location. From memory there is some two way syncing of the URL that can make things tricky. It should be doable though as we maintain some query string params for navigating pattern pages.
This is an improvement but I do wonder if it is adding too much complexity for a pretty contrived edge case. It might be good to get some fresh eyes here. Perhaps @glendaviesnz and @kevin940726 might have some thoughts? Thanks for your patience here again @t-hamano 👍 |
|
There's a more common way of doing this in general via The trick is to append a The benefit of storing the information in However, as pointed out by @aaronrobertshaw, the two-way syncing gotcha in the site editor might make this more complicated than it should be though. 🤔 |
I think the concern with this approach is that the user has other ways to navigate away from the Patterns page other than clicking on "Manage all template parts" link. Users might type "header" in the Command Palette to go to the template part's editing canvas. Or users might click "Create pattern" to go to the pattern editing canvas. So I'm concerned about whether we can control all of these actions by using history.state. 🤔 |
I tried working with this approach. It should work like this:
Screencasta32a7132be28e4a41c06b04c14c5992d.mp4I don't know if this approach is ideal, but I'd like your input. |
| path: '/wp_template_part/all', | ||
| // If a classic theme that supports template parts accessed | ||
| // the Patterns page directly, preserve that state in the URL. | ||
| didAccessPatternsPage: isTemplatePartsMode ? 1 : undefined, |
There was a problem hiding this comment.
Nit: I know isTemplatePartsMode implicitly implies ! isBlockBasedTheme but to me it's not that obvious from the naming alone 😅 . Do you think we should add ! isBlockBasedTheme just to make it clearer?
There was a problem hiding this comment.
When someone comes to this fresh without much context, I think it could really help to be explicit here and probably doesn't hurt much to make this tweak.
There was a problem hiding this comment.
I see, added by b3963ef 👍
There was a problem hiding this comment.
Tested as advertised for me as well:
With Classic theme:
✅ Could enter http://localhost:8889/wp-admin/site-editor.php?path=%2Fpatterns in the URL bar of browser to access the Patterns page
✅ "Manage all template parts" menu is not displayed
✅ New template parts cannot be created
With Hybrid theme:
✅ Could enter http://localhost:8889/wp-admin/site-editor.php?path=%2Fpatterns in the URL bar of browser to access the Patterns page
✅ "Manage all template parts" menu was displayed and worked as expected
There was a problem hiding this comment.
As @kevin940726 and @glendaviesnz have noted, the navigation once within the site editor looks pretty solid.
Unfortunately, an error gets thrown when trying to search commands in the command palette in the post editor.
If you have the time it might help to freshen up the PR description and test instructions along the lines of your comment above 🙏
Special thanks for the video, I might have missed retesting the command palette without it jogging my memory 🙂
| const { | ||
| params: { path, postType, didAccessPatternsPage }, | ||
| } = useLocation(); |
There was a problem hiding this comment.
This appears to throw an error when using the command palette anywhere except the site editor e.g. post editor, editing custom post type such as the wp_block patterns etc.
There was a problem hiding this comment.
We only need to check for these params while in the site editor, so a simple guard against the undefined location might suffice. What do you think?
There was a problem hiding this comment.
Good spot! Guarding against undefined seems logical to me 👍
|
Thank you everyone for your cooperation 🙇 I think this PR is ready to be merged. The PR description has also been updated. However, a PR for WP Core that allows access to the Site Editor from classic themes has not yet been committed. Should this PR be merged after it is committed in the core? My understanding is that this is a bug fix that prevents unintended behavior, so it should be able to be backported after the Beta1 release. Regarding the core PR, I'm also inviting the core team to review on the |
|
Tested well for me!
I think it's okay to merge this before the core patch. It can be seen as a bug fix and doesn't necessarily depend on the core patch. As long as merging it alone doesn't break any existing behavior, I think we can just yolo merge this. Worst case is reverting it, not a big deal to me 🤷♂️.
Thank you! Much appreciated! |
kevin940726
added
the
Backport to Gutenberg RC
|
Update: The core ticket milestone was assigned 6.4. As @kevin940726 said, just merging this should have no effect on existing behavior. Also, even if the core patch is not applied in WP6.4, I would like to merge this PR as it should be useful in the future! |
* Add action and selector to track access to Patterns page * Add a URL parameter to check if the Patterns page was accessed directly * Revert lib changes * Fix critical error in the Post Editor * Explicitly add `! isBlockBasedTheme` with `isTemplatePartsMode` * Fix critical error in the Post Editor
|
I just cherry-picked this PR to the release/16.7 branch to get it included in the next release: d7cd5cd |
mikachan
removed
the
Backport to Gutenberg RC
* Add action and selector to track access to Patterns page * Add a URL parameter to check if the Patterns page was accessed directly * Revert lib changes * Fix critical error in the Post Editor * Explicitly add `! isBlockBasedTheme` with `isTemplatePartsMode` * Fix critical error in the Post Editor
* Fix the install of system fonts from a font collection (#54713) * Fix set upload dir test (#54762) * Site Editor: Prevent unintended actions on the classic theme (#54422) * Add action and selector to track access to Patterns page * Add a URL parameter to check if the Patterns page was accessed directly * Revert lib changes * Fix critical error in the Post Editor * Explicitly add `! isBlockBasedTheme` with `isTemplatePartsMode` * Fix critical error in the Post Editor * Patterns: Fix back navigation after pattern creation (#54852) * Patterns: Fix category control width in site editor (#54853) * Patterns: Allow non-user patterns under Standard filter (#54756) * Performance Tests: Separate page setup from test (#53808) # Conflicts: # test/performance/specs/post-editor.spec.js * Performance Tests: Support legacy selector for expanded elements (#54690) * Paragraph: Make 'aria-label' consistent with other blocks (#54687) * Paragraph: Make 'aria-label' consistent with other blocks * Update tests * Try using BC labels in performance tests * Move lightbox render function to filter (#54670) * syntax refactor repace strpos with str_contains (#54832) * Font Library: avoid deprected error in test (#54802) * fix deprecated call * removing unwanted line * Fix the ShortcutProvider usage (#54851) Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com> * Image: Ensure `false` values are preserved in memory when defined in `theme.json` (#54639) * Modify conditional when parsing config * Only drop the value if it's undefined. --------- Co-authored-by: Michal Czaplinski <mmczaplinski@gmail.com> * Use "Not synced" in place of "Standard" nomenclature for patterns (#54839) * Standard -> Not synced * Fix broken test --------- Co-authored-by: Glen Davies <glen.davies@automattic.com> * Format Library: Try to fix highlight popover jumping (#54736) * Move mime-type collection generation to a function that can be tested… (#54844) * Move mime-type collection generation to a function that can be tested. Refactored to use that function. * linting changes * Add unit tests to mime type getter * Fixed linting errors * test the entire output array and replace assertTrue by assertEquals * fixing docs --------- Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com> * Ensure lightbox toggle is shown if block-level setting exists (#54878) * Block Editor: Update strings in blocks 'RenameModal' component (#54887) * Footnotes: Add aria-label to return links (#54843) * Add aria-label to footnotes front-end links. * Add visual output. Fix PHPCS errors. * Remove visual changes, fix in follow-up. * Font Library: Changed the OTF mime type expected value to be what PHP returns (#54886) * Changed the OTF mime type expected value to be what PHP returns * add unit test for otf file installation --------- Co-authored-by: madhusudhand <madhusudhan.dollu@gmail.com> * Image: Fix layout shift when lightbox is opened and closed (#53026) * Replace overflow: hidden with JavaScript callback to prevent scrolling * Disable scroll callback on mobile; add comments; fix scrim styles The page jumps around when trying to override the scroll behavior on mobile, so I disabled it altogether. I've also added comments to clarify this and other decisions made around the scroll handling. While testing, I realized that the scrim was completely opaque during the zoom animation, which does not match the design, so I added a new animation specifically for the scrim to fix that. * Add handling for horizontally oriented pages * Move close button so that it's further from scrollbar on desktop * Fix call to handleScroll() and add touch callback to new render method * Improve lightbox experience on mobile To ensure pinch to zoom works as expected when the lightbox is open on mobile, I added logic to disable the scroll override when touch is detected. Without this, the scroll override kicks in whenever one tries to pinch to zoom, and it breaks the experience. I also revised the styles for the scrim to make it opaque, as having content visible outside of the lightbox is distracting when pinching to zoom on a mobile device, and I think having a consistent lightbox across devices will make for the best user experience. * Fix spacing * Add touch directives to block supports * Fix spacing in block supports * Rename attribute for clarity * Update comment * Update comments * Fix spacing --------- Co-authored-by: Ricardo Artemio Morales <ric.morales22@gmail.com> * Font Library: move font uploads to a new tab (#54655) * move font uploads to a new tab in the modal * fix invalid success message and revert the dropzone to modal * add success notice for font uploads * make supported file formats message dynamic based on allowed extensions * update hint text and clear successful message with a fresh upload * Block custom CSS: Fix incorrect CSS when multiple root selectors (#53602) * Block custom CSS: Fix incorrect CSS when multiple root selectors * Fix PHP lint error * Use `scope_selector` and `append_to_selector` method and update unit test * Use `scopeSelector` and `appendToSelector` function and update JS unit test * Update packages/block-editor/src/components/global-styles/test/use-global-styles-output.js Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> * Update packages/block-editor/src/components/global-styles/test/use-global-styles-output.js Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> * Update packages/block-editor/src/components/global-styles/test/use-global-styles-output.js Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> * Update packages/block-editor/src/components/global-styles/test/use-global-styles-output.js Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> * Update packages/block-editor/src/components/global-styles/utils.js Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> * re-trigger CI --------- Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> * Add new e2e test for creating a pattern (#54855) * Use list role instead of listbox for patterns (#54884) * Popover: Fix the styles for components that use emotion within popovers (#54912) # Conflicts: # packages/components/CHANGELOG.md * Footnotes: use core’s meta revisioning if available (#52988) # Conflicts: # packages/block-library/src/footnotes/index.php * Remove base url from link control search results (#54553) * Expose baseURL setting on Post and Site Editors via block settings * Strip baseURL from rendered search results * Only fetch baseURL once in top level component * Simplify implementation to utilise URL parse functions * Improve comment wording to avoid referencing undefined var * Remove superfluous conditional * Decode URL prior to operations * Refactor for readability * Fix where url is not defined * Revert change to filter util * Ensure that filterURLForDisplay always receives a string as an arg * Make e2e test locator less strict * Prefer pipe * Force remove trailing slash * [Site Editor]: Update copy of using the default template in a page (#54728) * Remove overflow: hidden from the entity title h1 in the site editor sidebar (#54769) * Site Editor: Avoid same key warnings in template parts area listings (#54863) * TemplateAreas use template part clientId as key * HomeTemplateDetails use template part clientId as key * Cleanup useSelect hook * Fix ToolSelector popover variant (#54840) * Font Library: refactor endpoint permissions (#54829) * break the checking of user permission and file write permissions * return error 500 if the request to the install fonts endpoint needs write permissions and wordpress doens't have write permission on the server * do not ask file write permission on uninstall endpoint * Standardize the output of install and uninstall fonts endpoints Co-authored-by: Jason Crist <146530+pbking@users.noreply.github.com> Co-authored-by: Jeff Ong <5375500+jffng@users.noreply.github.com> * Handle standardized output from install and uninstall endpoints in the frontend Co-authored-by: Jason Crist <146530+pbking@users.noreply.github.com> Co-authored-by: Jeff Ong <5375500+jffng@users.noreply.github.com> * Update the install and unintall fonts endpoints unit tests for the new standardized output format Co-authored-by: Jason Crist <146530+pbking@users.noreply.github.com> Co-authored-by: Jeff Ong <5375500+jffng@users.noreply.github.com> * fix the refersh call for the library Co-authored-by: Jason Crist <146530+pbking@users.noreply.github.com> Co-authored-by: Jeff Ong <5375500+jffng@users.noreply.github.com> --------- Co-authored-by: Jason Crist <146530+pbking@users.noreply.github.com> Co-authored-by: Jeff Ong <5375500+jffng@users.noreply.github.com> * Don’t use TypeScript files in scripts package (#54856) * Fix Search Block not updating in Nav block (#54823) * Avoid setState in render * Attempt at test coverage * Improve tests and make them work * Remove word-wrap property (#54866) --------- Co-authored-by: Matias Benedetto <matias.benedetto@gmail.com> Co-authored-by: Aki Hamano <54422211+t-hamano@users.noreply.github.com> Co-authored-by: Aaron Robertshaw <60436221+aaronrobertshaw@users.noreply.github.com> Co-authored-by: Bart Kalisz <bartlomiej.kalisz@gmail.com> Co-authored-by: George Mamadashvili <georgemamadashvili@gmail.com> Co-authored-by: Artemio Morales <artemio.morales@a8c.com> Co-authored-by: Riad Benguella <benguella@gmail.com> Co-authored-by: Marin Atanasov <8436925+tyxla@users.noreply.github.com> Co-authored-by: Michal Czaplinski <mmczaplinski@gmail.com> Co-authored-by: Rich Tabor <hi@richtabor.com> Co-authored-by: Glen Davies <glen.davies@automattic.com> Co-authored-by: Jason Crist <jcrist@pbking.com> Co-authored-by: Alex Stine <alex.stine@yourtechadvisors.com> Co-authored-by: madhusudhand <madhusudhan.dollu@gmail.com> Co-authored-by: Carlos Bravo <37012961+c4rl0sbr4v0@users.noreply.github.com> Co-authored-by: Ricardo Artemio Morales <ric.morales22@gmail.com> Co-authored-by: Kai Hao <kevin830726@gmail.com> Co-authored-by: Adam Silverstein <adamjs@google.com> Co-authored-by: Dave Smith <getdavemail@gmail.com> Co-authored-by: Nik Tsekouras <ntsekouras@outlook.com> Co-authored-by: Ramon <ramonjd@users.noreply.github.com> Co-authored-by: Jason Crist <146530+pbking@users.noreply.github.com> Co-authored-by: Jeff Ong <5375500+jffng@users.noreply.github.com> Co-authored-by: Pascal Birchler <pascalb@google.com>
Related to #54066, #52150
Related core ticket: https://core.trac.wordpress.org/ticket/58827
Related core PR: WordPress/wordpress-develop#5201 (comment)
What?
This PR prevents the following three unintended actions in case the Classic theme is allowed to access the Patterns page of the Site Editor.
Why?
As a first step toward making the Patterns page (
site-editor.php?path=/patterns) accessible to all classic themes in the future, we are considering the following in WP6.4:Appearance -> Patternslink for all classic themes that point toedit.php?post_type=wp_block.site-editor.php?path=/patternsaccess for classic themes internally.This means that users of the classic theme will not see the link to the Pattern page (
site-editor.php?path=/patterns) anywhere, but will be able to access it by typing its URL directly into their browser.Therefore, actions that are not expected in that case need to be prevented in advance.
Testing Instructions
To temporarily allow access to the site editor from the classic theme, comment out here in the WordPress core
http://localhost:8889/wp-adminhttp://localhost:8889/wp-admin/site-editor.php?path=%2Fpatternsin the URL bar of your browser to access the Patterns pageAdditional Test
If a classic theme that supports block template parts goes directly to the Patterns page in the Site Editor, the back button link on the All Template Parts page should always go to the Patterns page instead of the Dashboard.
To control this, I introduce a URL parameter called
didAccessPatternsPage.Based on this, please test the following:
didAccessPatternsPageparameter.http://localhost:8889/wp-admin/site-editor.php?path=%2Fpatternsdirectly.didAccessPatternsPageURL parameter is retained even if the browser is reloaded.Cmd+Shift+T.http://localhost:8889/wp-admin/site-editor.php?path=%2Fpatternsdirectly.didAccessPatternsPageparameter, no matter what operation is performed, not to mention that the template parts are not accessible.