Page MenuHomePhabricator

Implement work-arounds for templated table cell rendering issues
Closed, ResolvedPublic

Description

Eventually we'd like to move towards more self-contained templates, but for now we should at least try to render the content as expected, even if that content remains uneditable for now.

Test cases:

{|
|{{echo|{{!}} Foo}}
|{{echo|style="color: red" {{!}} Bar}}
|}

Foo is bug 50589, while Bar is bug 50366.


Version: unspecified
Severity: normal

Details

Reference
bz50603

Event Timeline

bzimport raised the priority of this task from to High.Nov 22 2014, 2:03 AM
bzimport added a project: Parsoid.
bzimport set Reference to bz50603.

Hmm, Bar did not actually work. Fixed test case:

{|

{{echo{{!}} Foo}}
{{nomBar}}
}

With {{nom|bar}} expanding to http://en.wikipedia.org/w/api.php?action=expandtemplates&text={{nom|Bar}}

Some background from https://gerrit.wikimedia.org/r/#/c/76861/:

We are mainly dealing with two cases here (from bug 50603):

  1. |{{echo|{{!}} Foo}}

This produces two tds, which can be merged on the DOM just as in the list item case.

  1. |{{nom|Bar}} with {{nom|Bar}} expanding to something like 'style="foo"|Bar'

In this case attributes and (at least some) cell content are template-generated. In the templates I have seen the attributes don't contain wikitext links or other problematic syntax that would cause them to parse to non-text content. This means that we can directly convert the prefix of a text node into attributes. The (planned) addition of nested DSR on template content will also give us access to the original wikitext of other template content, which makes this sound even in theoretical edge cases.
In general I am trying to keep parsing context for template expansions as minimal as possible. Basically context-free expansions enable parallel expansion and efficient reuse. In this special case we are bending over backwards to accommodate a specific case of poorly nested template use. In the longer term the goal is to move away from this however, so we should keep the impact of such hacks on our code base as minimal as possible. The implementation should be self-contained enough so that we can disable them once less ideal nestings like these are fixed up in our content, potentially using the information we have in Parsoid itself.
Your assumption that our current parser framework is synchronous is not actually true. Template / extension expansions and link handling are highly asynchronous. Ironically it is exactly this asynchrony and independent expansion that makes handling this case hard. With a synchronous text-based preprocessor pass as in the PHP parser re-tokenization would take care of these cases. The price of this (no parallelism or template reuse, additional complexity of template encapsulation etc) would be high though.

Change 79430 had a related patch set uploaded by GWicke:
Bug 50603: Handle |{{echo|{{!}} Foo}}

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

Change 79430 merged by jenkins-bot:
Bug 50603, 50589: Handle |{{echo|{{!}} Foo}}

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

Change 79943 had a related patch set uploaded by GWicke:
WIP: Bug 50603, 44498: Handle |{{nom|Bar}}

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

Removed dependency on bug 50951 as that is an unrelated VE issue.

Change 79943 merged by jenkins-bot:
Bug 50603, 44498: Handle templates straddling cell attributes & content

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

The original test cases are now fixed in master. The second patch will likely be deployed in the next days after round-trip testing is complete.

A somewhat similar issue was found in round-trip testing, and is now tracked in bug 53139.

These work-arounds are now deployed. Closing as fixed.

ssastry updated the task description. (Show Details)Feb 3 2015, 11:54 PM
ssastry set Security to None.