Page MenuHomePhabricator

Charts are not compatible with Parsoid - show as raw SVG
Open, HighPublicBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

Also an issue in VE:

What happens?:

image.png (679×1 px, 290 KB)

I'm seeing the inlined SVG.

What should have happened instead?:

The chart should render and be easy to edit.

Software version (on Special:Version page; skip for WMF-hosted wikis like Wikipedia):

Other information (browser name/version, screenshots, etc.):

Event Timeline

CCiufo-WMF set the point value for this task to 2.Sep 23 2024, 6:41 PM
CCiufo-WMF moved this task from Backlog to Sprint 7 on the Charts board.
CCiufo-WMF edited projects, added Charts (Sprint 7); removed Charts.

I could be missing something, but it looks like the VisualEditor does not appear to support parser functions as well, compared to parser tags.

Other extensions such as Score and Math, as well as the Graph extension, use parser tags and integrate with the VisualEditor as a plugin. The parser tag is handled as a node, with some default handling that is way better than seeing the raw svg when editing.

Screenshot 2024-09-22 at 11.08.49 AM.png (1×2 px, 223 KB)

With a parser tag, we could fairly easily create a plugin for the VisualEditor with more customized functionality for selecting a chart definition and tabular data source.

A parser tag might not be a perfect fit for our use case, as they typically have content between the opening and closing tab, but think that is optional. The <references/> tag in the Cite extension usually doesn't have content inside the tag.

If we want to stick a parser function, then we would need to consult with the editing team and investigate further how to create a plugin (if possible) that works with the parser function. Or if there is anything else we can do.

Parser tag version of the chart syntax, with ignoring anything inside the tag:

<chart data="1993 Canadian federal election.tab" definition="1993 Canadian federal election.chart" width="500" height="400">this is ignored</chart>

From an implementation point-of-view, if we want to switch to a parser tag, it is a small change in implementation and the chart looks the same.

Screenshot 2024-09-24 at 8.31.38 PM.png (1×1 px, 203 KB)

Parser functions are viewed by Parsoid (and thus by VisualEditor) as templates. See the output of the parse API which I'll excerpt here:

<div class="mw-chart" about="#mwt1" typeof="mw:Transclusion" data-mw='{"parts":[{"template":{"target":{"wt":"#chart:1993 Canadian federal election.chart","function":"chart"},"params":{},"i":0}}]}' id="mwAw">A LOT OF MESSILY RENDERED AS WIKITEXT SVG</div>

Fortunately, VE has a system that lets an extension claim ownership over fairly arbitrary things, by specifying them more precisely than our core handlers. You can see an example of this in Cite, which takes over templates that generate a <references> tag. If you did this, you'd be on the hook for turning whatever Parsoid gives you into an actual useful rendering (which looks like kind of a pain with how mangled it is in that linked API reponse), or fetching something more appropriate via whatever charts APIs will exist.

I know that Abstract was looking into doing this for their functions, so it's possible they'll have something that's exactly-what-you're-doing to crib from, but I don't know how far they've gotten with it. I'm happy to give more information about this if you want to pursue it.

Incidentally, @aude is right that the default experience if you do absolutely nothing is going to be a lot nicer if you use a tag rather than a function. This is what the InputBox extension gets from its <inputbox> without writing anything for VE to use at all:

image.png (510×854 px, 38 KB)

I added a superfluous attribute to the <inputbox> that it doesn't actually use just to demonstrate that it gets picked up by VE.

If you decide to add some custom handling, they're equivalent in terms of what you can do with approximately-equal amounts of effort. If you don't want to deal with this at all the tag has some clear advantages.

We chatted about this yesterday and were open to changing this from a technical perspective but hesitant to make a major change to the syntax until we had a clear design on how this should behave in VE. While it's tempting to get VisualEditor behaviour for free, we're unlikely to ship with that so this would only be helpful on the short term.

Jdlrobson added a subscriber: ssastry.

This is also a problem in Parsoid HTML https://en.wikipedia.beta.wmflabs.org/w/index.php?title=Charts_2.0&useparsoid=1
@ssastry suggested that the sanitizer in Parsoid perhaps hasn't been updated. If this is a bug in the sanitizer, I think this is a suitable fix and there is no need to change our syntax.

Jdlrobson renamed this task from Editing a chart with VE shows raw SVG to Charts show as raw SVG in Parsoid HTML.Sep 26 2024, 7:13 PM
Jdlrobson updated the task description. (Show Details)

I (for one) strongly prefer the parser function syntax. We'll make this work.

I don't think it's a matter of the sanitizer in Parsoid diverging from core, I think it's more likely a matter of how the sanitizer in Parsoid applies even to certain types of generated HTML, whereas in core that content is protected via strip state and never gets sanitized. Needs some investigation.

