Page MenuHomePhabricator

Community Wishlist Survey 2021/Editing/Copy and paste from diffs
Closed, ResolvedPublic3 Estimated Story Points

Description

https://meta.wikimedia.org/wiki/Community_Wishlist_Survey_2021/Editing/Copy_and_paste_from_diffs

Problem: It is difficult to copy and paste from a diff without having to edit the resulting text afterwards.
Who would benefit: Everyone
Proposed solution: 1) Add CSS similar to Github to not include the + or - signs when copying. 2) Allow the ability to select text from just one column rather than having to copy both columns.
More comments:
Phabricator tickets: T192526, T270775
Proposer: Rschen7754 01:16, 17 November 2020 (UTC)

Event Timeline

My investigation started by looking at what approach is used on github. Below are my notes; of course, this doesn't have to be the final approach.


The diff is contained in a table, as one would expect. This table has a data-lock-side-selection attribute, initially empty, and the file-diff-split class. Also, every td element has a data-split-side immutable attribute, whose value is either "left" or "right" depending on the side of the diff.

When the user starts selecting from a column, the data-lock-side-selection attribute in the table is assigned a value of "left" or "right" depending on which column the cursor is in. This is done via JavaScript with an event listener for the selectionchange event.

Then the following CSS does the magic:

.file-diff-split[data-lock-side-selection=left] [data-split-side=right],
.file-diff-split[data-lock-side-selection=right] [data-split-side=left]
{
   -webkit-user-select:none;
   -moz-user-select:none;
   -ms-user-select:none;
   user-select:none
}

Here is a raw POC: https://jsfiddle.net/ua025vm9/

A few more things to consider:

  • Should this be default behaviour or guarded with a button/something?
  • Is it possible to do that with CSS only?
  • What JS event(s) should be used? Especially w.r.t browser support. selectionchange alone doesn't seem to work, mousedown does.
  • This solution alone has a couple of visual annoyances:
    • Once you've selected something from a column, if you try to select something on the other column it won't work (you have to click it first)
    • Once the selection is locked on a column and you hover on the other column, the mouse cursor is an arrow, not the text one.

Further thoughts / investigation:

  • It can be done with CSS only, as shown in T270775. However, not all browsers support grids. Thus, I think the user-select approach outlined above might be good for a start, until we can improve it.
  • I'd have to check cross-browser support, possibly with something BrowserStack (for which I don't have a license)
  • The selectionchange can only be used on the document object, so not on the table cells. It's trivial to use it anyway, just need to check the event target.
  • Browser support for selectionchange is good: according to caniuse [1], compared with the support matrix, the only incompatibility is with Firefox >=27 & <51.
    • Note that we also need support for document.getSelection(), but support for that is excellent.
  • The annoyances described above can be fixed quite easily with some JS/CSS. See updated demo.

[1] - Note that caniuse has one entry for the event, and another for the whole Selection API (with notes about the events). You have to intersect these sources manually to get useful data.

Change 701104 had a related patch set uploaded (by Daimona Eaytoy; author: Daimona Eaytoy):

[mediawiki/core@master] [POC] Lock selection to a single side in diffs

https://gerrit.wikimedia.org/r/701104

The POC above should work. Moving to review/feedback but it's not really ready for review (e.g. it doesn't pass CI), instead I'd like feedback on my approach.

Test wiki created on Patch Demo by Daimona Eaytoy using patch(es) linked to this task:

https://patchdemo.wmflabs.org/wikis/681a165c85/w/

Daimona set the point value for this task to 3.Jun 24 2021, 9:49 PM

As a final update on the investigation, I've manually verified basic browser support on chrome, firefox, opera, safari on macOS and safari on iOS. All of them seem to work, so I'll proceed.

Further thoughts / investigation:

  • It can be done with CSS only, as shown in T270775. However, not all browsers support grids. Thus, I think the user-select approach outlined above might be good for a start, until we can improve it.

Pretty sure all our Grade A browsers support grids. We can selectively use grids when supported so this isn't an issue (which the patch attached to that bug already does).

As discussed in T192526, user-select:none does not work for preventing copy in all browsers.

Further thoughts / investigation:

  • It can be done with CSS only, as shown in T270775. However, not all browsers support grids. Thus, I think the user-select approach outlined above might be good for a start, until we can improve it.

Pretty sure all our Grade A browsers support grids.

Looking at https://caniuse.com/css-grid, it seems that the following grade A browser don't support grids: safari 9.1 - 10, iOS safari 9-10.2, android browser (although I can't explain the version numbering gap in caniuse); also, IE 11 requires the "-ms-" prefix.

We can selectively use grids when supported so this isn't an issue (which the patch attached to that bug already does).

Right, but that means rewriting the HTML, which I'm not sure how well it would work, especially on large diffs. When planning work on this wish we considered the grid approach, but the decision was to use JS-based selection locking, and switch to using CSS grids when browser support improves (i.e. we increase requirements for grade A browsers).

As discussed in T192526, user-select:none does not work for preventing copy in all browsers.

This was also considered, but I remember we tested this on various browsers (at least chrome, firefox, opera and safari) and it was working as expected. Do you have a specific example of browser that doesn't work with user-select:none?

Test wiki on Patch demo by Daimona Eaytoy using patch(es) linked to this task was deleted:

https://patchdemo.wmflabs.org/wikis/681a165c85/w/