clients(devtools): escape assets based on filetype #8456

connorjclark · 2019-04-19T23:59:28Z

Was tripped up by https://github.com/GoogleChrome/lighthouse/blob/master/lighthouse-core/report/html/report-styles.css#L738

patrickhulce · 2019-04-20T02:48:43Z

build/build-dt-report-resources.js

+  /** @type {string} */
+  let prefix;
+  if (name.endsWith('.html')) {
+    // This will not support unicode characters in inline stylesheets or js.


do we need to do something about this then?

might be better to throw if we detect this case instead?

There actually isn't any non ascii characters in the HTML files (afaik). This is just for (semi) completeness.

How would you detect nonascii in a style rule (and not elsewhere in the html)? I'm ok with just deferring this until there is nonascii in these files

How would you detect nonascii in a style rule (and not elsewhere in the html)?

seems like it doesn't matter since we don't have any right now? It does seem better to just throw and be explicit about it

I can throw on any ascii. Cannot allow ascii in html but throw on ascii in style/script (w/o actually parsing). I'll do the former.

I can throw on any ascii.

that's what I was at least trying to say :)

patrickhulce · 2019-04-20T02:52:46Z

build/build-dt-report-resources.js

@@ -23,8 +23,20 @@ const htmlReportAssets = require('../lighthouse-core/report/html/html-report-ass
 */
 function convertToAsciiAndWriteFile(name, content) {
  assert(content);
+
+  /** @type {string} */
+  let prefix;


unicodeEscapePrefix maybe?

brendankenny

out of curiosity, why does this have to be ascii? Seems really 20th century for Chrome asset loading

brendankenny · 2019-04-22T18:25:46Z

build/build-dt-report-resources.js

-  // eslint-disable-next-line no-control-regex
-  const escaped = content.replace(/[^\x00-\x7F]/g, c => '\\\\u' + c.charCodeAt(0).toString(16));
+
+  /** @type {string} */


if going to declare this as a string, maybe give it \\\\u as the default (instead of doing it in the final else) and then override it if html or css

brendankenny · 2019-04-22T18:28:57Z

build/build-dt-report-resources.js

+  /** @type {string} */
+  let prefix;
+  if (name.endsWith('.html')) {
+    // This will not support unicode characters in inline stylesheets or js.


How would you detect nonascii in a style rule (and not elsewhere in the html)?

seems like it doesn't matter since we don't have any right now? It does seem better to just throw and be explicit about it

paulirish · 2019-04-22T19:46:26Z

out of curiosity, why does this have to be ascii? Seems really 20th century for Chrome asset loading

Aye. is there something we can fix on the DT side to avoid this?

connorjclark · 2019-04-22T21:06:30Z

Maybe. Can look into it post-5.0.

paulirish · 2019-04-22T21:32:28Z

What currently happens with our emdash without this escaping? (just curious where the error is)

Instead of this escaping route, could we change the character in the content property... just use the unicode escape instead.. https://stackoverflow.com/a/190412/89484 ?

content: '\2014'

connorjclark · 2019-04-22T22:06:47Z

What currently happens with our emdash without this escaping? (just curious where the error is)

Without the context-specific escaping: It was mistakenly escaped with \u{...}. It showed up as a literal "\u{...}".

With no escaping: devtools build errors.

Instead of this escaping route, could we change the character in the content property... just use the unicode escape instead.. stackoverflow.com/a/190412/89484 ?

content: '\2014'

Yes.

Confirmed it's just the dash.

report.js
≤
©
►
▼
…
…
├
─
─
├
─
─
├
─
─
├
─
─
└
─
─
└
─
─
├
─
─
└
─
─
├
─
─
├
─
─
├
─
─
…
⋮
report.css
—
template.html
templates.html

build/build-dt-report-resources.js

brendankenny · 2019-04-23T22:10:35Z

build/build-dt-report-resources.js

+  /** @type {string} */
+  let prefix;
+  if (name.endsWith('.html')) {
+    // This will not support unicode characters in inline stylesheets or js.


I can throw on any ascii.

that's what I was at least trying to say :)

brendankenny

LGTM

(though it seemed like you were agreeing with @paulirish before on just escaping it where we use it instead?)

connorjclark · 2019-04-23T22:16:04Z

I think we should keep it as unescaped in the source (for readability) and just make this small build tweak. Hopefully this whole thing goes away if devtools' build process can be updated.

brendankenny · 2019-04-23T22:18:13Z

SGTM

Co-Authored-By: Hoten <cjamcl@gmail.com>

googlebot · 2019-04-23T22:19:00Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to find agreements for all the commit author(s) or Co-authors. If you authored these, maybe you used a different email address in the git commits than was used to sign the CLA (login here to double check)? If these were authored by someone else, then they will need to sign a CLA as well, and confirm that they're okay with these being contributed to Google.
In order to pass this check, please resolve this problem and have the pull request author add another comment and the bot will run again. If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

clients(devtools): escape assets based on filetype

71d05d0

patrickhulce reviewed Apr 20, 2019

View reviewed changes

unicodeEscapePrefix

4ccfbd6

vercel bot deployed to staging April 22, 2019 17:37 View deployment

brendankenny reviewed Apr 22, 2019

View reviewed changes

connorjclark mentioned this pull request Apr 22, 2019

Investigate devtools ascii requirement #8525

Closed

throw on ascii in html

c374e06

vercel bot deployed to staging April 22, 2019 21:16 View deployment

brendankenny reviewed Apr 23, 2019

View reviewed changes

brendankenny approved these changes Apr 23, 2019

View reviewed changes

brendankenny assigned paulirish Apr 23, 2019

Update build/build-dt-report-resources.js

7e562d4

Co-Authored-By: Hoten <cjamcl@gmail.com>

vercel bot deployed to staging April 23, 2019 22:19 View deployment

brendankenny merged commit f42a4ea into master Apr 25, 2019

brendankenny deleted the dt-build-escape branch April 25, 2019 23:49

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clients(devtools): escape assets based on filetype #8456

clients(devtools): escape assets based on filetype #8456

connorjclark commented Apr 19, 2019

patrickhulce Apr 20, 2019

connorjclark Apr 20, 2019

brendankenny Apr 22, 2019

connorjclark Apr 22, 2019

brendankenny Apr 23, 2019

patrickhulce Apr 20, 2019

brendankenny left a comment

brendankenny Apr 22, 2019

connorjclark Apr 22, 2019

brendankenny Apr 22, 2019

paulirish commented Apr 22, 2019

connorjclark commented Apr 22, 2019

paulirish commented Apr 22, 2019 •

edited

Loading

connorjclark commented Apr 22, 2019 •

edited

Loading

brendankenny Apr 23, 2019

brendankenny left a comment

connorjclark commented Apr 23, 2019

brendankenny commented Apr 23, 2019

googlebot commented Apr 23, 2019

clients(devtools): escape assets based on filetype #8456

clients(devtools): escape assets based on filetype #8456

Conversation

connorjclark commented Apr 19, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented Apr 22, 2019

connorjclark commented Apr 22, 2019

paulirish commented Apr 22, 2019 • edited Loading

connorjclark commented Apr 22, 2019 • edited Loading

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

connorjclark commented Apr 23, 2019

brendankenny commented Apr 23, 2019

googlebot commented Apr 23, 2019

paulirish commented Apr 22, 2019 •

edited

Loading

connorjclark commented Apr 22, 2019 •

edited

Loading