Page MenuHomePhabricator

Inline Diff: Add legend and tooltips
Open, Needs TriagePublic3 Estimated Story Points

Description

Introduce legends and tooltips to inline diff so that users, that are not familiar with the blue and yellow highlights, can better understand the type of edits.

Feature Summary

On the current experience, new users might not be able to understand an edit. Enhance the UI so that newcomers can better understand edits.

Acceptance Criteria

  • A legend that would explain the colour highlights within a diff view.
  • Tooltips are triggered that indicate the type of edit when a user hover over a yellow or blue highlighted area. The tooltip popup says "Content added" for blue highlighted area and "Content deleted" for yellow highlighted area.
  • Should look and work correctly with and without the VisualEditor, RevisionSlider, and MobileFrontend extensions.

User Story

  • As a user of the diff tool, I want to see a legend to explain the symbology.
  • As a viewer of the Unified Inline Wikitext Diff on Desktop, I will be able to see a legend so that I can understand what visual markers mean.
  • As a viewer of the Unified Inline Wikitext Diff on Desktop, I will be able to see a tooltip that indicates "Content deleted" when hovering over a yellow highlighted area.
  • As a viewer of the Unified Inline Wikitext Diff on Desktop, I will be able to see a tooltip that indicates "Content added" when hovering over a blue highlighted area.

Designs

Screenshot from 2023-02-21 14-28-48.png (420Γ—623 px, 65 KB)

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

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

https://patchdemo.wmflabs.org/wikis/06be5995f9/w/

Hey @Samwilson I found a couple of minor things and a couple of just opinions.

Tested most in BrowserStack
OS: macOS 13.2, Windows 11
Browser: Chrome 111, Edge 111, Firefox 111, Safari 16.3
Environment: Patch Demo links tested

Difference in Zoom %- They all seem to be similar with the browsers up to 250% as seen in "View 100%", regarding the content deleted highlight spacing. At 250% though, the spacing seems to be extra but beyond that, it seems to go back as seen in "View 300%"

OS/BrowserView 100%View 250%View 300%
Windows 11/Chrome 111/Edge 111 & macOS 13.2 Safari
T324759_BetterDiffsLegend_W11_Edge100.png (702Γ—1 px, 506 KB)
T324759_BetterDiffsLegend_W11_Chrome250.png (1Γ—1 px, 1 MB)
T324759_BetterDiffsLegend_W11_Chrome300.png (998Γ—2 px, 926 KB)

Firefox spacing- Windows 11 was fine with the content deleted highlight spacing but & macOS 13.2 has that extra highlighted space again while zooming at 240%

OS/BrowserWindows 11 View 240% macOS 13.2 View 240%
Firefox 111
T324759_BetterDiffsLegend_W11_Firefoxx240.png (868Γ—1 px, 822 KB)
T324759_BetterDiffsLegend_Mac_Firefox240.png (1Γ—1 px, 613 KB)

Phones Tested- iPhone 14, Samsung S23, Google Pixel 7. Is the add space that's green supposed to go all the way through as seen in the screenshot or just a little one like your patch demo? https://patchdemo.wmflabs.org/wikis/7c5b07966a/wiki/Special:MobileDiff/151...153?diffmode=source

T324759_BetterDiffsLegend_IOS_iPhone14.png (1Γ—569 px, 357 KB)

Opinion 1: Alignment - Should the legends be lined up with Visual/Wikitext to make it look cleaner looking besides being staggered?

T324759_BetterDiffsLegend_Mac_Chrome_Alignment.png (651Γ—1 px, 131 KB)

Opinion 2: Size - Lengends has a height of 30.4 pixels while Visual/Wikitext has 32 pixels in height. Not sure if that's as expected or not.

T324759_BetterDiffsLegend_Mac_Chrome_HeightSize.png (620Γ—2 px, 132 KB)

Change 902567 had a related patch set uploaded (by Samwilson; author: Samwilson):

[mediawiki/extensions/VisualEditor@master] Float the diff-mode selector when viewing inline diff

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

Thanks @GMikesell-WMF!

  • Difference in Zoom % β€” The spacing differences you're seeing here (and in the following two items) are strange. I'm not able to reproduce them on Firefox or Chromium, either with or without my above patch. Are you seeing a difference in spacing between that patchdemo and current Beta? I don't think the changes I've made should be having any effect on that spacing.
  • Firefox spacing β€”
  • Phones Tested β€”
  • Opinion 1: Alignment β€” you're right it should be aligned! I've added a fix (in VisualEditor) for this.
  • Opinion 2: Size β€” The right-side switcher is going to change soon (to a dropdown), with T330229 which is going to be merged before this task, so I'll hold off on fixing this precisely for now.

I've made a new patchdemo:

