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

Escape Editable HTML #17994

Merged
merged 8 commits into from Nov 7, 2019
Merged

Escape Editable HTML #17994

merged 8 commits into from Nov 7, 2019

Conversation

@ellatrix
Copy link
Member

ellatrix commented Oct 17, 2019

Description

Fixes #16252.
Fixes #13218.
Alternative to #17789.

General problem: currently we only escape "lone" ampersands (& => &) in editable text. This is a bit strange, since if you create some text, you'd expect e.g. & to be rendered as such. This is NOT the case. & needs to be converted to & in order to be rendered as &. The same is true for any other HTML entity.

Also removes a unnecessary layer of escaping and unescaping from the code block. The attribute source is of the type text, which already unescapes entities. In the code block, we only need to make sure the value from PlainText is properly escaped for use in normal HTML elements (since PlainText is a textarea and its value is unescaped.

How has this been tested?

  • Create a paragraph block and type …. Preview the post. You should see … and not .
  • Create a code block and type …. Save. Reload the place. The block should be valid.

Screenshots

Types of changes

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
@dmsnell

This comment has been minimized.

Copy link
Contributor

dmsnell commented Nov 6, 2019

Tested this with the following post in master

Screen Shot 2019-11-06 at 9 43 38 AM

That's actually wrong because I typed "…HTML is &" and it changed it to "…HTML is &"

Before saving and reloading this was in the text view of the editor.

<!-- wp:paragraph -->
<p>We use the &lt;code> element to signify that the text represents software source code. We say one &amp; two to mean <em>one and two</em>. The way to represent a typographical ampersand in HTML is <code>&amp;</code>.</p>
<!-- /wp:paragraph -->

<!-- wp:code -->
<pre class="wp-block-code"><code>&lt;div>
  &lt;p>The formula&lt;/p>
  &lt;pre>&lt;code>if ( x > 5 &amp;&amp; x &lt; 10 ):&lt;/code>&lt;/pre>
&lt;/div></code></pre>
<!-- /wp:code -->

<!-- wp:paragraph -->
<p></p>
<!-- /wp:paragraph -->

And this displayed in the page view:
Screen Shot 2019-11-06 at 9 44 32 AM

BrokenAmpsInMaster mov


On this branch however…

Screen Shot 2019-11-06 at 10 03 56 AM

<!-- wp:paragraph -->
<p>We use the &lt;code&gt; element…one &amp; two…<code>&amp;amp;</code></p>
<!-- /wp:paragraph -->

<!-- wp:code -->
<pre class="wp-block-code"><code>&lt;div>
x > 5 &amp;&amp; x &lt; 10</code></pre>
<!-- /wp:code -->

and after save and reload

Screen Shot 2019-11-06 at 10 04 24 AM

FixedAmpsInBranch mov


Regardless of the details this seems to address the issue and get us out of a painful place where you can't type what you want. Thanks @ellatrix!

@dmsnell
dmsnell approved these changes Nov 6, 2019
Copy link
Contributor

dmsnell left a comment

This is a good fix and covers at least one common case that's frustrating.

@aduth

This comment has been minimized.

Copy link
Member

aduth commented Nov 6, 2019

Possibly related: #13218, #15780

cc @davilera

@davilera

This comment has been minimized.

Copy link
Contributor

davilera commented Nov 7, 2019

#13218 described an issue with < characters in core/code blocks, which is what #15780 tries to fix. @ellatrix, do you think that PR could/should be merged into this one?

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Nov 7, 2019

@davilera Have you tested this PR? It should fix that too.

@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Nov 7, 2019

I tested #13218, and this PR fixes it too. Added a test case.

@ellatrix ellatrix force-pushed the fix/escape-amp branch from 53f1253 to 03b8212 Nov 7, 2019
@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Nov 7, 2019

Just for extra clarity: this fixes all escaping for all HTML entities in editable text, not just &amp;.

@ellatrix ellatrix merged commit ac9a5a6 into master Nov 7, 2019
2 checks passed
2 checks passed
pull-request-automation
Details
Travis CI - Pull Request Build Passed
Details
@ellatrix ellatrix deleted the fix/escape-amp branch Nov 7, 2019
@ellatrix

This comment has been minimized.

Copy link
Member Author

ellatrix commented Nov 7, 2019

Thanks for the review @dmsnell!

@ellatrix ellatrix mentioned this pull request Nov 7, 2019
0 of 5 tasks complete
@youknowriad youknowriad added this to the Gutenberg 6.9 milestone Nov 11, 2019
CreativeDive added a commit to CreativeDive/gutenberg that referenced this pull request Nov 12, 2019
* Escape HTML: Always Escape Ampersand

* Revert "Escape HTML: Always Escape Ampersand"

This reverts commit 70dfa53.

* Escape HTML for Editable Text

* Revert PlainText changes

* Adjust code block escape util

* Update package lock

* Add test case for escapeEditableHTML

* Update docs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.