Fix HTML entity escaping in Glimmer printer #4533

omghax · 2018-05-24T16:20:48Z

Previously, prettier would unescape < and > which would result in invalid HTML. This change escapes Glimmer TextNode nodes to ensure that &, <, and > remain escaped properly.

lydell

Can you add tests for other escapes?

omghax · 2018-05-24T18:05:00Z

@lydell Sure, are there any others you had in mind? The full list is pretty large. Handlebars itself only escapes a handful of characters, though I'm not sure why they chose to escape the last 3 in that list.

lydell · 2018-05-24T18:48:30Z

&amp;
&nbsp;
&#160;
&#xa0;

lydell · 2018-05-24T19:06:56Z

src/language-handlebars/printer-glimmer.js

+  "&": "&amp;",
+  "<": "&lt;",
+  ">": "&gt;",
+  "\xa0": "&nbsp;"


This doesn’t feel right.

lydell · 2018-05-24T19:07:32Z

src/language-handlebars/printer-glimmer.js

+  "\xa0": "&nbsp;"
+};
+
+const badChars = /[&<>\xa0]/g;


So you had to add yet another character here? Where will it end?

lydell · 2018-05-24T19:08:18Z

tests/html_glimmer/__snapshots__/jsfmt.spec.js.snap

+<p>Some escaped characters: &lt; &gt; &amp; &nbsp; &#160; &#xa0;</p>
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+<p>
+  Some escaped characters: &lt; &gt; &amp; &nbsp; &nbsp; &nbsp;


So all of those are turned into  ? Not sure if that’s a feature or a bug. Perhaps it’s out of scope for this PR.

lydell · 2018-05-24T19:10:00Z

This doesn’t feel like the right thing to do overall.

Perhaps we could merge a quick-fix for only <, > and & so that at least the output code is valid, and then open an issue about properly handling escapes.

omghax · 2018-05-24T19:17:20Z

@lydell Okay, I'll remove that commit from this PR and just leave it focused on <, >, and &.

¯_(ツ)_/¯

rwjblue · 2018-05-25T00:06:55Z

FWIW - @glimmer/syntax uses simple-html-tokenizer which ships with a file with all the HTML char references (see here) which is actually generated from https://github.com/w3c/html/blob/master/entities.json...

j-f1 · 2018-06-28T18:08:55Z

What’s the status of this? Should we merge or close?

lifeart · 2018-11-01T09:34:55Z

@omghax @j-f1 sorry for pinging, is it possible to get it merged?

j-f1 · 2018-11-02T10:34:05Z

@lydell what do you think?

lydell · 2018-11-02T10:43:40Z

I guess we could merge it.

j-f1 · 2018-11-02T13:35:58Z

Can you fix the merge conflicts @omghax?

billdami · 2019-05-24T11:41:23Z

Just curious on the status of this issue? Also, it was a bit hard to judge from the discussion here, but will this fix result in   not being converted to literal spaces? We are seeing issues with these specifically, since   behaves differently in the DOM then a normal space (e.g. collapsing multiple adjacent spaces).

Ideally, we'd love a way to disable prettier's entity conversion entirely (for example we prefer to see/edit » in the source rather than »), but non-breaking spaces was the more immediate concern.

alexander-akait · 2019-05-24T12:16:12Z

/cc @omghax can you rebase?

alexander-akait · 2019-05-24T12:16:42Z

@billdami looks like PR was abandoned, feel free to send new PR

rwjblue · 2019-05-24T14:59:28Z

FWIW, new changes were made to the latest @glimmer/syntax package to allow a "codemod" mode. In that mode, no entity escaping is done at all. If we migrate to that new mode, I think we will have a much better time.

See glimmerjs/glimmer-vm#938 for the changes I'm referring to...

lydell · 2019-05-24T15:13:11Z

@rwjblue Awesome! We should definitely try that out.

Previously, prettier would unescape `<` and `>` which would result in invalid HTML. This change escapes Glimmer TextNode nodes to ensure that `&`, `<`, and `>` remain escaped properly.

GavinJoyce · 2019-06-19T20:26:45Z

I've created #6234 which uses the new "codemod" glimmer mode

