Last modified: 2013-08-20 20:21:37 UTC

Wikimedia Bugzilla is closed!

Wikimedia migrated from Bugzilla to Phabricator. Bug reports are handled in Wikimedia Phabricator.
This static website is read-only and for historical purposes. It is not possible to log in and except for displaying bug reports and their history, links might be broken. See T52589, the corresponding Phabricator task for complete and up-to-date bug report information.
Bug 50589 - Templates that create multiple table cells don't work
Templates that create multiple table cells don't work
Status: RESOLVED FIXED
Product: Parsoid
Classification: Unclassified
General (Other open bugs)
unspecified
All All
: Normal normal
: ---
Assigned To: Gabriel Wicke
:
Depends on: 50603 50607
Blocks:
  Show dependency treegraph
 
Reported: 2013-07-02 15:48 UTC by kwwilliams
Modified: 2013-08-20 20:21 UTC (History)
7 users (show)

See Also:
Web browser: ---
Mobile Platform: ---
Assignee Huggle Beta Tester: ---


Attachments

Description kwwilliams 2013-07-02 15:48:10 UTC
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.
Comment 1 James Forrester 2013-07-02 16:04:49 UTC
This is the same as bug 50366 - merging with that one.

*** This bug has been marked as a duplicate of bug 50366 ***
Comment 2 Gabriel Wicke 2013-07-02 16:21:47 UTC
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).
Comment 3 kwwilliams 2013-07-02 16:26:04 UTC
"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)"?
Comment 4 James Forrester 2013-07-02 16:27:43 UTC
My apologies for assuming that these two issues were related. Re-titling.
Comment 5 Gabriel Wicke 2013-07-02 18:06:34 UTC
An example from  http://en.wikipedia.org/wiki/5_O%27Clock_%28T-Pain_song%29:

|{{singlechart|Australia|29|artist=T-Pain feat. Wiz Khalifa & Lily Allen|song=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 .
Comment 6 Gabriel Wicke 2013-07-02 18:06:59 UTC
and Parsoid. ;)
Comment 7 kwwilliams 2013-07-02 19:00:43 UTC
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.
Comment 8 kwwilliams 2013-07-02 19:51:34 UTC
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",
  }
Comment 9 Gabriel Wicke 2013-07-02 20:33:13 UTC
> 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.
Comment 10 Gabriel Wicke 2013-07-02 20:39:28 UTC
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.
Comment 11 Andre Klapper 2013-07-04 10:35:13 UTC
[Parsoid component reorg by merging JS/General and General. See bug 50685 for more information. Filter bugmail on this comment. parsoidreorg20130704]
Comment 12 MZMcBride 2013-07-07 22:26:50 UTC
(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?
Comment 13 Kelson [Emmanuel Engelhart] 2013-07-12 20:01:30 UTC
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
Comment 14 C. Scott Ananian 2013-07-30 14:48:03 UTC
This seems to be related to bug 44498.
Comment 15 Gabriel Wicke 2013-08-20 20:21:37 UTC
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.

Note You need to log in before you can comment on or make changes to this bug.


Navigation
Links