Fixes firefox copy paste issue

[gstjepan2]

Fixes firefox copy paste issue where copy pasting from src adds extra newline.
Closes: #141464

[rustbot]

r? @GuillaumeGomez

rustbot has assigned @GuillaumeGomez.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

[rustbot]

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @jsha, @lolbinarycat

[GuillaumeGomez]

With this fix, this code:

fn foo() {}

fn bar() {}

would become:

fn foo() {}
fn bar() {}

Which definitely sounds like a new bug.

[gstjepan2]

You're right it it also like looks I've went with the wrong approach.

Just settings the data to text/plain seems to fix the issue

if (navigator.userAgent.includes('Firefox')) {
  document.addEventListener('copy', function(e) {
    const text = nonnull(window.getSelection()).toString();    
    nonnull(e.clipboardData).setData('text/plain', text);
    e.preventDefault();
  });
} 

I'll amend the commit with this

[lolbinarycat]

1. looking for the string "Firefox" is correct because safari also puts "Mozilla" in its user agent string
2. getSelection can only return null if in a contextless environment like a detached iframe (in which case we shouldn't be getting copy events) and clipboardData can only be null for manually constructed events, so the usage of nonnull here looks correct.

It would be nice to fix this without user agent detection, but I don't see an obvious cause, the pre element only contains one set of linebreaks, and there are no extraneous whitespace nodes. The issue is the same with js enabled and disabled.

If this fixes the issue, I guess that's good.

At the very least we should probably have a comment that says why we are doing this, like "workaround for "

[gstjepan2]

Added a workaround for comment

[GuillaumeGomez]

I'm really not convinced it's the right fix. Even more considering it's only for one specific use-case. Wouldn't adding the right tags to the code viewer element be a better alternative?

[gstjepan2]

I agree, but wouldn't that be a too big of a (breaking?) change just for copy pasting issue in firefox?

[GuillaumeGomez]

I don't think so? But maybe we have something different in mind. Adding an attribute on pre.rust or on pre.rust > code to tell the web browser how to copy-paste so we don't need a JS fix should be possible, no?

[gstjepan2]

Oh gotcha, i am not aware of any clean way of doing that though, expecially without js not sure if copy paste behaviour can be modified

[lolbinarycat]

Hold on, have we verified this is only an issue on firefox? It doesn't seem to happen in chrome but someone should probably also check safari at least.

[GuillaumeGomez]

There is a bug report open in firefox about it: https://bugzilla.mozilla.org/show_bug.cgi?id=1273836

It's kinda a weird corner case apparently where they don't really what's the right behaviour.

[GuillaumeGomez]

Ah interesting: if we remove the user-select: none CSS property, the backline characters are removed. Seems like there is something doable on our side then. Gonna experiment.

[GuillaumeGomez]

So I think JS is the only way to fix this bug. However, it can be reproduced on other pages that src so the fix needs to be moved into main.js. We also need to make some changes to only tamper with the clipboard copy if we're copying from a <code> element.

So at the end of the main.js file you can add:

// This section is a bugfix for firefox: when copying text with `user-select: none`, it adds
// extra backline characters.
//
// Rustdoc issue: Workaround for https://github.com/rust-lang/rust/issues/141464
// Firefox issue: https://bugzilla.mozilla.org/show_bug.cgi?id=1273836
(function() {
    document.body.addEventListener('copy', event => {
        let target = event.target;
        let isInsideCode = false;
        while (target !== document.body) {
            if (target.tagName === 'CODE') {
                isInsideCode = true;
                break;
            }
            target = target.parentElement;
        }
        if (!isInsideCode) {
            return;
        }
        const selection = document.getSelection();
        nonnull(event.clipboardData).setData('text/plain', selection.toString());
        event.preventDefault();
    });
}());

I can also send a PR with the change directly, as you prefer. 😉

[gstjepan2]

I ammended the commit with that change.

[GuillaumeGomez]

It needs to be in main.js.

[GuillaumeGomez]

Almost there, just need to fix typescript errors now. :)

If you need help, don't hesitate to ask me.

[GuillaumeGomez]

Just thought that it could potentially crash here since we never check target:

Suggested change

	while (target !== document.body) {
	while (target && target !== document.body) {

[GuillaumeGomez]

CI is happy and code is almost all good. Just one change then I'll approve this PR. Thanks a lot!

[gstjepan2]

pushed it.

[lolbinarycat]

There's a few hypothetical crashes in the event handler, but since preventDefault is called last, I believe this should be fine.

[GuillaumeGomez]

@lolbinarycat Absolutely. Although I do expect that it's not possible on any main browser to have this code fail (or maybe under special security levels? Unsure).

Thanks a lot for your work @gstjepan2!

@bors r+ rollup

[bors]

📌 Commit a3ede10 has been approved by GuillaumeGomez

It is now in the queue for this repository.