Patch is not really ready for review though. There's no space above the legend, and the floated switcher is floating even on mobile. Fix is coming.

Samwilson changed the task status from Open to Stalled.Apr 4 2023, 4:42 AM

Stalled pending T330229, because the layout of the switcher dropdown there is going to effect the location and styling of the new legend.

Updated switcher designs

VisualWikitext - Two columnWikitext - InlineReview changes - VisualReview changes - Wikitext
Screenshot 2023-04-06 at 15.26.29.png (1Γ—2 px, 1 MB)
Screenshot 2023-04-20 at 17.33.30.png (1Γ—2 px, 1 MB)
Screenshot 2023-04-06 at 15.27.16.png (1Γ—2 px, 1 MB)
Screenshot 2023-04-06 at 15.27.32.png (1Γ—1 px, 265 KB)
Screenshot 2023-04-20 at 17.38.09.png (1Γ—2 px, 381 KB)
JSengupta-WMF changed the task status from Stalled to Open.Apr 6 2023, 5:10 PM

@Samwilson The legend is showing up when you toggle to the Visual option.

(Some notes on why we might want a new hook for implementing this.)

There are a few extensions that add things above the diff table with the DifferenceEngineViewHeaderHook, such as Revision-Slider, VisualEditor, and MediaWiki-extensions-SpamDiffTool (others use the hook but not to add HTML to the output).

In T330229 we're adding a new hook with which to change the available diff-mode and -type, so it seems reasonable to add space for that while adding the new legend here. DifferenceEngineViewHeader hook runs in DifferenceEngine::showDiffPage(), which is before DifferenceEngine::addHeader() is run. Hook handlers can add HTML to the beginning or end of the output, but that's about all the control they get. That means you can get things being added between the main elements that we care about in Wikimedia deployment, such as here with the 'add to spam' link:

above-diff-table.png (322Γ—1 px, 46 KB)

The new hook means extensions can add whatever they want to a horizontal area directly above the main diff table (or modify other items there, such as the legend and soon the switcher).

Change 900146 merged by jenkins-bot:

[mediawiki/core@master] diff: Add legend and tooltips to inline diff display

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

Change 902567 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Move the diff-mode selector to the new DifferenceEngineBeforeDiffTable hook

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

Change 901332 merged by jenkins-bot:

[mediawiki/extensions/MobileFrontend@master] diff: Hide legend for inline diffs

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

Test wiki created on Patch demo by GMikesell-WMF using patch(es) linked to this task:
https://patchdemo.wmflabs.org/wikis/afa351f7c9/w

Test wiki created on Patch demo by GMikesell-WMF using patch(es) linked to this task:
https://patchdemo.wmflabs.org/wikis/d08cff28d9/w

Change 902567 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Move the diff-mode selector to the new DifferenceEngineBeforeDiffTable hook

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

@Samwilson I think this has caused a duplicate Visual/Wikitext switcher to appear when reviewing changes in VisualEditor:

testwiki_duplicate_switcher.png (720Γ—1 px, 81 KB)

Example is from testwiki but I can also reproduce on beta and locally.

I don't see the duplicate switcher locally when I check out the parent to that commit.

Change 914465 had a related patch set uploaded (by Samwilson; author: Samwilson):

[mediawiki/extensions/VisualEditor@master] Remove duplicated diff-mode selector in save dialog

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

The duplicate switcher is being added in the save dialog. In action=diff, the duplicate is replaced, but that doesn't happen for the save dialog diff review. I'll make a patch for a quick fix, but I think there are a couple of options for actually sorting it out.

Change 914465 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Remove duplicated diff-mode selector in save dialog

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

Change 914429 had a related patch set uploaded (by Samwilson; author: Samwilson):

[mediawiki/extensions/VisualEditor@wmf/1.41.0-wmf.7] Remove duplicated diff-mode selector in save dialog

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

We are also showing the switcher when doing a diff while editing ("Show Changes"). If you try to switch to the "Visual" diff (or have it already selected) you get an "Invalid response from server" error.

This does not affect Live Preview.

switcher_show_changes.png (841Γ—1 px, 94 KB)

switcher_show_changes.gif (768Γ—1 px, 813 KB)

Change 914429 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@wmf/1.41.0-wmf.7] Remove duplicated diff-mode selector in save dialog

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

