Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix broken Page title for pages created inline within in Nav block #41063

Merged
merged 12 commits into from
May 20, 2022
40 changes: 35 additions & 5 deletions packages/block-library/src/navigation-link/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
* External dependencies
*/
import classnames from 'classnames';
import { escape } from 'lodash';
import { escape, unescape } from 'lodash';

/**
* WordPress dependencies
Expand Down Expand Up @@ -43,6 +43,7 @@ import {
import { placeCaretAtHorizontalEdge } from '@wordpress/dom';
import { link as linkIcon, addSubmenu } from '@wordpress/icons';
import { store as coreStore } from '@wordpress/core-data';
import { decodeEntities } from '@wordpress/html-entities';

/**
* Internal dependencies
Expand Down Expand Up @@ -239,6 +240,15 @@ export const updateNavigationLinkBlockAttributes = (
normalizedTitle !== normalizedURL &&
originalLabel !== title;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so don’t escape if:

  • title is empty
  • the link already has a label which is not the title
  • we obtained a title, so we don’t use the URL

probably the conditions here are to avoid double escaping? IDK.


// Unfortunately this causes the escaping model to be inverted.
// The escaped content is stored in the block attributes (and ultimately in the database),
// and then the raw data is "recovered" when outputting into the DOM.
// It would be preferable to store the **raw** data in the block attributes and escape it in JS.
// Why? Because there isn't one way to escape data. Depending on the context, you need to do
// different transforms. It doesn't make sense to me to choose one of them for the purposes of storage.
// See also:
// - https://github.com/WordPress/gutenberg/pull/41063
// - https://github.com/WordPress/gutenberg/pull/18617.
const label = escapeTitle
? escape( title )
: originalLabel || escape( normalizedURL );
Expand Down Expand Up @@ -606,7 +616,17 @@ export default function NavigationLinkEdit( {
return {
id: page.id,
type: postType,
title: page.title.rendered,
// Make `title` property consistent with that in `fetchLinkSuggestions` where the `rendered` title (containing HTML entities)
// is also being decoded. By being consistent in both locations we avoid having to branch in the rendering output code.
// Ideally in the future we will update both APIs to utilise the "raw" form of the title which is better suited to edit contexts.
// e.g.
// - title.raw = "Yes & No"
// - title.rendered = "Yes & No"
// - decodeEntities( title.rendered ) = "Yes & No"
// See:
// - https://github.com/WordPress/gutenberg/pull/41063
// - https://github.com/WordPress/gutenberg/blob/a1e1fdc0e6278457e9f4fc0b31ac6d2095f5450b/packages/core-data/src/fetch/__experimental-fetch-link-suggestions.js#L212-L218
title: decodeEntities( page.title.rendered ),
url: page.link,
kind: 'post-type',
};
Expand Down Expand Up @@ -795,10 +815,20 @@ export default function NavigationLinkEdit( {
text={ tooltipText }
>
<>
<span>
<span
aria-label={ __(
'Navigation link text'
) }
>
{
/* Trim to avoid trailing white space when the placeholder text is not present */
`${ label } ${ placeholderText }`.trim()
// Some attributes are stored in an escaped form. It's a legacy issue.
// Ideally they would be stored in a raw, unescaped form.
// Unescape is used here to "recover" the escaped characters
// so they display without encoding.
// See `updateNavigationLinkBlockAttributes` for more details.
`${ unescape(
label
) } ${ placeholderText }`.trim()
}
</span>
<span className="wp-block-navigation-link__missing_text-tooltip">
Expand Down
20 changes: 0 additions & 20 deletions packages/block-library/src/navigation-link/test/edit.js
Original file line number Diff line number Diff line change
Expand Up @@ -372,26 +372,6 @@ describe( 'edit', () => {
url: 'https://wordpress.org',
} );
} );
// https://github.com/WordPress/gutenberg/pull/18617
it( 'label is javascript escaped', () => {
const setAttributes = jest.fn();
const linkSuggestion = {
opensInNewTab: false,
title: '<Navigation />',
type: 'URL',
url: 'https://wordpress.local?p=1',
};
updateNavigationLinkBlockAttributes(
linkSuggestion,
setAttributes
);
expect( setAttributes ).toHaveBeenCalledWith( {
opensInNewTab: false,
label: '&lt;Navigation /&gt;',
kind: 'custom',
url: 'https://wordpress.local?p=1',
} );
} );
// https://github.com/WordPress/gutenberg/pull/19679
it( 'url when escaped is still an actual link', () => {
const setAttributes = jest.fn();
Expand Down
52 changes: 52 additions & 0 deletions packages/e2e-tests/specs/editor/blocks/navigation.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -807,6 +807,58 @@ describe( 'Navigation', () => {
);
} );

it( 'correctly decodes special characters in the created Page title for display', async () => {
await createNewPost();
await insertBlock( 'Navigation' );
const startEmptyButton = await page.waitForXPath( START_EMPTY_XPATH );
await startEmptyButton.click();
const appender = await page.waitForSelector(
'.wp-block-navigation .block-list-appender'
);
await appender.click();

// Wait for URL input to be focused
// Insert name for the new page.
const pageTitle = 'This & That & Some < other > chars';
const input = await page.waitForSelector(
'input.block-editor-url-input__input:focus'
);
await input.type( pageTitle );

// When creating a page, the URLControl makes a request to the
// url-details endpoint to fetch information about the page.
// Because the draft is inaccessible publicly, this request
// returns a 404 response. Wait for the response and expect
// the error to have occurred.
const createPageButton = await page.waitForSelector(
'.block-editor-link-control__search-create'
);
const responsePromise = page.waitForResponse(
( response ) =>
response.url().includes( 'url-details' ) &&
response.status() === 404
);
const createPagePromise = createPageButton.click();
await Promise.all( [ responsePromise, createPagePromise ] );

await waitForBlock( 'Navigation' );

const innerLinkBlock = await waitForBlock( 'Custom Link' );

const linkText = await innerLinkBlock.$eval(
'[aria-label="Navigation link text"]',
( element ) => {
return element.innerText;
}
);

expect( linkText ).toContain( pageTitle );

expect( console ).toHaveErroredWith(
'Failed to load resource: the server responded with a status of 404 (Not Found)'
);
} );

it( 'renders buttons for the submenu opener elements when the block is set to open on click instead of hover', async () => {
await createClassicMenu( { name: 'Test Menu 2' }, menuItemsFixture );
await createNewPost();
Expand Down