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 escaping issues with HTML entities #160

Merged
merged 3 commits into from
Sep 7, 2020
Merged

Conversation

donnapep
Copy link
Contributor

@donnapep donnapep commented Jul 28, 2020

Fixes #152.

This was tricky as the browser's HTML parser kept converting HTML entities like " to ". Fortunately, WordPress already solved this problem in their Code block, so this solution borrows from that.

Testing Instructions

  1. Add a SyntaxHighlighter Code block to a post using the version from master. Content to use is given in SyntaxHighlighter Code block: improper encoding/decoding of HTML entities #152.
  2. Switch to this branch and add a SyntaxHighlighter Code block to a new post using the same content as above.
  3. Ensure both the editor and the front-end render properly.
  4. Switch to the old version and ensure it still works.

@donnapep donnapep self-assigned this Jul 28, 2020
@donnapep donnapep force-pushed the fix/escaping-html-entities branch from 4c86d7e to c65ad7a Compare July 28, 2020 16:31
@donnapep donnapep force-pushed the fix/escaping-html-entities branch from 335ebd3 to 3da0d51 Compare July 28, 2020 16:33
@donnapep donnapep force-pushed the fix/escaping-html-entities branch from 225e254 to d4ae347 Compare July 28, 2020 17:24
@donnapep donnapep added this to the v3.5.6 milestone Jul 28, 2020
@donnapep donnapep requested a review from jom July 28, 2020 17:28
@jom jom requested a review from alexsanford August 12, 2020 12:25
Copy link
Contributor

@alexsanford alexsanford left a comment

Choose a reason for hiding this comment

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

This seems to work well for newly created posts 🎉

I'm not sure what this is supposed to do in terms of migrating old versions of this block. Is it intended to fix old versions?

I'm doing the following:

  • Move to master.
  • npm install && npm run build
  • Create a post, add the block, paste in the content, save.
  • Load the frontend (see some characters not displaying properly).
  • Switch to this branch.
  • npm install && npm run build
  • Reload the frontend.

At this point, I see this:

Screen Shot 2020-08-19 at 16 35 22

And this:

Screen Shot 2020-08-19 at 16 35 46

And this:

Screen Shot 2020-08-19 at 16 36 18

Two things:

  • The entities don't seem to be fixed in migration. Is that something that we should expect?
  • The <pre> tags have been visibly added. They aren't removed until I open the block editor and save the post.

@donnapep
Copy link
Contributor Author

donnapep commented Aug 20, 2020

I don't think this is going to be fixable for existing blocks, because the solution requires a change to the markup (to add a <code> element), which you can't do because the editor detects a mismatch between existing post content and the save function. Hence the deprecation.

@alexsanford
Copy link
Contributor

Maybe I don't understand what the deprecation is supposed to do. I don't think I got a notice or anything when I upgraded, it just quietly changed the output of the block. I'm concerned that this will happen for our users too.

@yscik
Copy link
Contributor

yscik commented Sep 7, 2020

[deprecation] just quietly changed the output

That's the point, it's basically a compatibility tool to read block data from old versions, and use it seamlessly.

I think there might be a way to fix the entities in old versions in the deprecation's migrate function, but it would be so overly complex that it's probably not worth pursuing.

Copy link
Contributor

@yscik yscik left a comment

Choose a reason for hiding this comment

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

Fix works well, code looks good

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SyntaxHighlighter Code block: improper encoding/decoding of HTML entities
3 participants