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

HTML serialization #3619

Merged
merged 16 commits into from
Feb 22, 2022
Merged

HTML serialization #3619

merged 16 commits into from
Feb 22, 2022

Conversation

silamon
Copy link
Contributor

@silamon silamon commented Jan 29, 2022

Fixes #1883
Makes use of the existing serialize addon and the templates

@meganrogge meganrogge self-assigned this Feb 1, 2022
@meganrogge
Copy link
Member

Thanks for working on this.

Could you please:

  • add information about how embedders are expected to hook this up to copy from selection
  • revert the formatting changes made to client.ts
  • maybe I'm doing something wrong, but the demo doesn't seem to work for me
    • the button says it copies, but it doesn't.
    • when I copied the output manually and pasted it in:
      • the vscode editor, it didn't produce the expected result or any red text
        Screen Shot 2022-02-01 at 12 13 01 PM
      • apple notes, it didn't produce the expected result - all of the text was colored red
        Screen Shot 2022-02-01 at 12 12 28 PM

@silamon
Copy link
Contributor Author

silamon commented Feb 2, 2022

Selection has been added, but unfortunately broke some existing tests. I'll fix that when I have time. :)
I've used the new asynchronous clipboard api, but a lot of browsers don't support it. I've replaced that with a deprecated alternative, which seems to be more supported anyway.

@meganrogge
Copy link
Member

Thanks for making the updates.

Can you pls clarify how I should be testing this?

I have some colored text in the demo, click the serialize button, copy and paste it into other apps and am still not seeing the right thing.
Screen Shot 2022-02-02 at 11 21 26 AM

@meganrogge
Copy link
Member

meganrogge commented Feb 2, 2022

from what I've read, it looks like an embedder would have to intercept the copy event, set the data format as 'text/html' and provide the serialized data.

it looks like you're doing that in the code, but it's not working for me. I'll try a different browser.

@silamon
Copy link
Contributor Author

silamon commented Feb 2, 2022

When you click the 'serialize' button, it should add a html code onto your clipboard of the selected part of the terminal. Don't copy the html in the visible window, that's there for debugging or to save as html manually if copy to clipboard doesn't work. From as soon as the button is clicked, you should be able to paste the selected part in an editor that supports pasting html like word or something like that.

@meganrogge
Copy link
Member

Doesn't work for me in edge, but I see it working for some cases that I've tried in Chrome 👍🏼.

@Eugeny
Copy link
Member

Eugeny commented Feb 2, 2022

Copying itself does work reliably for me in Chrome, however when the alternate screen is active, it still copies random content from the primary screen.

@meganrogge
Copy link
Member

meganrogge commented Feb 2, 2022

It actually does work in edge for me as well. I was expecting the background color to match that of the terminal, but perhaps that's not necessary.

I see that's done here if it's set

https://github.com/xtermjs/xterm.js/pull/3619/files#diff-9879222d670ac4c1d89f8fa8bc3051f5a51ee5bf1edbfad958e17ecaba1ff25bR565-R566

Copy link
Member

@Tyriar Tyriar left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @silamon, looks pretty close 😍

@Eugeny
Copy link
Member

Eugeny commented Feb 2, 2022

BG/FG colors were correct in the HTML I got, although when I first tested it by pasting into Word, it turned out inverted - that's Word's problem though, the HTML itself is correct and opening it in Chrome gets the colors right.

@meganrogge
Copy link
Member

Yes @Eugeny I saw the same - it was inverted in apple notes, but not in chrome.

@Tyriar Tyriar added this to the 4.18.0 milestone Feb 3, 2022
silamon and others added 3 commits February 3, 2022 18:55
Co-authored-by: Daniel Imms <2193314+Tyriar@users.noreply.github.com>
@silamon
Copy link
Contributor Author

silamon commented Feb 3, 2022

I can't seem to reproduce the text being pasted inverted. Do you see colored text in that paste or is it just clear text in black?

@Eugeny
Copy link
Member

Eugeny commented Feb 3, 2022

@silamon it's all colored with the exception of black and white being inverted. I don't even think it's your fault - my custom HTML copy implementation suffers from the same if background and foreground are #000 and #fff respectively.

Comment on lines +457 to +463
function listener(e: any) {
e.clipboardData.setData("text/html", output);
e.preventDefault();
}
document.addEventListener("copy", listener);
document.execCommand("copy");
document.removeEventListener("copy", listener);
Copy link
Member

Choose a reason for hiding this comment

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

I've used the new asynchronous clipboard api, but a lot of browsers don't support it. I've replaced that with a deprecated alternative, which seems to be more supported anyway.

It would be better if we moved on to more modern APIs like navigator.clipboard, it looks like just IE doesn't support writing to the clipboard which doesn't matter https://caniuse.com/async-clipboard

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've tested with navigator.clipboard in the first commit and Firefox doesn't support it. The caniuse link does mention that as well.

*
* @param options Custom options to allow control over what gets serialized.
*/
public serializeAsHTML(options?: Partial<IHTMLSerializeOptions>): string;
Copy link
Member

Choose a reason for hiding this comment

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

We should add some basic test to SerializeAddon.api.ts that cover:

  • Each option
  • Colored/styled vs all default color/style text

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll write some unit tests as soon as I have time. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tests have been added. Unfortunately, not as playwright tests since I couldn't get it to work at this moment.

@Tyriar
Copy link
Member

Tyriar commented Feb 10, 2022

Looks good other than those comments 👍

@meganrogge
Copy link
Member

The tests look good 👍🏼 thanks

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.

Copying HTML-formatted output
4 participants