Prevent diff linenumber selection

[simonwiles]

Applies user-select: none; to .d2h-code-line-prefix and .d2h-code-linenumber classes. This allows selection text from diffs for copy and paste without getting line numbers and +/- prefixes.

Closes #1214

[campersau]

This will disable the selection of the whole line (e.g. with the content). Is this intended? I guess for deletions this might be correct but not for changes / additions.

[simonwiles]

I don't think it will -- it should just disable selection of the +/- prefix (i.e., the element with the class d2h-code-line-prefix).

Edit: with respect to deletions etc. it's not altogether clear what the most desirable behaviour is, and will probably boil down to individual preferences and use-cases, which is why I kept this really minimal. It seems that disabling selection for the line numbers and +/- prefixes is going to be a simple and uncontroversial (if minor) improvement.

[campersau]

For me this isn't working. Here you can see that none of the changed lines is now selected:

Because the d2h-code-line-prefix also includes the d2h-code-line-ctn which inherits the user-select: none;:

Can you also please fix the side-by-side diff? It uses the d2h-code-side-linenumber class:

---

On GitHub you can also select addition, modifications and deletions so I think that's fine then.

[simonwiles]

I'm puzzled -- you appear to be looking at something rather different to what I have:

Because the d2h-code-line-prefix also includes the d2h-code-line-ctn

If this were the case (and again, I can't reproduce this), that would be a problem anyway, right?

Can you also please fix the side-by-side diff?

Yes, thanks for catching this!

On GitHub you can also select addition, modifications and deletions so I think that's fine then.

I agree :)

I've also added all the prefixed versions of the user-select property, as I had mistakenly assumed the LESS preprocessor would do this.

[campersau]

Ahh I was looking at the diffs in the staging area, they are rendered differently because of the patch feature I guess.

ungit/components/textdiff/textdiff.js

Lines 191 to 196 in e234722

	getPatchCheckBox(symbol, index, isActive) {
	if (isActive) {
	this.numberOfSelectedPatchLines++;
	}
	return `<div class="d2h-code-line-prefix"><span data-bind="visible: editState() !== 'patched'">${symbol}</span><input ${isActive ? 'checked' : ''} type="checkbox" data-bind="visible: editState() === 'patched', click: togglePatchLine.bind($data, ${index})"></input>`;
	}

Can you please add the missing </div> in this line? Then it should also work there.

diff --git a/components/textdiff/textdiff.js b/components/textdiff/textdiff.js
index 9698fe1e..bfe11ce2 100644
--- a/components/textdiff/textdiff.js
+++ b/components/textdiff/textdiff.js
@@ -192,7 +192,7 @@ class TextDiffViewModel {
     if (isActive) {
       this.numberOfSelectedPatchLines++;
     }
-    return `<div class="d2h-code-line-prefix"><span data-bind="visible: editState() !== 'patched'">${symbol}</span><input ${isActive ? 'checked' : ''} type="checkbox" data-bind="visible: editState() === 'patched', click: togglePatchLine.bind($data, ${index})"></input>`;
+    return `<div class="d2h-code-line-prefix"><span data-bind="visible: editState() !== 'patched'">${symbol}</span><input ${isActive ? 'checked' : ''} type="checkbox" data-bind="visible: editState() === 'patched', click: togglePatchLine.bind($data, ${index})"></input></div>`;
   }

   togglePatchLine(index) {

---

The commit diffs look good though 👍

[jung-kim]

I feel like patch feature causes more confusions and problems and not sure if it is worth keeping it...

[campersau]

@simonwiles any update on this? If you want I can make the last change so we can merge the PR.

[simonwiles]

@campersau apologies, I got distracted because I noticed that this doesn't work perfectly in Firefox, and I couldn't find a solution (and then I got busy with other things).

In Firefox a lot of unnecessary linebreaks are copied when selecting multiple lines from the diffs. I experimented with a number of different solutions, but I couldn't get a perfect behaviour in FF, and I still haven't got a solution. However, the behaviour effected in this PR is good in Chrom[e|ium], Opera, Brave etc., and better than it was previously in FF, so in the spirit of not letting the perfect be the enemy of the good, I've made the requested change, and I think it might be ready to merge.

I assume the failing tests are not related to this PR?

[campersau]

Thanks!

[jung-kim]

Test is breaking something completely irrelevant and I will try to fix it