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

Element: Avoid double-escaping valid character references #6620

Merged
merged 1 commit into from Jun 20, 2018

Conversation

Projects
None yet
3 participants
@aduth
Member

aduth commented May 7, 2018

This pull request seeks to improve the element serializer to avoid double ampersand encoding of valid character references.

BeforeAfter
serialize( 'Foo & Bar & Baz' );
// "Foo & Bar & Baz"
serialize( 'Foo & Bar & Baz' );
// "Foo & Bar & Baz"

Testing instructions:

Ensure unit tests pass:

npm run test

Verify that unexpected double-escaping no longer invalidates blocks:

  1. Log in as a contributor
  2. Navigate to Posts > Add New
  3. Insert a paragraph block with some text
  4. In Blocks > Advanced, set "Additional class names" to "foo&bar"
  5. Save the post
  6. Refresh the page
  7. Note the block is not invalid

@aduth aduth requested a review from WordPress/gutenberg-core May 31, 2018

@noisysocks

Very cool! 🎉

@@ -219,6 +219,46 @@ const CSS_PROPERTIES_SUPPORTS_UNITLESS = new Set( [
'zoom',
] );
/**
* Returns a string with ampersands escaped. Note that this is an imperfect

This comment has been minimized.

@noisysocks

noisysocks Jun 4, 2018

Member

When I read 'imperfect' it makes me think that the function is unfinished, when in fact it's intentionally imperfect.

This comment has been minimized.

@aduth

aduth Jun 4, 2018

Member

When I read 'imperfect' it makes me think that the function is unfinished, when in fact it's intentionally imperfect.

Depends how you view it. Technically it could be improved, but as I saw it the only option would be to include a full list of valid named character references, and in weighing the cost in doing so, I thought it to not have a net positive benefit.

@aduth

This comment has been minimized.

Member

aduth commented Jun 4, 2018

Thanks for the review @noisysocks ! I'll plan to merge this sometime after the upcoming 3.0 release.

@gziolo

This comment has been minimized.

Member

gziolo commented Jun 20, 2018

It looks like this PR was missed, 3.1 is coming up soon, let's get it in :)

@gziolo gziolo merged commit 5851831 into master Jun 20, 2018

2 checks passed

codecov/project 46.33% (+0.01%) compared to 802f890
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@gziolo gziolo deleted the fix/serialize-double-ampersand-escape branch Jun 20, 2018

@aduth

This comment has been minimized.

Member

aduth commented Jun 20, 2018

Thanks @gziolo , I carelessly forgot about the pull request, but fortunately at least included it in the 3.1 milestone so it didn't go overlooked 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment