Page MenuHomePhabricator

Visual diff regression on enwikivoyage
Closed, ResolvedPublic

Description

I am seeing a number of visual diff regressions on enwikivoyage in the latest run (I didn't run last week).

See rendering of banner 0 vs banner 1 on https://en.wikivoyage.org/wiki/Talk:Antarctic_Peninsula

image.png (613×1 px, 1 MB)

Compare rendering with Parsoid: https://en.wikivoyage.org/api/rest_v1/page/html/Talk%3AAntarctic_Peninsula
image.png (634×1 px, 1 MB)

I suspect this is related to rollout of CSS changes with the new media dom structure.

Event Timeline

Arlolra added a subscriber: Arlolra.
<figure typeof="mw:File/Thumb"><span data-mw-comment-start="" id="c-Ikan_Kekek-2022-02-16T06:17:00.000Z-Banner"></span><a href="/wiki/File:Antarctic_Peninsula-banner.jpg" class="mw-file-description"><img alt="" src="//upload.wikimedia.org/wikipedia/commons/thumb/8/82/Antarctic_Peninsula-banner.jpg/700px-Antarctic_Peninsula-banner.jpg" decoding="async" width="700" height="100" srcset="//upload.wikimedia.org/wikipedia/commons/thumb/8/82/Antarctic_Peninsula-banner.jpg/1050px-Antarctic_Peninsula-banner.jpg 1.5x, //upload.wikimedia.org/wikipedia/commons/thumb/8/82/Antarctic_Peninsula-banner.jpg/1400px-Antarctic_Peninsula-banner.jpg 2x" data-file-width="2100" data-file-height="300"></a><figcaption>Banner 0</figcaption></figure>

Some script is modifying to page to insert <span data-mw-comment-start="" id="c-Ikan_Kekek-2022-02-16T06:17:00.000Z-Banner"></span> and the position as the first child is messing with the css. This is related to the decisions made in T314097

Some script is modifying to page to inser

Oh, hmm, wait, this seems present in the parser output, from DiscussionTools

The wikitext is

== Banner ==

While I like the current banner, given this is one of the more touristed areas of Antarctica, am wondering whether a banner of some hikers would be nice to feature. --<span style="font-family:BlinkMacSystemFont">[[User:SHB2000|SHB2000]] <small>([[User talk:SHB2000|talk]] &#124; [[Special:Contributions/SHB2000|contribs]] &#124; [[m:User:SHB2000|meta.wikimedia]])</small></span> 05:53, 16 February 2022 (UTC)
[[File:Antarctic Peninsula-banner.jpg|thumb|700px|Banner 0]]
[[File:Antarctic Peninsula banner.jpg|thumb|700px|Banner 1]]

:Banner 0, without the hikers, is nicer to look at. You can put yourself vicarious in the picture as the one who's seeing the view; it's not essential to put other people in the view. [[User:Ikan Kekek|Ikan Kekek]] ([[User talk:Ikan Kekek|talk]]) 06:17, 16 February 2022 (UTC)

and it looks like CommentFormatter::addDiscussionToolsInternal is putting the thread item start marker in an awkward place. I would assume that it wouldn't be hoisted outside the dd or at least placed above the figure, not inside it.

We have some hacks for figures already: https://gerrit.wikimedia.org/g/mediawiki/extensions/DiscussionTools/+/0e6cd67d164694f9f61e7ad26dbd71abed1bc128/includes/CommentUtils.php#125 so we can probably add another one.

However, I think the real problem is that the CSS depends so critically on the HTML structure. Would it be possible to use classes instead of > a and :first-child etc.? DiscussionTools is not the only thing that inserts content into the page. This is likely to also affect e.g. the video player.

The existing hacks there have this pessimistic comment: "We also can't return true [to insert stuff outside of it rather than inside] for the whole 'figure' (or '.thumb' in legacy DOM), because we might want to handle content in the caption.", which I wrote myself, but on a closer look it appears to not be true. Inserting the markers outside doesn't prevent us from handling replies inside the caption, or even replying to them (although this breaks on the wikitext level, since list item syntax isn't allowed there).

The only negative effect is that the higlights when linking to a comment now cover the full width of the page, rather than just the image. This seems acceptable, given that there is approximately one page where comments in image captions are the norm.

Change 840340 had a related patch set uploaded (by Bartosz Dziewoński; author: Bartosz Dziewoński):

[mediawiki/extensions/DiscussionTools@master] Don't insert comment markers inside <figure>

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

However, I think the real problem is that the CSS depends so critically on the HTML structure. Would it be possible to use classes instead of > a and :first-child etc.?

Yeah, we explored that in T314097 and, for now, would like to see how feasible it is this way. The direct descendent is somewhat unavoidable because of nested media in the figcaption.

This is likely to also affect e.g. the video player.

It did but was accounted for in T304010 and https://gerrit.wikimedia.org/r/c/mediawiki/extensions/TimedMediaHandler/+/812387/

I would assume that it wouldn't be hoisted outside the dd or at least placed above the figure, not inside it.

The banners there are part of the first comment, even though they were placed after the signature,
https://en.wikivoyage.org/w/index.php?title=Talk%3AAntarctic_Peninsula&type=revision&diff=4390000&oldid=4339396
https://en.wikivoyage.org/w/index.php?title=Talk%3AAntarctic_Peninsula&type=revision&diff=4390005&oldid=4390000

Change 840340 merged by jenkins-bot:

[mediawiki/extensions/DiscussionTools@master] Don't insert comment markers inside <figure>

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

matmarex added a project: Editing QA.
matmarex updated the task description. (Show Details)

After today's deploy and an purge action, the html that follows now has the comment preceding the figure and styles apply as expected.

...
</p>
<span data-mw-comment-start="" id="c-Ikan_Kekek-2022-02-16T06:17:00.000Z-Banner"></span><figure typeof="mw:File/Thumb"><a href="/wiki/File:Antarctic_Peninsula-banner.jpg" class="mw-file-description"><img alt="" src="//upload.wikimedia.org/wikipedia/commons/thumb/8/82/Antarctic_Peninsula-banner.jpg/700px-Antarctic_Peninsula-banner.jpg" decoding="async" width="700" height="100" srcset="//upload.wikimedia.org/wikipedia/commons/thumb/8/82/Antarctic_Peninsula-banner.jpg/1050px-Antarctic_Peninsula-banner.jpg 1.5x, //upload.wikimedia.org/wikipedia/commons/thumb/8/82/Antarctic_Peninsula-banner.jpg/1400px-Antarctic_Peninsula-banner.jpg 2x" data-file-width="2100" data-file-height="300"/></a><figcaption>Banner 0</figcaption></figure>
<figure typeof="mw:File/Thumb"><a href="/wiki/File:Antarctic_Peninsula_banner.jpg" class="mw-file-description"><img alt="" src="//upload.wikimedia.org/wikipedia/commons/thumb/e/e1/Antarctic_Peninsula_banner.jpg/700px-Antarctic_Peninsula_banner.jpg" decoding="async" width="700" height="100" srcset="//upload.wikimedia.org/wikipedia/commons/thumb/e/e1/Antarctic_Peninsula_banner.jpg/1050px-Antarctic_Peninsula_banner.jpg 1.5x, //upload.wikimedia.org/wikipedia/commons/thumb/e/e1/Antarctic_Peninsula_banner.jpg/1400px-Antarctic_Peninsula_banner.jpg 2x" data-file-width="2499" data-file-height="356"/></a><figcaption>Banner 1</figcaption></figure>
<dl><dd>Banner 0...

So, it looks good to me but I'll leave this open for Editing QA to resolve. Also, @ssastry can report on the next visual diff run.

This is now fixed in visual diff testing.