Page MenuHomePhabricator

Diff-view: indicators should not be copied on copy-and-paste
Closed, ResolvedPublic2 Estimated Story Points

Description

When selecting text in a diffview the + and - indicators for added/removed paragraphs, as well as the indicators for a moved-paragraph and the spaces for unchanged paragraphs should not be taken to clipboard when copying text.
This is an issue since people often want to copy something from a diff back to the editor in case of edit-conflicts as well as in the case of quoting someone. It can be solved by adding CSS rules for this elements.

diffbug.png (477Γ—1 px, 44 KB)

Event Timeline

set .diff-marker to have user-select:none. But note that this might not work in all browsers. For security reasons, it's problematic to 'randomly' strip away things that people otherwise might expect to be copying. So I think for Chrome for instance, it only works if it is the first or the last character, and not when it's a character in between.

That's the main reason this CSS property is still vendor prefixed in many browsers.

See also T32773#1405757

Change 434049 had a related patch set uploaded (by PeterTheOne; owner: PeterTheOne):
[mediawiki/core@master] Diff-view: indicators should not be copied on copy-and-paste

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

Change 434049 merged by jenkins-bot:
[mediawiki/core@master] Diff-view: indicators should not be copied on copy-and-paste

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

Change 651809 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/core@master] Create diff markers with CSS

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

I don’t think the user-select approach works in all browsers, e.g. Safari. The follow up approach above uses content which appears to be more reliable.

Change 651809 merged by jenkins-bot:
[mediawiki/core@master] Create diff markers with CSS

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

While investigating T285285 I noticed that diff markers are still selected (at least with chrome). If this is the case for everyone, than having it broken for safari might be the lesser evil.

Diff markers are still selected in Firefox, too. Caniuse.com claims -webkit-user-select should work in Safari, but I agree even if it doesn't that's fine because this is less than 10% of our desktop traffic and the issue doesn't apply to mobile diffs.

Uh, actually... There's something weird going on. As I said, markers are selected in prod and on the beta cluster, but not locally or on patchdemo. At first I thought it was my patch for T285285, but then I realized it doesn't matter. You can verify this with any patchdemo instance, e.g. here.

Uh, actually... There's something weird going on. As I said, markers are selected in prod and on the beta cluster, but not locally or on patchdemo. At first I thought it was my patch for T285285, but then I realized it doesn't matter. You can verify this with any patchdemo instance, e.g. here.

For reference, I was given an answer at T285546.

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

[mediawiki/php/wikidiff2@master] Move diff markers content to a data attribute

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

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

[mediawiki/core@master] diff: Add user-select:none to moved paragraph markers

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

Change 711149 merged by jenkins-bot:

[mediawiki/core@master] diff: Add user-select:none to moved paragraph markers

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

Change 702196 merged by jenkins-bot:

[mediawiki/php/wikidiff2@master] Move diff markers content to a data attribute

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

QA notes: as with T285776#7274214 I don't believe this is testable on the beta cluster until a new version of wikidiff2 is cut. We will let you know when this happens.

I left a comment at https://gerrit.wikimedia.org/r/c/mediawiki/core/+/711149/ about adding the user-select: none property to .diff-marker, which if I'm not mistaken is what is necessary for the same fix to apply to the PHP-based diff engine.

Daimona set the point value for this task to 2.Oct 7 2021, 5:35 PM

Sample url: https://en.wikipedia.beta.wmflabs.org/wiki/Conflict-title-0.4626996476451548-I%C3%B1t%C3%ABrn%C3%A2ti%C3%B4n%C3%A0liz%C3%A6ti%C3%B8n
Tested on mobile(iPhone and Android) and desktop
Browser tested are Safari, Chrome, IE, Firefox, Edge
Verified the following:
Selected text in a diff view the + and - indicators for added/removed paragraphs, as well as the indicators for a moved-paragraph and the spaces for unchanged paragraphs was not taken to clipboard when copying text.
Note:

  1. Copy and paste in Chrome was a bit wonky, when two sections was selected and copied, the items shows in double frames when pasted on a word doc. See below screen shot
  2. Firefox had the best copy and past experience
  3. Mobile view diff2 is one page as expected and is still functional

IMG_3402.PNG (2Γ—1 px, 1 MB)

IMG_3401.PNG (2Γ—1 px, 477 KB)

Screen Shot 2021-10-13 at 5.07.55 PM.png (1Γ—2 px, 711 KB)

Screen Shot 2021-10-13 at 5.08.56 PM.png (824Γ—2 px, 217 KB)

Screen Shot 2021-10-13 at 5.07.01 PM.png (1Γ—2 px, 683 KB)

Screen Shot 2021-10-13 at 5.12.19 PM.png (988Γ—2 px, 300 KB)

Per review of test result during RTL meeting. The testing is done and existing issues will be tracked in ticket T292207

NRodriguez claimed this task.
NRodriguez added a subscriber: NRodriguez.

great work team!