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/title encoding bug #19187

Merged
merged 5 commits into from Dec 18, 2019
Merged

Fix/title encoding bug #19187

merged 5 commits into from Dec 18, 2019

Conversation

@glendaviesnz
Copy link
Contributor

glendaviesnz commented Dec 17, 2019

Description

The encoding that was recently added to the post title block works fine on standalone wp installs, but not on wpcom - Automattic/wp-calypso#38362 - this is because for some reason wpcom adds leading zeros to the html entity codes, and lodash unescape does not account for this.

Switching from the lodash unescape to WordPress decodeEntities resolves the issue without causing any problems that I can see.

How has this been tested?

Tested manually, and also added an additional unit test to make sure decodeEntities accounts for leading zeros.

Screenshots

Before:

encode-before

After:

encode-after

Types of changes

Change string decoding method

Fixes Automattic/wp-calypso#38362

@glendaviesnz glendaviesnz requested a review from retrofox Dec 17, 2019
@glendaviesnz glendaviesnz requested a review from talldan as a code owner Dec 17, 2019
@@ -123,7 +123,7 @@ class PostTitle extends Component {
<Textarea
id={ `post-title-${ instanceId }` }
className="editor-post-title__input"
value={ unescape( title ) }
value={ decodeEntities( title ) }

This comment has been minimized.

Copy link
@gwwar

gwwar Dec 17, 2019

Contributor

What are the differences in implementation? Can we add a unit test to catch the broken case here?

This comment has been minimized.

Copy link
@glendaviesnz

glendaviesnz Dec 18, 2019

Author Contributor

Some level of filtering on wpcom converts #39; to #039;. lodash unescape only looks for an exact match of #39;. WP decodeEntities on the other hand, doesn't search for specific entity strings, it just throws everything into a temporary text field in the dom and then extracts, so relies entirely on the browsers own encoding/decoding, which handles both #39; and #039;

This comment has been minimized.

Copy link
@glendaviesnz

glendaviesnz Dec 18, 2019

Author Contributor

Added an additional test to cover this variation in entity codes.

…le quote is accounted for
@glendaviesnz glendaviesnz requested a review from aduth as a code owner Dec 18, 2019
Glen Davies
@aduth
aduth approved these changes Dec 18, 2019
@@ -123,7 +123,7 @@ class PostTitle extends Component {
<Textarea
id={ `post-title-${ instanceId }` }
className="editor-post-title__input"
value={ unescape( title ) }
value={ decodeEntities( title ) }

This comment has been minimized.

Copy link
@aduth

aduth Dec 18, 2019

Member

I wonder a bit about the corresponding escape which happens in the onUpdate dispatch below. In truth, I'm not actually sure why it's needed at all. But at least previously, there was a duality between unescape and escape which is no longer guaranteed.

This comment has been minimized.

Copy link
@gwwar

gwwar Dec 18, 2019

Contributor

Hmm I wonder why we added only decodeEntities in #7977. I think an upstream patch to lodash (as including leading zeros is okay), and dependency update would work too, but I think I'm missing some context here.

This comment has been minimized.

Copy link
@glendaviesnz

glendaviesnz Dec 18, 2019

Author Contributor

I wonder a bit about the corresponding escape

Yes, it would be good to update that to an internal encoding method once one is available.

@gwwar
gwwar approved these changes Dec 18, 2019
Copy link
Contributor

gwwar left a comment

I think changes here make sense @glendaviesnz 👍

@glendaviesnz glendaviesnz merged commit e31f457 into master Dec 18, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@glendaviesnz glendaviesnz deleted the fix/title-encoding-bug branch Dec 18, 2019
@youknowriad youknowriad added this to the Gutenberg 7.2 milestone Jan 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.