Mentioned in SAL (#wikimedia-operations) [2023-05-03T07:18:40Z] <taavi@deploy1002> Started scap: Backport for [[gerrit:914429|Remove duplicated diff-mode selector in save dialog (T324759)]]

Mentioned in SAL (#wikimedia-operations) [2023-05-03T07:20:13Z] <taavi@deploy1002> taavi and samwilson: Backport for [[gerrit:914429|Remove duplicated diff-mode selector in save dialog (T324759)]] synced to the testservers: mwdebug2001.codfw.wmnet, mwdebug1001.eqiad.wmnet, mwdebug1002.eqiad.wmnet, mwdebug2002.codfw.wmnet

Mentioned in SAL (#wikimedia-operations) [2023-05-03T07:28:55Z] <taavi@deploy1002> Finished scap: Backport for [[gerrit:914429|Remove duplicated diff-mode selector in save dialog (T324759)]] (duration: 10m 14s)

Change 915365 had a related patch set uploaded (by Samwilson; author: Samwilson):

[mediawiki/extensions/VisualEditor@master] Don't show the diff-mode selector if not viewing a diff

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

We are also showing the switcher when doing a diff while editing ("Show Changes"). If you try to switch to the "Visual" diff (or have it already selected) you get an "Invalid response from server" error.

The above patch prevents the switcher from being shown for 'Show Changes', which avoids the issue. As far as I know, we're not planning on enabling switching visual/source while viewing changes in the wikitext editor. The switcher should only be shown when viewing a diff or in VE's save dialog when viewing changes.

Change 915365 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Don't show the diff-mode selector if not viewing a diff

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

Further to T324759#8722735, I tested the alignment of the legend and switcher in 5 skins with various zoom levels on:

  • Safari on enwiki beta
  • Firefox on arwiki beta (to test RTL behaviour)

I didn't notice any alignment issues. The switcher and legend are now vertically aligned.

legend_switcher_aligned.png (593Γ—1 px, 78 KB)

I checked that they interacted fine with RevisionSlider and FlaggedRevs on dewiki beta (e.g. https://de.wikipedia.beta.wmflabs.org/w/index.php?diff=25353&oldid=25352&title=Energie&diff-type=inline).

Change 915365 merged by jenkins-bot:

[mediawiki/extensions/VisualEditor@master] Don't show the diff-mode selector if not viewing a diff

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

This appears to fix the bug described in T324759#8822493. It also fixes the bug when doing a rollback or undo action.

I checked as many different types of diffs as I could find to see that the legend or switcher do not appear when they shouldn't. I tested everything here except mobile app, SpamDiffTool and WikEdDiff.

We still seem to have a bug with Translatewiki. See T336262#8843883. I will continue testing there and move this task along.

I also raised T336480, T336481 and T336482 for less urgent bugs.

Locally, I also tested:

  • wiki with VisualEditor but not wikidiff2
  • wiki with neither VE nor wikidiff2

Change 921773 had a related patch set uploaded (by Samwilson; author: Samwilson):

[mediawiki/core@master] diff: Add legend and tooltips to inline diff display

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

The following tickets will be closed since this new approach will solve the problems described in this tasks:

T336909: Logic for VE diff switch should have an inclusive check is no longer valid since a different solution in patch is being implemented.

The following tickets will be closed since this new approach will solve the problems described in this tasks:

T336909: Logic for VE diff switch should have an inclusive check is no longer valid since a different solution in patch is being implemented.

@Samwilson please let me know if this makes sense :) Thank you!

Yes, I think this is right.

I do wonder if we should reorganise the patches a bit, e.g.:

  1. add the new DifferenceEngineBeforeDiffTable hook, but with nothing in it (so it'd not change anything);
  2. update VE to use the new hook, to move the existing mode switch;
  3. add the type toggle to core, passing it in the hook's $parts paramater;
  4. add the legend, also in $parts.

That way, we don't have the awkward situation of things being implemented before they can be used (the legend) or of needing to merge extension and core patches at the same time in order for them to work properly and not break the display.

Yes, I think this is right.

I do wonder if we should reorganise the patches a bit, e.g.:

  1. add the new DifferenceEngineBeforeDiffTable hook, but with nothing in it (so it'd not change anything);
  2. update VE to use the new hook, to move the existing mode switch;
  3. add the type toggle to core, passing it in the hook's $parts paramater;
  4. add the legend, also in $parts.

That way, we don't have the awkward situation of things being implemented before they can be used (the legend) or of needing to merge extension and core patches at the same time in order for them to work properly and not break the display.

As discussed during our collaboration meeting, we'll continue with this approach and revert the MobileFrontend and VE patches.

Change 922640 had a related patch set uploaded (by Samwilson; author: Samwilson):

[mediawiki/extensions/VisualEditor@master] Move the diff-mode selector to the new DifferenceEngineBeforeDiffTable hook

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

Change 922789 had a related patch set uploaded (by Samwilson; author: Samwilson):

[mediawiki/extensions/MobileFrontend@master] diff: Hide legend for inline diffs

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

Test wiki created on Patch demo by HMonroy (WMF) using patch(es) linked to this task:
https://patchdemo.wmflabs.org/wikis/260273a763/w