Prevent newlines from being duplicated after copying an enhanced codeblock

[noah-weingarden]

Closes #246. I wrote a test to expose this (and it's the first test for copying an enhanced codeblock in general). Unfortunately, jsdom doesn't implement innerText (see jsdom/jsdom#1245), so I had to modify the map function to use textContent instead. I think that should be ok in this use case since the element containing the text for a single line of code is unlikely to ever have child nodes. If not though, I probably wrote that test for nothing.

[github-actions]

Thanks for opening this pull request! 🙏

I've changed this PR's base branch to develop. A maintainer will review your PR soon!

[github-actions]

The spec from this PR is available at https://preview.sesh.rs/previews/eecs485staff/primer-spec/248/.

^{(Available until Mon May 08 2023.)}

[noah-weingarden]

I changed this to use CustomEvent instead of document.createEvent because the latter relies on Event.initEvent, which is deprecated.

[seshrs]

Thanks for putting this up @noah-weingarden! I’ll try to get to this over the weekend.
Seems like we should just release a patch update asap to fix the bug.

I had to modify the map function to use textContent instead. I think that should be ok in this use case since the element containing the text for a single line of code is unlikely to ever have child nodes.

Actually, it’s likely for a single line of code to have child nodes since syntax highlighting is applied as span elements over specific tokens.

However, I agree with you that textContent is likely more appropriate for this use case. According to the MDN docs, textContent returns text from descendant nodes as well.

Could you update the unit test with more nested syntax highlighting nodes from an actual rendered code block? You should be able to get this by viewing the HTML source of a Primer Spec page.

[noah-weingarden]

Actually, it’s likely for a single line of code to have child nodes since syntax highlighting is applied as span elements over specific tokens. However, I agree with you that textContent is likely more appropriate for this use case. According to the MDN docs 1, textContent returns text from descendant nodes as well.

Haha, then textContent is appropriate for the exact opposite of why I thought it is. XD

Could you update the unit test with more nested syntax highlighting nodes from an actual rendered code block? You should be able to get this by viewing the HTML source of a Primer Spec page.

I don't think that would work: the event handler for clicking the button won't be attached if I just use the raw HTML for the rendered code block. We'd have to convert the HTML code to DOM nodes and then manually call copyLines instead of clicking the button. A couple downsides of that would be that 1) we'd need to export copyLines even though it makes more sense to be private to its module like it currently is and 2) the test would be less realistic at simulating a real user's interaction with the page. I guess another option would be to dynamically attach an event handler within the test, but that would still require extracting the event handler to be an exportable function, which feels a little arbitrary if it only needs to be exported to a test. Also, that would mean the test would be duplicating work that Primer Spec already does.

[seshrs]

I'm sorry I still haven't got to this. It's on my todo list for this weekend though!

[seshrs]

LGTM after pushing the nits that I referenced in my previous comment. Thanks for putting this up @noah-weingarden!

[noah-weingarden]

Oh, I see what you meant now! And no problem!