Templates that create multiple table cells don't work
Closed, ResolvedPublic

Description

Author: kwwilliams

Description:
VE can't properly display any table using {{singlechart}} on English Wikipedia, which makes any editing of the fields pretty dicey.

Taking http://en.wikipedia.org/wiki/5_O%27Clock_%28T-Pain_song%29 as an example (it's actually one of thousands), http://en.wikipedia.org/wiki/File:Singlechart-Articleview.PNG shows what the table looks like when being displayed. http://en.wikipedia.org/wiki/File:Visual-editor-singlechart.png shows what it looks like after Visual editor has been engaged. Note that South Korea (GAON) rows and Romanian Top 100 rows are generated by manual table markup and the remainder are template calls.

This probably comes about from two factors:

  1. Singlechart generates a "|" or "!" internally to start a table row, based on whether it has been called with the rowheader parameter.
  2. Singlechart generates another | between the two data items it outputs to separate the fields in the table row.

Version: unspecified
Severity: normal
See Also:
https://bugzilla.wikimedia.org/show_bug.cgi?id=50366
https://bugzilla.wikimedia.org/show_bug.cgi?id=44498
https://bugzilla.wikimedia.org/show_bug.cgi?id=50951
https://bugzilla.wikimedia.org/show_bug.cgi?id=52296

bzimport added a project: Parsoid.Via ConduitNov 22 2014, 2:02 AM
bzimport set Reference to bz50589.
bzimport created this task.Via LegacyJul 2 2013, 3:48 PM
Jdforrester-WMF added a comment.Via ConduitJul 2 2013, 4:04 PM

This is the same as bug 50366 - merging with that one.

  • This bug has been marked as a duplicate of bug 50366 ***
GWicke added a comment.Via ConduitJul 2 2013, 4:21 PM

The problem here is 2), the extra | generated by Singlechart. The PHP parser flattens everything to a string, so it will eventually see '\n||' with one of those pipes generated by 2) and the other from 1). Parsoid on the other hand needs to keep track of where things came from, so sees those pipes as two td tags.

This can be fixed by avoiding 2).

bzimport added a comment.Via ConduitJul 2 2013, 4:26 PM

kwwilliams wrote:

"This can be fixed by avoiding 2"? How exactly is the template supposed to generate a row header and two table cells while "avoiding 2)"?

Jdforrester-WMF added a comment.Via ConduitJul 2 2013, 4:27 PM

My apologies for assuming that these two issues were related. Re-titling.

GWicke added a comment.Via ConduitJul 2 2013, 6:06 PM

An example from http://en.wikipedia.org/wiki/5_O%27Clock_%28T-Pain_song%29:

{{singlechartAustralia29artist=T-Pain feat. Wiz Khalifa & Lily Allensong=5 O'Clock}}

The issue here is that there is a pipe in the page source while another pipe is emitted by Singlechart. If the pipe in the page source is removed this will work in both the PHP parser and .

GWicke added a comment.Via ConduitJul 2 2013, 6:06 PM

and Parsoid. ;)

bzimport added a comment.Via ConduitJul 2 2013, 7:00 PM

kwwilliams wrote:

Hadn't noticed the leading | character. That is wrong, albeit harmless in most cases (if someone set rowheader=true, it would be a visible error). I removed all of those in a bot run before, and it looks like it's time to do it again.

Removing the pipe doesn't quite work, though: take a peek at http://en.wikipedia.org/wiki/2012_%28It_Ain%27t_the_End%29?veaction=edit vs. http://en.wikipedia.org/wiki/2012_%28It_Ain%27t_the_End%29 and notice how the manual entries and the templated entries align in the normal view but scramble in the edit view.

You really need to be parsing *after* template expansion and tracking all the text that the templates generate. That template should display as the chart and reference in the first column followed by a position in the second column, because that's what the wikitext the template generates says to do. Once the editor tries to edit any of those fields, the editor should be taken to the template editor. Displaying the output as a doublespaced item without any table boundaries that doesn't align with other entries is not the right answer to this problem.

bzimport added a comment.Via ConduitJul 2 2013, 7:51 PM

kwwilliams wrote:

As a help: there are only a few dozen templates that do things like this. I would have no problem at all adding something in the TemplateData that gave VE a clue as to what was going on, like

"table":

{
  "cells": 2,
  "rowheader": "true",
}
GWicke added a comment.Via ConduitJul 2 2013, 8:33 PM

You really need to be parsing *after* template expansion and tracking all the
text that the templates generate.

We are tokenizing templates and content separately so that we can cut template vs. non-template content on a token level. Attribute keys and values can also be templated, but we don't support templates producing several attributes and parts of a token.

This and the need to infer resulting DOM structures affected by template-produced tokens already adds a lot of complexity, but at least the results can somewhat sanely be represented in DOM. Having 1/3 of a DOM element's start tag be generated by page source and 2/3 templated on some arbitrary boundary would be a nightmare to represent in DOM and even worse in a UI.

You are right though that we *could* in theory represent something like |{{singlechart|..}} as a special case of a multi-part templated construct. This would run counter to our desire to move towards self-contained templates though that are easier to edit both in wikitext and the VE. Removing the pipes in the page and returning the full table row from singlechart would be better in that regard.

Removing the pipe doesn't quite work, though: take a peek at
http://en.wikipedia.org/wiki/2012_%28It_Ain%27t_the_End%29?veaction=edit vs.
http://en.wikipedia.org/wiki/2012_%28It_Ain%27t_the_End%29 and notice how the
manual entries and the templated entries align in the normal view but
scramble
in the edit view.

Indeed, it seems that we don't recognize single-line rows that don't start with ||. As a workaround, it should be possible to emit '||' from the template instead. I'll file a bug for our table row tokenization bug.

GWicke added a comment.Via ConduitJul 2 2013, 8:39 PM

Oh wow, on some more investigation it looks like we are actually rendering the table just fine:

http://parsoid.wmflabs.org/en/2012_%28It_Ain%27t_the_End%29

So the odd rendering seems to be a VE issue.

Aklapper added a comment.Via ConduitJul 4 2013, 10:35 AM

[Parsoid component reorg by merging JS/General and General. See bug 50685 for more information. Filter bugmail on this comment. parsoidreorg20130704]

MZMcBride added a comment.Via ConduitJul 7 2013, 10:26 PM

(In reply to comment #10)

Oh wow, on some more investigation it looks like we are actually rendering
the table just fine:

http://parsoid.wmflabs.org/en/2012_%28It_Ain%27t_the_End%29

So the odd rendering seems to be a VE issue.

Should this bug be recategorized then?

Kelson added a comment.Via ConduitJul 12 2013, 8:01 PM

Not sure if I should open a new or make a comment here... But is this wrong rendering (too much td nodes) the result of this bug?
http://parsoid.wmflabs.org/sr/%D0%93%D0%BB%D0%B0%D0%B2%D0%BD%D0%B0_%D1%81%D1%82%D1%80%D0%B0%D0%BD%D0%B0

cscott added a comment.Via ConduitJul 30 2013, 2:48 PM

This seems to be related to bug 44498.

GWicke added a comment.Via ConduitAug 20 2013, 8:21 PM

Fixed by https://gerrit.wikimedia.org/r/#/c/79430/, which is now deployed. Chart tables such as the one on http://en.wikipedia.org/wiki/2012_%28It_Ain%27t_the_End%29?veaction=edit now render and round-trip as expected.

The unrelated table rendering issue in http://en.wikipedia.org/wiki/2012_%28It_Ain%27t_the_End%29?veaction=edit is VisualEditor bug 50607.

Add Comment