Page MenuHomePhabricator

Parsoid image output is (intentionally) missing the magnify links found in PHP parser output, leading to differences
Closed, ResolvedPublic0 Story Points

Description

As part of visual diff testing, I found that one source of really common diffs is the absence of the magnify button found below thumbnails on all pages. It seems part of core PHP parser output and is not something that is added by a skin or gadgets or extensions.

I think we should replicate this in Parsoid as well. Would this affect VE (and Flow and CX that use VE) behavior or MCS behavior?

Event Timeline

ssastry created this task.Mar 20 2017, 10:13 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 20 2017, 10:13 PM

We considered removing the magnify icon in the past. IIRC the main concern about that was about the edge case where the image itself is linked elsewhere.

Tgr added a subscriber: Tgr.EditedMar 20 2017, 10:39 PM

I think the main concern is that people don't realize the thumbnail itself is clickable (the icon used to get a surprisingly high share of the thumbnail clicks). A combination of link+thumb is very rare, I think.

Jdforrester-WMF renamed this task from Parsoid image output is missing the magnify links found in PHP parser output to Parsoid image output is (intentionally) missing the magnify links found in PHP parser output, leading to differences.
Jdforrester-WMF set the point value for this task to 0.
GWicke added a comment.EditedMar 21 2017, 7:31 PM

I don't think the rendering difference is intentional. It's more that nobody has gotten around to either

a) remove the icon in the MediaWiki parser output, or
b) adding CSS / JS to show the icon when using Parsoid HTML, without making presentation concerns like this permanently part of the Parsoid DOM.

It was intentionally not done in Parsoid output, because at the time we wanted to do the former. But as @Tgr says it apparently turns out that people don't recognise images are clickable, but somehow do think this icon is(?!).

Option (b) seems sensible.

ssastry triaged this task as Normal priority.Apr 24 2017, 10:02 PM

Is this really related to ContentTranslation?

Is this really related to ContentTranslation?

I had tagged CX because of this question in the description: 'Would this affect VE (and Flow and CX that use VE) behavior or MCS behavior?'

Oh, OK, missed it somehow.

I guess this is a question for @santhosh and @dchan.

Change 363861 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[mediawiki/core@master] [WIP] T160960: Add magnify links to Parsoid output

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

Arlolra claimed this task.Jul 21 2017, 7:39 PM

Change 367378 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/extensions/VisualEditor@master] Remove code for magnify icon from figcaption node

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

Pinging @Volker_E as he had some concerns about removing the .magnify div?

Thanks @Esanders – I don't have real concerns on removing the class, I was wondering if we allow or in any way imply users to use .magnify class as it's currently used in several places like Minerva/MobileFrontend or MediaWiki-extensions-MultimediaViewer

Esanders added a comment.EditedJul 25 2017, 6:48 PM

Thanks.

In Minerva there's a display:none CSS rule that should be updated (otherwise VE + Minerva will start showing the icon again)

In MMV we have some selectors that work on PHP or Parsoid DOM, that need updating (so MMV works on Parsoid renders, e.g. Flow). The jQuery selectors will just match nothing. As the new magnify icon is part of the main image <a> it should just work with the existing click event.

Change 367717 had a related patch set uploaded (by Esanders; owner: Esanders):
[mediawiki/skins/MinervaNeue@master] Disable magnify icon in Parsoid output

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

Change 363861 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[mediawiki/core@master] resources: Add magnify links to Parsoid output

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

Change 363861 merged by jenkins-bot:
[mediawiki/core@master] mediawiki.skinning: Add magnify links to Parsoid output

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

Change 367717 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Disable magnify icon in Parsoid output

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

Change 367378 merged by jenkins-bot:
[mediawiki/extensions/VisualEditor@master] Remove code for magnify icon from figcaption node

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

Jdforrester-WMF closed this task as Resolved.Aug 9 2017, 4:51 PM
Jdforrester-WMF removed a project: Patch-For-Review.
Restricted Application added a project: User-Ryasmeen. · View Herald TranscriptAug 9 2017, 4:51 PM