Page MenuHomePhabricator

<!--__DTEMPTYTALKPAGE__--> turning indicator HTML from <link> to <span>
Closed, ResolvedPublic

Description

Given the wikitext <indicator name="test">blah</indicator>:

in https://test.wikipedia.org/api/rest_v1/page/html/Indicators you get: <link typeof="mw:Extension/indicator" about="#mwt3" data-mw='{"name":"indicator","attrs":{"name":"test"},"body":{"extsrc":"blah"}}' id="mwAw"/>

But in https://test.wikipedia.org/api/rest_v1/page/html/Talk:Indicators you get: <span typeof="mw:Extension/indicator" about="#mwt3" data-mw='{"name":"indicator","attrs":{"name":"test"},"body":{"extsrc":"blah"}}' id="mwAw"><!--__DTEMPTYTALKPAGE__--></span>

I couldn't find a Specs page for indicators so I don't know if this is a violation of the spec, but it seems wrong to me that indicators are different depending on the namespace they're in. I am assuming the behavior caused by __DTEMPTYTALKPAGE__ here is unintentional, so filing a bug.

I noticed this because the mwbot-rs test suite started failing because it could no longer find the indicator on the page since it was explicitly looking for a <link> tag.

Related Objects

Event Timeline

While Parsoid support for indicators is pending, I don't think you can assume that extensions return a specific tag type (in this case, you are assuming a <link> which is just a fallback case for when Parsoid doesn't know anything about the extension and it returns an empty html string). When Parsoid adds native support for indicators, which will be relatively soonish, your tests will fail again.

But, in the general case, extensions may return different html (with different wrappers) depending on input / params. There is nothing in the contract or spec that says that the output wrapper node should be the same for all inputs.

<link> which is just a fallback case for when Parsoid doesn't know anything about the extension and it returns an empty html string

https://github.com/wikimedia/parsoid/blob/master/src/Wt2Html/TT/ExtensionHandler.php#L220-L225

The empty talk page marker shouldn't be ending up inside the indicator tag. That seems like it could be a problem.

Parsoid still calls the legacy parser to parse extension tags that it doesn't have a native implementation for, which invokes onParserAfterTidy and adds DiscussionTools,

https://test.wikipedia.org/w/api.php?action=parse&contentmodel=wikitext&text=%3Cindicator%20name=%22test%22%3Eblah%3C/indicator%3E&showstrategykeys=0&disablelimitreport=1&wrapoutputclass=&title=Talk:Indicators

Whatever the call returns gets wrapped as the extension's content.

While Parsoid support for indicators is pending, I don't think you can assume that extensions return a specific tag type (in this case, you are assuming a <link> which is just a fallback case for when Parsoid doesn't know anything about the extension and it returns an empty html string). When Parsoid adds native support for indicators, which will be relatively soonish, your tests will fail again.

Thanks, this is helpful to know for future things I run into. Do we know what indicators will look like in the future?

But, in the general case, extensions may return different html (with different wrappers) depending on input / params. There is nothing in the contract or spec that says that the output wrapper node should be the same for all inputs.

Well in this case the input and parameters were identical, it was just the namespace of the page. But I get your general point. But I also expect to be able to rely on the spec to be comprehensive in what output formats tags will be in. Is that reasonable? (Obviously no spec exists here so it doesn't apply, but for other things)

Indicators are intended to be hoisted, so @Esanders' point is correct. The value returned by the indicator should end up in the ContentMetadataCollector/ParserOutput, not the output html. I expect that will be fixed and what Parsoid will output is an empty <span> or <meta> tag indicating the *location* of the indicator, while the actual content will get tucked away and not appear explicitly in the output HTML, although the contents will probably be made available via data-mw for editing. As @ssastry says, clients should in general be looking at the typeof, not the tag type, but in this case because the content is explicitly "swallowed" I'd wager that we'd end up with an empty span: <span...></span> for consistency with other tag-likes, although you could possibly make an argument for <meta> like we use for markers like __TOC__ (but to reinforce @ssastry's point, the principles of T204370 would suggest that {{#toc}} should eventually be equivalent to __TOC__ and we currently emit <span>/<div> for parser function invocations).

https://phabricator.wikimedia.org/T107332#8132136 has some suggestions about the API interface for indicators, but that shouldn't affect how they are represented in the HTML.

matmarex claimed this task.
matmarex subscribed.

Resolved in the subtask in March. Testing on https://test.wikipedia.org/api/rest_v1/page/html/Talk:Indicators again:

Before purging the page:

<!DOCTYPE html>
<html prefix="dc: http://purl.org/dc/terms/ mw: http://mediawiki.org/rdf/" about="https://test.wikipedia.org/wiki/Special:Redirect/revision/542851"><head prefix="mwr: https://test.wikipedia.org/wiki/Special:Redirect/"><meta property="mw:TimeUuid" content="f0c82b30-319d-11ed-bc01-0f4c450ffa63"/><meta charset="utf-8"/><meta property="mw:pageId" content="146170"/><meta property="mw:pageNamespace" content="1"/><link rel="dc:replaces" resource="mwr:revision/0"/><meta property="mw:revisionSHA1" content="dc2a3f8f0873c866750f517f03f433c3b5f2b0c4"/><meta property="dc:modified" content="2022-09-11T06:49:53.000Z"/><meta property="mw:htmlVersion" content="2.6.0"/><meta property="mw:html:version" content="2.6.0"/><link rel="dc:isVersionOf" href="//test.wikipedia.org/wiki/Talk%3AIndicators"/><base href="//test.wikipedia.org/wiki/"/><title>Talk:Indicators</title><meta property="mw:moduleStyles" content="ext.discussionTools.init.styles"/><link rel="stylesheet" href="/w/load.php?lang=en&amp;modules=ext.discussionTools.init.styles%7Cmediawiki.skinning.content.parsoid%7Cmediawiki.skinning.interface%7Csite.styles&amp;only=styles&amp;skin=vector"/><meta http-equiv="content-language" content="en"/><meta http-equiv="vary" content="Accept"/></head><body id="mwAA" lang="en" class="mw-content-ltr sitedir-ltr ltr mw-body-content parsoid-body mediawiki mw-parser-output" dir="ltr"><section data-mw-section-id="0" id="mwAQ"><p class="mw-empty-elt" id="mwAg"><span typeof="mw:Extension/indicator" about="#mwt3" data-mw='{"name":"indicator","attrs":{"name":"test"},"body":{"extsrc":"blah"}}' id="mwAw"><!--__DTEMPTYTALKPAGE__--></span></p></section></body></html>

After purging:

<!DOCTYPE html>
<html prefix="dc: http://purl.org/dc/terms/ mw: http://mediawiki.org/rdf/" about="https://test.wikipedia.org/wiki/Special:Redirect/revision/542851"><head prefix="mwr: https://test.wikipedia.org/wiki/Special:Redirect/"><meta property="mw:TimeUuid" content="cbed8c90-e81a-11ed-9c97-d91649f3f29d"/><meta charset="utf-8"/><meta property="mw:pageId" content="146170"/><meta property="mw:pageNamespace" content="1"/><link rel="dc:replaces" resource="mwr:revision/0"/><meta property="mw:revisionSHA1" content="dc2a3f8f0873c866750f517f03f433c3b5f2b0c4"/><meta property="dc:modified" content="2022-09-11T06:49:53.000Z"/><meta property="mw:htmlVersion" content="2.7.0"/><meta property="mw:html:version" content="2.7.0"/><link rel="dc:isVersionOf" href="//test.wikipedia.org/wiki/Talk%3AIndicators"/><base href="//test.wikipedia.org/wiki/"/><title>Talk:Indicators</title><meta property="mw:jsConfigVars" content='{"wgDiscussionToolsPageThreads":[]}'/><meta property="mw:moduleStyles" content="ext.discussionTools.init.styles"/><link rel="stylesheet" href="/w/load.php?lang=en&amp;modules=ext.discussionTools.init.styles%7Cmediawiki.skinning.content.parsoid%7Cmediawiki.skinning.interface%7Csite.styles&amp;only=styles&amp;skin=vector"/><meta http-equiv="content-language" content="en"/><meta http-equiv="vary" content="Accept"/></head><body id="mwAA" lang="en" class="mw-content-ltr sitedir-ltr ltr mw-body-content parsoid-body mediawiki mw-parser-output" dir="ltr"><section data-mw-section-id="0" id="mwAQ"><p id="mwAg"><link typeof="mw:Extension/indicator" about="#mwt2" data-mw='{"name":"indicator","attrs":{"name":"test"},"body":{"extsrc":"blah"}}' id="mwAw"/></p></section></body></html>

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

[mediawiki/core@REL1_39] Mark some parserTests on talk pages Parsoid only on REL1_39

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

Change 1002622 merged by jenkins-bot:

[mediawiki/core@REL1_39] Mark some parserTests on talk pages Parsoid only on REL1_39

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

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

[mediawiki/core@REL1_40] Mark some parserTests on talk pages Parsoid only on REL1_40

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

Change #1015099 merged by jenkins-bot:

[mediawiki/core@REL1_40] Mark some parserTests on talk pages Parsoid only on REL1_40

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