-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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
Conversation
Size Change: +40 B (0%) Total Size: 1.24 MB
ℹ️ View Unchanged
|
The unit tests are failing due to However, this PR moves the escaping to render time so as to selectively choose whether it's appropriate to escape or not depending on context. We should be able to remove the test. |
@draganescu as you reviewed #18617 would you mind taking a look at this one? |
Why don't we decode in the span? |
Decoding is actually only required when we're outputting the We're still escaping when we're putting stuff into the |
4ea320a
to
2a73216
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
const escapeTitle = | ||
title !== '' && | ||
normalizedTitle !== normalizedURL && | ||
originalLabel !== title; |
There was a problem hiding this comment.
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.
I think we should include an e2e test here as well. |
a053386
to
28eda09
Compare
I really appreciate the review. I'm having second thoughts about the approach here as I have uncovered some further potential bugs.
I've walked through all that with Dave and we concluded that the escaping model is inverted. We store the escaped content in the block attributes, then recover the raw data in JS. Instead, I'd like 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. Now, inverting the escaping model is quite a large project. I think the rendering fix @getdave proposes here is a perfectly good solution for starters. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
E2E failures seem unrelated and the code works and makes sense. Let's leave a note explaining why it's all so confusing and 🚢
Noting that we may need to revert this pending further investigation.
Implemented following discussion with @youknowriad
08d8c2f
to
8703fa2
Compare
…p-tests-config * 'trunk' of github.com:WordPress/gutenberg: (88 commits) Components: refactor `AlignmentMatrixControl` to pass `exhaustive-deps` (#41167) [RNMobile] Add 'Insert from URL' option to Image block (#40334) [RNMobile] Improvements to Getting Started Guides (#40964) Post Author Name: Add to and from Post Author transformations (#41151) CheckboxControl: Add unit tests (#41165) Improve inline documentation (#41209) Mobile Release v1.76.1 (#41196) Use explicit type definitions for entity configuration (#40995) Scripts: Convert file extension to js in `block.json` during build (#41068) Reflects revert in 6446878 (#41221) get_style_nodes should be compatible with parent method. (#41217) Gallery: Opt-in to axial (column/row) block spacing controls (#41175) Table of Contents block: convert line breaks to spaces in headings. (#41206) Add support for button elements to theme.json (#40260) Global Styles: Load block CSS conditionally (#41160) Update URL (#41188) Improve autocompleter performance (#41197) Site Editor: Set min-width for styles preview (#41198) Remove Navigation Editor screen from experiments page (#40878) Fix broken Page title for pages created inline within in Nav block (#41063) ...
What?
Fixes #30111.
Fixes the output of decoded and/or escaped HTML entities in the Nav block when creating a link (page) from within the Link UI that contains chars such as
&
.Why?
There are two facets to this PR:
escape
ing of the label for draft postsDisplay of encoded HTML entities
Calling
saveEntityRecord
results in an API response which contains atitle
field with the following:The
handleCreate
function in the Nav Link block consumes thetitle.rendered
field of the newly created Page post type.gutenberg/packages/block-library/src/navigation-link/edit.js
Lines 606 to 612 in b495580
This field contains a version of the post title which has been passed through various WordPress display filters and thus contains HTML entities (e.g.
Yes & No
).We then render this directly into the Nav link block which means we "see" the entities.
To fix this we now call
decodeEntities
on therendered
field. We could just use theraw
field but the current approach is more consistent with the implementation infetchLinkSuggestions
which is also consuming therendered
field:gutenberg/packages/core-data/src/fetch/__experimental-fetch-link-suggestions.js
Lines 212 to 218 in a1e1fdc
Unwanted
escape
ing of the label for draft postsThe output of the Nav Link block's
label
attribute is handled differently depending on the post status of the post represented by the link.Posts with a
Published
status they are output into a<RichText>
field:gutenberg/packages/block-library/src/navigation-link/edit.js
Lines 741 to 745 in b83ce72
Posts with a
Draft
status are output into a<span>
.gutenberg/packages/block-library/src/navigation-link/edit.js
Lines 798 to 803 in b83ce72
Unfortunately the following code from
updateNavigationLinkBlockAttributes
which updates the Navigation Link block attributes escapes the title before it is output to the DOM. Indeed, the escaping model here appears to have become inverted, with the content being escaped for storage in block attributes (and ultimately in the database) when it would have been preferable to escape (or not!) at the point of output.gutenberg/packages/block-library/src/navigation-link/edit.js
Lines 242 to 244 in b83ce72
The result, is that for draft posts being rendered into a
<span>
we see an escaped version of the label attribute (e.g.Yes & No
).This doesn't happen for posts with a
published
status because they are rendered into aRichText
and thus the escaping isn't "visible" to the user.To fix this we have to "restore" the data by
unescape
ing the label for draft posts.How?
This PR fixes things by:
handleCreate
making it consistent withfetchLinkSuggestions
.label
attribute before it is output in the<span>
for Draft pages.Testing Instructions
&
.&
is still rendered correctly.Screenshots or screencast
Screen.Capture.on.2022-05-13.at.21-22-27.mov