Page MenuHomePhabricator

[SPIKE] - Investigate replacing mobile diffs with diffs for moderator tools work [16hrs]
Closed, ResolvedPublic

Description

Per T328035: [SPIKE] - Investigate technical approaches for improving mobile diffs [32hrs], we need to identify where we want to make our changes to the diff experience; in this task, I'm taking a look at what it might take to implement some of the currently-under-research changes in the core-provided inline diffs rather than the mobilefrontend mobileDiff

Event Timeline

jsn.sherman changed the task status from Open to In Progress.EditedFeb 6 2023, 6:16 PM

The header is setup in the difference engine and uses a hook to allow modification. If we want to change up the order without using an extension, the best bet would be a css/js-only approach. That's the first thing I'll try.

https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/core/+/refs/heads/master/includes/diff/DifferenceEngine.php#804

			$oldHeader = '<div id="mw-diff-otitle1"><strong>' . $oldRevisionHeader . '</strong></div>' .
				'<div id="mw-diff-otitle2">' .
				Linker::revUserTools( $oldRevRecord, !$this->unhide ) . '</div>' .
				'<div id="mw-diff-otitle3">' . $oldminor . $oldRevComment . $ldel . '</div>' .
				'<div id="mw-diff-otitle5">' . $oldChangeTags[0] . '</div>' .
				'<div id="mw-diff-otitle4">' . $prevlink . '</div>';
jsn.sherman added a subscriber: Jdlrobson.

I think we're really picking a fight with this approach to present ui elements such as buttons. Among other things, DifferenceEngine.php wraps elements with innerhtml parenthesis
https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/core/+/refs/heads/master/includes/diff/DifferenceEngine.php#852

		// Put each one in parentheses (poor man's button)
		foreach ( $revisionTools as $key => $tool ) {
			$toolClass = is_string( $key ) ? $key : 'mw-diff-tool';
			$element = Html::rawElement(
				'span',
				[ 'class' => $toolClass ],
				$this->msg( 'parentheses' )->rawParams( $tool )->escaped()
			);
			$formattedRevisionTools[] = $element;
		}

so even if I manage to style something from that table to look like a button, the button label isn't clean.

image.png (707×408 px, 67 KB)

I think we'd be much better off if we could generate appropriate html to start with. The only thing I can think of is to add another query parameter to vary the markup it outputs so that the current diffs stay the same, but a different ui could be presented. That would split the cache, but I don't think that would be any worse than separate pages for desktop/mobile.

@Jdlrobson, were you imagining upstreaming any of the markup changes from Mobilediff for T240624: Style desktop Minerva diff page to look like Special:MobileDiff ? Do you have any recommended approaches to this?

jsn.sherman renamed this task from [SPIKE] - Investigate replacing mobile diffs with diffs for moderator tools work [8hrs] to [SPIKE] - Investigate replacing mobile diffs with diffs for moderator tools work [16hrs].Feb 8 2023, 8:33 PM

The mediawiki.interface.helpers.styles module should be helpful here. Are you familiar with it? Instead of hardcoding parentheses in the text, it moves those to CSS allowing more options on the skinning level.

Note there are lots of parts of the cache to think about: Diffs have their own cache, there is HTML varnish cache for the entire HTML of the page. I believe that currently varies on query string. If you are only concerned with the inlinediff mode (diff-type=inline) [1], then you should be able to vary the HTML based on the query string using OutputPage::getRequest(). However, if you are looking to vary HTML on the same URL that's not going to happen. You will need to find a solution where HTML is the same but CSS differs based on skin.

[1] https://en.wikipedia.org/w/index.php?title=San_Francisco&type=revision&diff=687927333&oldid=687926997&diffonly=1&useskin=minerva&diff-type=inline&diffmode=source

Helpful! I was imagining adding a new query parameter so that the display would maintain consistency if ui for changing diff type is added down the road, but that could be left for later.

I should note, that I'm not sure if diff-type=inline is being cached in any way. Ideally the result of wikidiff2 should be cached and the entire page go through Varnish. It's not linked to in production yet, so I'd recommend checking that before adding more eye balls.

I personally would recommend adding the HTML to all pages, and then hiding it using skinStyles if you can as that will give you the most flexibility around rolling this out to more skins in future. I'd love to see some of your ideas in T328035 implemented in Vector 2022 one day! For example I've seen a mock with a sticky toolbar which contains Undo and thanks links - I could see that being useful in a responsive version of Vector 2022. tldr: Don't vary HTML if you can avoid it!

I should note, that I'm not sure if diff-type=inline is being cached in any way. Ideally the result of wikidiff2 should be cached and the entire page go through Varnish. It's not linked to in production yet, so I'd recommend checking that before adding more eye balls.

100% makes sense

I personally would recommend adding the HTML to all pages, and then hiding it using skinStyles if you can as that will give you the most flexibility around rolling this out to more skins in future. I'd love to see some of your ideas in T328035 implemented in Vector 2022 one day! For example I've seen a mock with a sticky toolbar which contains Undo and thanks links - I could see that being useful in a responsive version of Vector 2022. tldr: Don't vary HTML if you can avoid it!

I think that makes sense as an end goal, but how can we take incremental steps to get there starting with the mobile experience? Making an across the board change seems like a big step up in community relations scope, and I feel way out of my depth on how to proceed with that as an engineer.

We took this approach on the Special:Contributions page - for example headings show on mobile skin but not desktop skin. We did this by introducing hidden HTML elements. The important thing here would be using visual regression testing to ensure no visible UI changes to desktop site. Happy to talk through this approach with you on Monday.

I had a good chat with jason about this yesterday (and we did a screen recording if anyone is interested) on how we might approach this.

Jon's team had a very similar problem to solve with Special Contributions.
They created some elements that have display:none by default but shows for screen readers in mediawiki.specials.changeslist

they defined a default skin style with display:none
https://gerrit.wikimedia.org/g/mediawiki/core/+/f139afe1cf2ffe1bcbab35eab594fc904590b20b/resources/Resources.php#2134
https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/core/+/f139afe1cf2ffe1bcbab35eab594fc904590b20b/resources/src/mediawiki.special.changeslist/default.less

minerva replaces that with it's own styling
https://gerrit.wikimedia.org/g/mediawiki/skins/MinervaNeue/+/90dde889128cb96984d91c558eece5c956af155b/skin.json#254
https://gerrit.wikimedia.org/g/mediawiki/skins/MinervaNeue/+/90dde889128cb96984d91c558eece5c956af155b/skinStyles/mediawiki.special.changeslist.less

Where we have those parentheses mucking things up, we can replace them with css-based separators using helpers
https://github.com/wikimedia/mediawiki/blob/master/resources/src/mediawiki.interface.helpers.styles.less#L65-L78

We should consider using pixel to ensure that our changes don't impact desktop
https://github.com/wikimedia/pixel

Our next step should be to do that prep work on the existing diff markup

The work to implement the changes would follow on that.

Jon also suggested that we might want to implement mobile permissions or config to allow projects to manage who the undo / revert / rollback appears for on mobile