If it's expected that the Parsoid-HTML rendering of the chart is going to get fixed, it'll be pretty easy to write whatever you need for purely the editing side of this for VE.

This is a dup of T279094: Support for parser functions using "isHTML"?, and could probably be worked around by implementing a Parsoid-specific hook, but I do plan on working on T279094 in the near future for https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1071016

There's some synergy with wikifunctions here, who are likely going to prototype a new parser function API/DOM representation for us (T373253) in Q2; we might want to have Charts hop on that same bandwagon.

This is a dup of T279094: Support for parser functions using "isHTML"?, and could probably be worked around by implementing a Parsoid-specific hook, but I do plan on working on T279094 in the near future for https://gerrit.wikimedia.org/r/c/mediawiki/core/+/1071016

Is this something you'll be picking up soon-ish? We're aiming to deploy charts to production in the coming months and would want to implement a workaround if the parser fix isn't going to be resolved for a while.

CCiufo-WMF edited projects, added Charts; removed Charts (Sprint 7).
Jdlrobson renamed this task from Charts show as raw SVG in Parsoid HTML to Charts are not compatible with Parsoid - show as raw SVG.Tue, Nov 5, 6:15 PM

@CCiufo-WMF thinking ahead to the pilot wiki release - is this a blocker? In current state any editor who has opted into Parsoid or is editing via VisualEditor will not be able to render charts. One consideration here is that I suspect many of the technical editors we want feedback from might be opted into that state.

@CCiufo-WMF thinking ahead to the pilot wiki release - is this a blocker? In current state any editor who has opted into Parsoid or is editing via VisualEditor will not be able to render charts. One consideration here is that I suspect many of the technical editors we want feedback from might be opted into that state.

Yes I'd consider this a blocker...has there been any progress on addressing it?

Chatted to @Dbrant and this is actually breaking the apps display of charts in read mode.

Change #1088365 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/core@master] Parsoid DataAccess: support strip state in parser function results

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

aude removed aude as the assignee of this task.Thu, Nov 7, 9:08 PM
aude subscribed.

https://gerrit.wikimedia.org/r/1088365 should resolve this bug when it lands and rides the train.

CCiufo-WMF removed the point value for this task.Fri, Nov 15, 5:29 PM

Change #1088365 merged by jenkins-bot:

[mediawiki/core@master] Parsoid DataAccess: support strip state in parser function results

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

Change #1092341 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[operations/mediawiki-config@master] Enable experimental Parsoid fragment support on labs and test wikis

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

Change #1092341 merged by jenkins-bot:

[operations/mediawiki-config@master] Enable experimental Parsoid fragment support on labs and test wikis

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

Mentioned in SAL (#wikimedia-operations) [2024-11-19T21:39:39Z] <urbanecm@deploy2002> Started scap sync-world: Backport for [[gerrit:1092341|Enable experimental Parsoid fragment support on labs and test wikis (T374661)]], [[gerrit:1092850|Revert "editcheck: Remove try/catch around transaction squashing" (T333710 T380234)]], [[gerrit:1092851|Revert "editcheck: Remove try/catch around transaction squashing" (T333710 T380234)]]

Mentioned in SAL (#wikimedia-operations) [2024-11-19T21:45:09Z] <urbanecm@deploy2002> cscott, kemayo, urbanecm: Backport for [[gerrit:1092341|Enable experimental Parsoid fragment support on labs and test wikis (T374661)]], [[gerrit:1092850|Revert "editcheck: Remove try/catch around transaction squashing" (T333710 T380234)]], [[gerrit:1092851|Revert "editcheck: Remove try/catch around transaction squashing" (T333710 T380234)]] synced to the testservers (https://wikitech.wikimedia.or

Mentioned in SAL (#wikimedia-operations) [2024-11-19T22:00:19Z] <urbanecm@deploy2002> Finished scap sync-world: Backport for [[gerrit:1092341|Enable experimental Parsoid fragment support on labs and test wikis (T374661)]], [[gerrit:1092850|Revert "editcheck: Remove try/catch around transaction squashing" (T333710 T380234)]], [[gerrit:1092851|Revert "editcheck: Remove try/catch around transaction squashing" (T333710 T380234)]] (duration: 20m 39s)

Change #1093398 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/services/parsoid@master] Turn on Parsoid Fragment Support for RT testing

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

Change #1093399 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[operations/mediawiki-config@master] Turn on Parsoid fragment support everywhere

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

Change #1093398 merged by jenkins-bot:

[mediawiki/services/parsoid@master] Turn on Parsoid Fragment Support for RT testing

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