GavinJoyce · 2019-06-19T23:07:56Z

@evilebottnawi perhaps you meant to approve #6234 instead? I think this PR can then be closed

lydell · 2019-07-08T09:08:12Z

Supersedes by #6234.

lydell previously requested changes May 24, 2018

View reviewed changes

lydell reviewed May 24, 2018

View reviewed changes

src/language-handlebars/printer-glimmer.js Outdated

"&": "&",

"<": "<",

">": ">",

"\xa0": " "

Copy link

Member

lydell May 24, 2018

Choose a reason for hiding this comment

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

This doesn’t feel right.

lydell reviewed May 24, 2018

View reviewed changes

omghax force-pushed the fix-glimmer-escaping branch from 4d32cc6 to 2eaf2b5 Compare May 24, 2018 19:17

j-f1 added the status:awaiting response Issues that require answers to questions from maintainers before action can be taken label Jun 28, 2018

lydell mentioned this pull request Oct 25, 2018

glimmer formatter issue for special symbols > and < #5300

Closed

mfeckie mentioned this pull request May 24, 2019

Disable html entity conversion mfeckie/handlebars-formatter#8

Closed

jgwhite mentioned this pull request Jun 19, 2019

[QUEST] Glimmer jgwhite/prettier#1

Closed

Fix HTML entity escaping in Glimmer printer

8c19e67

Previously, prettier would unescape `<` and `>` which would result in invalid HTML. This change escapes Glimmer TextNode nodes to ensure that `&`, `<`, and `>` remain escaped properly.

omghax force-pushed the fix-glimmer-escaping branch from 2eaf2b5 to 8c19e67 Compare June 19, 2019 15:34

GavinJoyce mentioned this pull request Jun 19, 2019

[Glimmer] preserve HTML entities #6234

Merged

4 tasks

tchak mentioned this pull request Jun 19, 2019

[QUEST] Glimmer #4908

Closed

alexander-akait approved these changes Jun 19, 2019

View reviewed changes

lydell closed this Jul 8, 2019

lydell removed the status:awaiting response Issues that require answers to questions from maintainers before action can be taken label Jul 8, 2019

lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Oct 6, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 6, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix HTML entity escaping in Glimmer printer #4533

Fix HTML entity escaping in Glimmer printer #4533

omghax commented May 24, 2018

lydell left a comment

omghax commented May 24, 2018

lydell commented May 24, 2018

lydell May 24, 2018

lydell May 24, 2018

lydell May 24, 2018

lydell commented May 24, 2018

omghax commented May 24, 2018

rwjblue commented May 25, 2018

j-f1 commented Jun 28, 2018

lifeart commented Nov 1, 2018

j-f1 commented Nov 2, 2018

lydell commented Nov 2, 2018

j-f1 commented Nov 2, 2018

billdami commented May 24, 2019

alexander-akait commented May 24, 2019

alexander-akait commented May 24, 2019

rwjblue commented May 24, 2019

lydell commented May 24, 2019

GavinJoyce commented Jun 19, 2019

GavinJoyce commented Jun 19, 2019

lydell commented Jul 8, 2019

Fix HTML entity escaping in Glimmer printer #4533

Fix HTML entity escaping in Glimmer printer #4533

Conversation

omghax commented May 24, 2018

lydell left a comment

Choose a reason for hiding this comment

omghax commented May 24, 2018

lydell commented May 24, 2018

lydell May 24, 2018

Choose a reason for hiding this comment

lydell May 24, 2018

Choose a reason for hiding this comment

lydell May 24, 2018

Choose a reason for hiding this comment

lydell commented May 24, 2018

omghax commented May 24, 2018

rwjblue commented May 25, 2018

j-f1 commented Jun 28, 2018

lifeart commented Nov 1, 2018

j-f1 commented Nov 2, 2018

lydell commented Nov 2, 2018

j-f1 commented Nov 2, 2018

billdami commented May 24, 2019

alexander-akait commented May 24, 2019

alexander-akait commented May 24, 2019

rwjblue commented May 24, 2019

lydell commented May 24, 2019

GavinJoyce commented Jun 19, 2019

GavinJoyce commented Jun 19, 2019

lydell commented Jul 8, 2019