Hosting Command Palette: Trim double quotes from selected items if present #84373

katinthehatsite · 2023-11-21T14:20:07Z

Related to https://github.com/Automattic/dotcom-forge/issues/4550

Proposed Changes

This PR trims the double quotes from the selected site title before it is processed by the document.querySelector. This ensures that the command palette does not break if the site titles include double quotes.

This is what the error looks like if there are double quotes present before they are being processed by the document.querySelector in the command palette:

Testing Instructions

Ensure that you have an Atomic site
Set the site title under Settings > General on an Atomic site to include double quotes e.g. "My Site"
First, test on https://wpcalypso.wordpress.com/sites by triggering cmd + k on Mac and ctrl + k on Windows
Navigate to one of the SSH commands e.g. Open SSH details
Try navigating to the site with double quotes in the site title
Confirm this breaks the command palette
Next, navigate to Calypso Live Link generated in this PR
Trigger command palette with cmd + k on Mac and ctrl + k on Windows
Try navigating to the site with double quotes in the site title
Confirm that the command executes as expected

Pre-merge Checklist

Has the general commit checklist been followed? (PCYsg-hS-p2)
https://wpcalypso.wordpress.com/devdocs/docs/testing/index.md for your changes?
Have you tested the feature in Simple (P9HQHe-k8-p2), Atomic (P9HQHe-jW-p2), and self-hosted Jetpack sites (PCYsg-g6b-p2)?
Have you checked for TypeScript, React or other console errors?
Have you used memoizing on expensive computations? More info in Memoizing with create-selector and Using memoizing selectors and Our Approach to Data
Have we added the "[Status] String Freeze" label as soon as any new strings were ready for translation (p4TIVU-5Jq-p2)?
For changes affecting Jetpack: Have we added the "[Status] Needs Privacy Updates" label if this pull request changes what data or activity we track or use (p4TIVU-ajp-p2)?

github-actions · 2023-11-21T14:20:24Z

Calypso Live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-88484

Jetpack Cloud live (direct link)

	https://calypso.live?image=registry.a8c.com/calypso/app:build-88484&env=jetpack

matticbot · 2023-11-21T14:21:31Z

Here is how your PR affects size of JS and CSS bundles shipped to the user's browser:

Sections (~44 bytes added 📈 [gzipped])

name                     parsed_size           gzip_size
sites-dashboard               +318 B  (+0.1%)      +57 B  (+0.0%)
import-hosted-site-flow       +318 B  (+0.0%)      +72 B  (+0.0%)
import-flow                   +318 B  (+0.0%)      +72 B  (+0.0%)

Sections contain code specific for a given set of routes. Is downloaded and parsed only when a particular route is navigated to.

Legend

What is parsed and gzip size?

Parsed Size: Uncompressed size of the JS and CSS files. This much code needs to be parsed and stored in memory.
Gzip Size: Compressed size of the JS and CSS files. This much data needs to be downloaded over network.

Generated by performance advisor bot at iscalypsofastyet.com.

sejas

Wow!, great catch @katinthehatsite . 🙌

This PR solves the issue that I was able to replicate on production. I also tried adding other characters besides the quote, like [, ], but they worked fine.

We should upstream this change.
https://github.com/WordPress/gutenberg/blob/trunk/packages/commands/src/components/command-menu.js#L161C17-L164

I confirm that having a site with quotes or other weird characters don't break the command palette. And I was able to navigate to the SSH details. 👍

Screencast

quotes.on.command.palette.mp4

danielbachhuber · 2023-11-21T17:37:19Z

client/components/command-pallette/wpcom-command-pallette.tsx

