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

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.

T266149#6752389: Dynamically adjust size from broken media - Filed as T273013
T266149#6752491: Caption as link content for broken media - Filed as T273014
T266149#6755826: Margin bottom only applies to thumbnails
T266149#6759641: TimedMediaHandler sets width to 100%
T266149#6781425: Rounding difference in galleries - Filed as T272186
T266149#6788204: Various overflowing differences

Details

Subject	Repo	Branch	Lines +/-
Copy upstream change to break up overflowing filename	mediawiki/skins/Timeless	master	+1 -0
Break up overflowing filename	mediawiki/core	master	+1 -0
Don't select on a.image	mediawiki/core	master	+1 -1
Account for broken media in styling	mediawiki/core	master	+14 -4
Revert most of the changes from 16b76a4	mediawiki/core	master	+41 -13
Audit margins in content.media.less	mediawiki/core	master	+23 -4

Customize query in gerrit

Related Objects

Mentioned In: T297984: Media html read view considerations
T273014: Caption as link content for inline broken media
Mentioned Here: T273014: Caption as link content for inline broken media
T273013: Dynamically adjust size for broken media
T171761: Figcaption overflows image width on unbroken words
rMW16b76a486d77: mediawiki.skinning: Show border-bottom when figcaption is absent on Parsoid
T272186: Update extensions to handle the new media structure
T41700: A File: link to a non-existing image can inject html
T169975: Missing images render as broken img tags, not redlinks

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 added a project: Parsoid.Jan 4 2021, 5:54 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.

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.

Only thumbnails should get the margin-bottom: 0.5em; from,
https://github.com/wikimedia/mediawiki/blob/master/resources/src/mediawiki.skinning/content.less#L110

See https://gerrit.wikimedia.org/r/c/integration/visualdiff/+/654311/1/lib/parsoid.custom_styles.yaml#b25

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

gerritbot added a project: Patch-For-Review.Jan 19 2021, 9:03 PM

In T266149#6752543, @Arlolra wrote:

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

ReleaseTaggerBot added a project: MW-1.36-notes (1.36.0-wmf.28; 2021-01-26).Jan 21 2021, 11:00 PM

Maintenance_bot removed a project: Patch-For-Review.Jan 21 2021, 11:10 PM

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

gerritbot added a project: Patch-For-Review.Jan 21 2021, 11:36 PM

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

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

Maintenance_bot removed a project: Patch-For-Review.Jan 22 2021, 8:10 PM

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

gerritbot added a project: Patch-For-Review.Jan 26 2021, 7:33 PM

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

Arlolra mentioned this in T273014: Caption as link content for inline broken media.Jan 26 2021, 10:38 PM

Arlolra updated the task description. (Show Details)

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

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

ReleaseTaggerBot edited projects, added MW-1.36-notes (1.36.0-wmf.29; 2021-02-02); removed MW-1.36-notes (1.36.0-wmf.28; 2021-01-26).Jan 27 2021, 10:00 AM

Maintenance_bot removed a project: Patch-For-Review.Jan 27 2021, 10:10 AM

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

In T266149#6781425, @Arlolra wrote:

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)Jan 29 2021, 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?

In T266149#6788451, @Arlolra wrote:

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

gerritbot added a project: Patch-For-Review.Feb 2 2021, 6:39 PM

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

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

Maintenance_bot removed a project: Patch-For-Review.Feb 2 2021, 10:10 PM

ReleaseTaggerBot edited projects, added MW-1.36-notes (1.36.0-wmf.30; 2021-02-09); removed MW-1.36-notes (1.36.0-wmf.29; 2021-02-02).Feb 2 2021, 11:00 PM

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

gerritbot added a project: Patch-For-Review.Feb 3 2021, 4:39 PM

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

A case where maybe overflow: hidden was desirable?

https://mw-expt-tests.wmflabs.org/diff/frwikivoyage/Roumanie
https://mw-expt-tests.wmflabs.org/diff/frwikivoyage/Maurice
https://mw-expt-tests.wmflabs.org/diff/frwikivoyage/Venezuela

Empty figcaption is a little higher https://mw-expt-tests.wmflabs.org/diff/frwiki/Astrolabe

ssastry added a project: Parsing-Active-Work.Feb 4 2021, 6:59 PM

ssastry moved this task from Incoming Work (but not yet ready) to Doing (Roadmap Critical Path) on the Parsing-Active-Work board.

Arlolra edited projects, added Parsoid-Media-Structure; removed Parsoid.Jul 13 2021, 5:14 PM

MBinder_WMF added a project: Parsoid-Read-Views-Deprecated-Project (Phase 0 - Parsoid-Media-Structure).Sep 3 2021, 9:30 PM

MBinder_WMF edited projects, added Parsoid; removed Parsoid-Read-Views-Deprecated-Project (Phase 0 - Parsoid-Media-Structure).Sep 3 2021, 9:37 PM

MBinder_WMF added a project: Parsoid-Read-Views (Phase 0 - Parsoid-Media-Structure).Sep 3 2021, 9:58 PM

I think everything here is either fixed or captured elsewhere now.

We could probably use another fresh round of visual diff'ing to see where we're at.

Arlolra moved this task from Backlog to To Verify on the Parsoid-Read-Views (Phase 0 - Parsoid-Media-Structure) board.Jun 15 2022, 3:32 PM

Arlolra mentioned this in T297984: Media html read view considerations.Aug 18 2022, 4:12 PM

Arlolra updated the task description. (Show Details)Jun 19 2023, 5:45 PM

Change 661409 merged by jenkins-bot:

[mediawiki/core@master] Break up overflowing filename

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

Maintenance_bot removed a project: Patch-For-Review.Jun 19 2023, 8:10 PM

Arlolra updated the task description. (Show Details)Jun 19 2023, 8:28 PM

ReleaseTaggerBot added a project: MW-1.41-notes (1.41.0-wmf.15; 2023-06-27).Jun 19 2023, 9:00 PM

Arlolra updated the task description. (Show Details)Jun 22 2023, 7:47 PM

Change 932346 had a related patch set uploaded (by Arlolra; author: Arlolra):

[mediawiki/skins/Timeless@master] Copy upstream change to break up overflowing filename

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

gerritbot added a project: Patch-For-Review.Jun 22 2023, 10:48 PM

Change 932346 merged by jenkins-bot: