Page MenuHomePhabricator

VisualEditor normalizing LST tags outside of edited section
Closed, ResolvedPublicBUG REPORT

Description

In https://en.wikipedia.org/w/index.php?title=Cricket_at_the_2022_Asian_Games_%E2%80%93_Women%27s_tournament&diff=prev&oldid=1320354942

Section tag attributes were given quotes. I thought the point of Selser was to not do that ...

<section begin=bronze medal match /> was changed to <section begin="bronze" medal="" match="" />. The former syntax is valid and can be transcluded, the latter is not.

Event Timeline

Izno changed the subtype of this task from "Task" to "Bug Report".Nov 9 2025, 11:02 PM

Reproducible on https://en.wikipedia.org/w/index.php?title=User:SSastry_(WMF)/T409675&veaction=edit ... so, something is busted here. The question is if Parsoid is messing up or if VE is normalizing something it shouldn't.

I can reproduce this locally via the php bin/parse.php script without involving VE which makes it simpler to debug. Looks like dom diff is marking the LST tags as modified for some reason: <link typeof="mw:Extension/section" about="#mwt32" data-parsoid='{"dsr":[7782,7802,20,0]}' data-mw='{"name":"section","attrs":{"begin":"B1"}}' data-parsoid-diff='{"diff":["modified-wrapper"]}'/> ... so need to look at the old & new html and why dom diff is marking it modified.

Huh ...

[DOM-DIFF]     | --> A : <link typeof="mw:Extension/section" about="#mwt27" data-parsoid='{"dsr":[6521,6541,20,0]}' data-mw='{"name":"section","attrs":{"begin":"A1"}}'/>
[DOM-DIFF]     | --> B : <link typeof="mw:Extension/section" about="#mwt27" data-parsoid='{"dsr":[6521,6541,20,0]}' data-mw='{"name":"section","attrs":{"begin":"A1"}}'/>
[DOM-DIFF]     | -- not equal --

Interesting, failing on the strict data-parsoid check because of the tmp blob.

A-val: {"tmp":{"bits":0},"dsr":{"start":6521,"end":6541,"source":null,"openWidth":20,"closeWidth":0,"leadingWS":0,"trailingWS":0}}
B-val: {"dsr":{"start":6521,"end":6541,"source":null,"openWidth":20,"closeWidth":0,"leadingWS":0,"trailingWS":0}}

Okay .. I see what is happening. DataParsoid::getTemp()creates a TempData object if one doesn't exist. And, adding some tracing shows that in some DOMDiff code paths, we call WTUtils::emitsSolTransparentSingleLineWT.. which in the call chain calls WTUtils::isSolTransparentLink which calls DOMDataUtils::getDataParsoid( $node )->getTemp()->empty and that now initializes tmp on the original node. So, depending on the specific edit, and the code paths that execute during DOM diff and the parts of the DOM tree that are examined, on some code paths, we effectively make either the original or edited DOM node mismatch because of this! This could have been a source of many random dirty diffs over time that are hard to track down. It just happened that on this page, it was readily reproducible without VE.

Trying to do a non-object comparison will make the diffs slower since data-parsoid is compared for all nodes ... so, lets see if we can find a different solution that doesn't involve comparing individual non-temp properties of data-parsoid.

Change #1311543 had a related patch set uploaded (by Subramanya Sastry; author: Subramanya Sastry):

[mediawiki/services/parsoid@master] WIP: Fix source of spurious dom-diffs that in turn caused dirty diffs

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

That patch fixes the issue and eliminates a dirty diff.

There is another part of this bug.

<section begin=bronze medal match /> was changed to <section begin="bronze" medal="" match="" />. The former syntax is valid and can be transcluded, the latter is not.

This is T101841: Value-less extension attributes not preserved

Change #1311543 merged by jenkins-bot:

[mediawiki/services/parsoid@master] Fix source of spurious dom-diffs that in turn caused dirty diffs

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

Won't selser stop that from happening now that the first issue is fixed, though?

Yes, but, if that element is edited (or if dom-diff marks it edited .... occasionally, there are other reasons that dom-diff is not accurate), it will get normalized to that form since selser won't be involved. So, for that case, T101841 needs to be addressed.

I personally dislike these "create the object if it doesn't exist" getters, for reasons such as these. Calling DOMDataUtils::getDataParsoid( $node )->getTempIfExists()?->empty would fix this bug but it's a pattern which is a foot-gun IMO.

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

[mediawiki/vendor@master] Bump wikimedia/parsoid to 0.24.0-a16

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

Change #1314959 merged by jenkins-bot:

[mediawiki/vendor@master] Bump wikimedia/parsoid to 0.24.0-a16

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

Reproducible on https://en.wikipedia.org/w/index.php?title=User:SSastry_(WMF)/T409675&veaction=edit ... so, something is busted here. The question is if Parsoid is messing up or if VE is normalizing something it shouldn't.

This is now fixed. Editing the infobox in VE and "Publish Changes" > "Review your changes" shows that there are no dirty diffs anymore.