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

• Desktop: https://patchdemo.wmflabs.org/wikis/7c5b07966a/w/index.php?title=Main_Page&diff=153&oldid=151&diffmode=source&diff-type=inline
• Mobile: https://patchdemo.wmflabs.org/wikis/7c5b07966a/wiki/Special:MobileDiff/151...153?diffmode=source

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/Browser	View 100%	View 250%	View 300%
Windows 11/Chrome 111/Edge 111 & macOS 13.2 Safari

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/Browser	Windows 11 View 240%	macOS 13.2 View 240%
Firefox 111

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

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

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.

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:

• Desktop: https://patchdemo.wmflabs.org/wikis/09351dc76e/w/index.php?title=Main_Page&diff=152&oldid=151&diffmode=source&diff-type=inline
• Mobile: https://patchdemo.wmflabs.org/wikis/09351dc76e/wiki/Special:MobileDiff/151...152?diffmode=source

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.

Updated switcher designs

Visual	Wikitext - Two column	Wikitext - Inline	Review changes - Visual	Review changes - Wikitext

@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:

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

In T324759#8813953, @gerritbot wrote:

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:

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.

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.

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).

In T324759#8842876, @gerritbot wrote:

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:

• T336482: Legend appears on Special:MobileDiff when viewed on desktop
• T336582: Disable visual diffs in Special:AbuseFilter diffs

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

In T324759#8874993, @HMonroy wrote:

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

• T336482: Legend appears on Special:MobileDiff when viewed on desktop
• T336582: Disable visual diffs in Special:AbuseFilter diffs

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.

In T324759#8875363, @Samwilson wrote:

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