Page MenuHomePhabricator

<p>-wrapping lost inside <td>
Open, LowPublic

Description

See comments on https://gerrit.wikimedia.org/r/195902

<td><p>foo<p></td> serializes to |foo, which means it doesn't roundtrip html -> wt -> html because the <p>-wrapping disappears:

$ echo '<table><tr><td><p>foo</p></td></tr></table>' | tests/parse.js --html2html --normalize
<table>
<tbody>
<tr>
<td>foo</td>
</tr>
</tbody>
</table>

But in fact we can recover the <p>-wrapping by adding a newline after the |:

$ ( echo '{|' ; echo '|' ; echo 'foo' ; echo '|}' ) | tests/parse.js --normalize
<table>
<tbody>
<tr>
<td>
<p>foo</p>
</td>
</tr>
</tbody>
</table>

So there is a way to say whether <p>-wrapping should happen in wikitext or not -- we need to emit a leading \n if we want <p>-wrapping. This is a WTS bug. The fix for T88318 fixed a different bug ( <td><meta/>-</td> for instance ), but we should fix <p>-wrapping inside <td> properly by re-adding the missing newline after the |.

Event Timeline

cscott raised the priority of this task from to Needs Triage.
cscott updated the task description. (Show Details)
cscott added a project: Parsoid.
cscott added a subscriber: cscott.

So, right now, we are serializing <td><p>foo</p></td> as if the p-tag weren't there. However, if we change this behavior to get html2html roundtripping, this might require VE to not add the p-tag where they didn't mean to affect rendering on the parsed output. This is because <td><p>foo</p></td> renders differently in the browser than <td>foo</td>. So, I have a suspicion we handled the <p> tag in the <td> this way for some VE-specific reason.

@Catrope, @Esanders, @Jdforrester-WMF ... thoughts?

Blocked on VE:

(04:24:28 PM) cscott-free: [...]which just pushes over to the VE side the question of whether the <p> wrapper should appear.
(04:24:15 PM) RoanKattouw: Well VE will introduce <p> wrappers relatively aggressively
(04:24:27 PM) RoanKattouw: If the original HTML is <td></td> then any typed text will be p-wrapped
(04:24:37 PM) RoanKattouw: The only case in which we don't p-wrap is when there was existing non-p-wrapped text
(04:24:59 PM) subbu: cscott, so .. i was right .. we have the current wts behavior because of that.
(04:25:19 PM) cscott-free: well, we're just covering for VE then.
(04:25:32 PM) cscott-free: the "correct" serialization of <td><p>x</p></td> is |\nx
(04:25:58 PM) subbu: RoanKattouw, the reason this came up .. is because <td><p>foo</p></td> doesn't currently HTML -> wt -> HTMl roundtrip
(04:25:59 PM) cscott-free: otherwise we can't round-trip it.
(04:26:15 PM) RoanKattouw: Right
(04:26:15 PM) subbu: <td><p>foo</p></td> -> |foo -> <td>foo</td>
(04:26:36 PM) RoanKattouw: We could also put in logic that would bias towards unwrapped paragraphs a bit more strongly
(04:26:41 PM) subbu: cscott, i would say this is low priority.
(04:26:47 PM) RoanKattouw: But that would be complicated
(04:26:54 PM) subbu: since rendering is not affected either way.
(04:26:56 PM) cscott-free: i'd agree, and i think VE should make their change before Parsoid fixes the 'bug'
(04:27:29 PM) cscott-free: otherwise a lot of users might be unhappy that their "a | b | c" tables render in wikitext as |\na\n|\nb\n|\nc
(04:27:34 PM) RoanKattouw: Yeah exactly
(04:27:37 PM) cscott-free: I'm sure we'd get complaints about that
(04:27:48 PM) subbu: yes, we shoudln't change anything in parsoid land now.

ssastry triaged this task as Medium priority.Mar 24 2015, 4:48 PM
ssastry set Security to None.
ssastry lowered the priority of this task from Medium to Low.Jun 18 2020, 6:27 PM