Conversation
|
022c787 was deployed to: https://fred-pr1266.review.mdn.allizom.net/ |
| const { css, html, js } = controller.code; | ||
| let code = `<!doctype html>`; | ||
| if (css) code += `<style>${css}</style>`; | ||
| if (html) code += html; |
There was a problem hiding this comment.
Nit: What if html contains the doctype, or a full HTML structure? (I guess in this case, chances are css is empty.)
There was a problem hiding this comment.
We have the same issue in the playground/runner currently: a user could add a full HTML structure to the HTML input.
I don't think it's really a problem, HTML parsers are able to work around it, but if we did want to fix it we should do it cohesively as a separate issue.
There was a problem hiding this comment.
Here's a reason to set a more complete HTML structure.
Copy the following in the Playground:
document.body.style.backgroundColor = 'DeviceMotionEventAcceleration' in window ? 'green' : 'red';It is green in the Playground runner, but when copying and opening the data URL, the page is white, because there is no body.
Co-authored-by: Claas Augner <495429+caugner@users.noreply.github.com>
caugner
left a comment
caugner
left a comment
There was a problem hiding this comment.
LGTM. I support this feature, and will probably use it frequently.
Before merging, let's briefly discuss it in today's meeting.
| it("should only uri-encode non-space whitespace and percent symbols", async () => { | ||
| await $(`mdn-play-editor[language="html"]`).click(); | ||
| await browser.keys("@\n@ @%20 @"); | ||
| await $(`mdn-play-editor[language="css"]`).click(); | ||
| await browser.keys("body {\n font-size: 5em;\n}"); | ||
|
|
||
| await $(`[data-id="share"]:not([disabled])`).click(); | ||
| await $(`[data-glean-id="playground: share-data-url"]`).click(); | ||
|
|
||
| // will cause a webdriver error about permissions, but we've set them in firefox through about:config | ||
| await expect(browser).toHaveClipboardText( | ||
| expect.stringContaining("@%0A@ @%2520 @"), | ||
| ); | ||
| await expect(browser).toHaveClipboardText( | ||
| expect.stringContaining( | ||
| "<style>body {%0A font-size: 5em;%0A}</style>", | ||
| ), | ||
| ); | ||
| }); |
There was a problem hiding this comment.
[nit] I would expect this to be a unit test, rather than an e2e test.
| "dom.events.testing.asyncClipboard": true, | ||
| "dom.events.testing.asyncClipboard.readText": true, |
There was a problem hiding this comment.
[nit] Are these here to stay,? Is there a page or bug with more info?
| describe("Playground", () => { | ||
| describe("copy data url", () => { | ||
| beforeEach(async () => { | ||
| await DocPage.open("en-US/play"); |
There was a problem hiding this comment.
[nit] Why is there no leading slash here?
| await DocPage.open("en-US/play"); | |
| await DocPage.open("/en-US/play"); |
2 participants
@caugner as we briefly discussed in our 1:1
since working on devtools, I've noticed it's quite a common/nice pattern to share testcases (on slack, bugzilla, etc.) with
data:text/htmlurls. authoring them is a bit of pain though: I asked the team if there was any better way than manually typing into the url bar... and they told me that's what they were all doing!I've added in the simplest possible way for now: obviously references to static assets are going to break, we could remove non-syntactic line breaks to make the urls a bit shorter/prettier, etc. but we can make those improvements if the metrics show any usage/we get direct feedback about it.
I did make the effort to only urlencode non-space whitespace as otherwise the urls are very un-readable, and what's nice about the practice is you can read what the page you're about to load is before pasting it in. Firefox/Chromium/Epiphany seem to handle unencoded utf8 characters just fine when pasted in (I'll check in Safari once my mac is charged).
For an example where newlines are important:
The generated url is readable, and functional!
data:text/html;charset=utf-8,<!doctype html><style>body {%0A white-space: pre;%0A}%0A</style>hello%0Aworld%0A❤️<script>console.log(`hello%0Aworld!`);%0A</script>