Page MenuHomePhabricator

Run another round of visual diff testing after tweaking core patches that change media output to <figure>, etc.
Open, MediumPublic

Description

We've done these tests in the past and made fixes but @cscott felt that we could do one final round of testing after tweaking and dusting off all patches before merge and deploy.

Event Timeline

ssastry triaged this task as Medium priority.Oct 21 2020, 3:07 PM
ssastry created this task.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 21 2020, 3:07 PM
Arlolra moved this task from Needs Triage to Media Structure on the Parsoid board.

A round of visual diff'ing is now running with the key "media-test", results at https://mw-expt-tests.wmflabs.org/

It's using PS16 of https://gerrit.wikimedia.org/r/c/mediawiki/core/+/507512

Base is $wgParserOutputLegacyMediaDOM = true; and expt is $wgParserOutputLegacyMediaDOM = false;

We might want to do a round of $wgParserOutputLegacyMediaDOM = true; vs HEAD^ to establish a baseline that nothing is broken.

Arlolra added a comment.EditedJan 15 2021, 9:39 PM

From the early results, https://mw-expt-tests.wmflabs.org/diff/enwiki/Anglo-Saxon%20settlement%20of%20Britain is an example of T169975#4176954

16:31 <+arlolra> ya, that's known
16:31 <+arlolra> https://www.mediawiki.org/wiki/Specs/HTML/2.1.0#Missing_media
16:31 <+arlolra> spans don't support a width
16:31 <+arlolra> we add a data-width so that can be dynamically fixed, if desired

https://mw-expt-tests.wmflabs.org/diff/dewiki/Liste%20der%20preu%C3%9Fischen%20Handelsminister is an example of https://gerrit.wikimedia.org/r/c/mediawiki/core/+/507512/16/includes/Linker.php#b417

In Parsoid, the caption for inline media is in the data-mw, which is equally true of broken media. However, core puts that as the link target for broken media. We should consider doing that away, despite the redundance.

It's captured in the test "Broken image links with HTML captions (T41700)"
https://gerrit.wikimedia.org/r/c/mediawiki/core/+/507512/16/tests/parser/mediaParserTests.txt#2173

https://mw-expt-tests.wmflabs.org/diff/enwiki/Diffusion shows that TimedMediaHandler isn't recognizing the new output. That should be handled in T272186

Also on https://mw-expt-tests.wmflabs.org/diff/enwiki/Diffusion, it appears that thumb at "Diffusion in the context of different disciplines" is clearing right, when it shouldn't be, since it's floating above the heading. That's a bug in the CSS that needs resolving.

WRT captions appearing in the error case:
The {MediaWiki DOM Spec](https://www.mediawiki.org/wiki/Specs/HTML/2.2.0#Images) says:

<figcaption (absent when inline)>...</figcaption>

and

The outer <figure> element needs to become a <span> element when the figure is rendered inline, since otherwise the HTML5 parser will interrupt a surrounding block context. The inner <figcaption> element is rendered as a data-mw attribute in this case (since block content in an invisible caption would otherwise break parsing).

So the issue here is a bit of ambiguity in what "when rendered inline" means, and we can be more precise about it. But probably "rendered inline" meaning "uses a span as the wrapper" and so if we decide to output the caption for broken error cases we should also change the outer wrapper from "span" to "figure". That might also cause rendering differences, but at least it would be self-consistent. Then we'd just need to clarify that media will be "rendered inline" only for non-error cases.

Change 657188 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[mediawiki/core@master] Audit margins in content.media.less

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

Also on https://mw-expt-tests.wmflabs.org/diff/enwiki/Diffusion, it appears that thumb at "Diffusion in the context of different disciplines" is clearing right, when it shouldn't be, since it's floating above the heading. That's a bug in the CSS that needs resolving.

Upon further investigation, if I remove video from T266149#6752524, the bug resolves itself, so it's more likely an issue with the extension than anything css related that need fixing.

Arlolra updated the task description. (Show Details)Jan 19 2021, 9:29 PM

Change 657188 merged by jenkins-bot:
[mediawiki/core@master] Audit margins in content.media.less

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

Arlolra updated the task description. (Show Details)Jan 21 2021, 11:18 PM

Change 657703 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[mediawiki/core@master] Revert most of the changes from 16b76a4

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

Change 657703 merged by jenkins-bot:
[mediawiki/core@master] Revert most of the changes from 16b76a4

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

https://mw-expt-tests.wmflabs.org/diff/dewiki/Ingvar_Carlsson is an example where the image in the figcaption overflows, resulting in a rendering difference. This similar to T171761 but the word-break isn't going to work here

Change 658667 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[mediawiki/core@master] Account from broken media in styling

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

Arlolra updated the task description. (Show Details)Jan 26 2021, 10:31 PM

Change 658667 merged by jenkins-bot:
[mediawiki/core@master] Account for broken media in styling

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

https://mw-expt-tests.wmflabs.org/diff/frwiki/Ic%C3%B4ne%20des%20proph%C3%A8tes%20Daniel%2C%20David%20et%20Salomon seems to be rounding difference introduced when $wgParserOutputLegacyMediaDOM = false; in I978187f9f6e9e0a105521ab3e26821e36a96b911. Needs investigation

Arlolra updated the task description. (Show Details)Jan 27 2021, 6:30 PM

Needs investigation

Looks like resources/src/mediawiki.page.gallery.js needs to be rejiggered to account for the structural differences. I guess this is part of T272186

Arlolra updated the task description. (Show Details)Jan 28 2021, 5:12 PM

https://mw-expt-tests.wmflabs.org/diff/enwiki/Assassination%20of%20James%20A.%20Garfield is similar to T266149#6775639 but here the filename is overflowing span. This can be fixed with word-break.

Arlolra updated the task description. (Show Details)Fri, Jan 29, 8:57 PM

https://mw-expt-tests.wmflabs.org/diff/dewiki/Freigericht%20(Hessen) is just a reminder that <imagemap> extension isn't installed on the vd vms. We probably need to enable a role?

We probably need to enable a role?

Ok, I ran vagrant roles enable imagemap and provisioned it. <imagemap> is rendered now.

Change 661169 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[mediawiki/core@master] Don't select on a.image

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

Change 661169 merged by jenkins-bot:
[mediawiki/core@master] Don't select on a.image

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

Change 661409 had a related patch set uploaded (by Arlolra; owner: Arlolra):
[mediawiki/core@master] [WIP] Break up overflowing filename

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

Arlolra updated the task description. (Show Details)Wed, Feb 3, 4:51 PM