@@ -69,16 +69,23 @@ interface CommandInputProps {
 function CommandInput( { isOpen, search, setSearch }: CommandInputProps ) {
 	const commandMenuInput = useRef< HTMLInputElement >( null );
 	const _value = useCommandState( ( state ) => state.value );
+	const sanitizedValue = useMemo( () => {
+		const removedQuotesValue = _value.replace( /"/g, '' ); // Remove double quotes from any selected items before processing input


Should we use escapeAttribute from @wordpress/escape-html? https://developer.wordpress.org/block-editor/reference-guides/packages/packages-escape-html/#escapeattribute

Or how do we match the sanitization the library is already applying, to make sure our value exactly matches what's rendered on the page?

Also, what other characters might cause problems?

Also, what other characters might cause problems?

Yesterday reviewing the PR I found that the double quotes character is problematic. The rest worked fine.

sejas · 2023-11-22T10:54:03Z

@katinthehatsite, let's pair on this.
All this logic is for accessibility improvements and should be implemented directly on cmdk. I think we can get the id withtout accessing the DOM elements at all.

sejas · 2023-11-22T11:38:39Z

For context:
The accessibility issue was fixed on cmdk but there hasn't been a new release since March. Therefore those have the same risk.

sejas · 2023-11-22T12:43:57Z

Hey Kat, I created this PR using yours as a base. We are taking a different/better approach from cmdk and @wordpress/commands #84412

…ters (#84412) * Use the value slugified for item id and activedesdcendant * calculate slug only when the value changes

katinthehatsite · 2023-11-22T20:23:03Z

client/components/command-pallette/wpcom-command-pallette.tsx

 						onSelect={ () => command.callback( { close, setSearch } ) }
-						id={ command.name }
+						id={ cleanForSlug( itemValue ) }


I like this solution with cleanForSlug! TIL: https://developer.wordpress.org/block-editor/reference-guides/packages/packages-url/#cleanforslug

katinthehatsite · 2023-11-22T20:24:16Z

@sejas - thanks for all the research and the refactor. I think if we can make the fix in Gutenberg, this is definitely the approach that we should be taking!

…m-double-quotes-in-command-palette

…esent (#84373) * Use the slugified value for item id and activedesdcendant --------- Co-authored-by: Kateryna Kodonenko <kateryna@automattic.com> Co-authored-by: Antonio Sejas <antonio@sejas.es>

Remove double quotes from site title if present

80e13a8

katinthehatsite self-assigned this Nov 21, 2023

katinthehatsite added the Hosting Command Palette label Nov 21, 2023

katinthehatsite added the [Type] Bug label Nov 21, 2023

katinthehatsite requested a review from a team November 21, 2023 14:34

matticbot added the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 21, 2023

katinthehatsite requested a review from sejas November 21, 2023 14:34

Remove unnecessary trim

d5e8cd8

sejas approved these changes Nov 21, 2023

View reviewed changes

danielbachhuber reviewed Nov 21, 2023

View reviewed changes

sejas mentioned this pull request Nov 22, 2023

Hosting Command Palette: fix active descendant id for reserved characters #84412

Merged

7 tasks

Hosting Command Palette: fix active descendant id for reserved charac…

fd8dae6

…ters (#84412) * Use the value slugified for item id and activedesdcendant * calculate slug only when the value changes

katinthehatsite commented Nov 22, 2023

View reviewed changes

Merge branch 'trunk' of github.com:Automattic/wp-calypso into fix/tri…

ad6e24a

…m-double-quotes-in-command-palette

sejas merged commit f916efc into trunk Nov 23, 2023
12 checks passed

sejas deleted the fix/trim-double-quotes-in-command-palette branch November 23, 2023 11:41

github-actions bot removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Nov 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Hosting Command Palette: Trim double quotes from selected items if present #84373

Hosting Command Palette: Trim double quotes from selected items if present #84373

katinthehatsite commented Nov 21, 2023 •

edited

Loading

github-actions bot commented Nov 21, 2023 •

edited by matticbot

Loading

matticbot commented Nov 21, 2023 •

edited

Loading

sejas left a comment •

edited

Loading

danielbachhuber Nov 21, 2023

sejas Nov 22, 2023 •

edited

Loading

sejas commented Nov 22, 2023 •

edited

Loading

sejas commented Nov 22, 2023 •

edited

Loading

sejas commented Nov 22, 2023 •

edited

Loading

katinthehatsite Nov 22, 2023

katinthehatsite commented Nov 22, 2023

Hosting Command Palette: Trim double quotes from selected items if present #84373

Hosting Command Palette: Trim double quotes from selected items if present #84373

Conversation

katinthehatsite commented Nov 21, 2023 • edited Loading

Proposed Changes

Testing Instructions

Pre-merge Checklist

github-actions bot commented Nov 21, 2023 • edited by matticbot Loading

matticbot commented Nov 21, 2023 • edited Loading

sejas left a comment • edited Loading

Choose a reason for hiding this comment

Screencast

danielbachhuber Nov 21, 2023

Choose a reason for hiding this comment

sejas Nov 22, 2023 • edited Loading

Choose a reason for hiding this comment

sejas commented Nov 22, 2023 • edited Loading

sejas commented Nov 22, 2023 • edited Loading

sejas commented Nov 22, 2023 • edited Loading

katinthehatsite Nov 22, 2023

Choose a reason for hiding this comment

katinthehatsite commented Nov 22, 2023

katinthehatsite commented Nov 21, 2023 •

edited

Loading

github-actions bot commented Nov 21, 2023 •

edited by matticbot

Loading

matticbot commented Nov 21, 2023 •

edited

Loading

sejas left a comment •

edited

Loading

sejas Nov 22, 2023 •

edited

Loading

sejas commented Nov 22, 2023 •

edited

Loading

sejas commented Nov 22, 2023 •

edited

Loading

sejas commented Nov 22, 2023 •

edited

Loading