r80430 MediaWiki - Code Review archive

elements and other contructs in a {| wikitable |} are now no longer a direct child of a table cell.

Adding <p>-paragraphs to each and every table cell seems a bad idea to me and is probably not semantically correct either. It for one breaks javascript that works with direct child relations in templates.

However treating stuff in a table cell as "new line" ish ( headings, lists, tables ) sounds great and solves the need for an extra new line. But right now it breaks stuff.

The current behaviour is that ‎<p> tags are added to all paragraphs in the cell which begin on a new line, as usual, with the exception that the first cell is not a new line (unless it comes from a template, in which case it is). So

{|
| Foo

Bar
|
Foo

Bar
| {{template-containing-Foo}}

Bar
|}

Becomes (with some tweaks to the whitespace)

<table><tr>

<td>    Foo     <p>Bar</p> </td>
<td> <p>Foo</p> <p>Bar</p> </td>
<td> <p>Foo</p> <p>Bar</p> </td>

</tr></table>

Which is horribly inconsistent, and results (erratically) in the behaviour you say is problematic, but not consistently so. Having parts of the text on different context levels is almost certainly less semantically correct than applying ‎<p> universally; the fact that single-paragraph cells don't get a ‎<p> tag at all is a side-effect, and not necessarily a desirable one: why should adding more content to a cell alter the behaviour of the existing content? And why should the two cells in the table:

{|
| Text
* And 
* List
| 
Text
* And
* List
|}

Have different internal structures?

You are correct in that the current behaviour (before your commit) doesn't always make sense, but that was the output that has been accepted and lots of stuff depends on it.

For example the sortable script appends linked images to the table cells as sortable-buttons, which, before your commit, where on the same line. Now there's suddenly a "paragraph" in every table cell (including table headings) which makes no sense and thus anything before or after (since P is a block level element) is now on a new line. (try it, make a class="wikitable sortable" table)

Perhaps you can make it so that it looks to see if there's stuff like * # {| coming and treat them as such, and otherwise stick to inline like it used to.

Either way, it's more a bug that the the current parser sometimes adds a paragraph, it's not a bug that it sometimes doesn't.

(

also bring along CSS

p {
   line-height: 1.5em;
  margin: 0.4em 0px 0.5em;
} 

making table cells much bigger than they used to be.

sorry for the double post, pressed submit too early.

Testcase

• before / after

But presumably these problems with sortable tables already occur, just not consistently so; whenever someone hasn't started the table header on the same line as the cell marker, or whenever there's a multi-paragraph header. For sure, these cases are not the common ones, but they are perfectly reasonable ones. It would be easy to fix SortableTables by prepending the links and right-floating them, that would also make them work consistently with anything else that might be inserted into a cell. Or you could make them translucent and attached to the upper-right corner of the cell, and become opaque on hover, which is also a common position.

Overall, I'm not denying that anything you've brought up happens; it's the timeless battle between consistency and optimising-for-the-most-common-case. If one piece of subtext is parsed into paragraph tags in a given position, and another substring in the same position is parsed into a list, why in another position should one of the strings be parsed the same, and the other be parsed differently? We could remove paragraph margins inside tables, but why should the same markup appear differently in that context?

A follow-up which was a step in the right direction might be to remove the vertical padding from wikitable cells, since we now know that anything placed inside them will have margins of its own.

SortableTables could be altered to place the linked images at the end of the first paragraph or floating before the paragraph. But that brings other issues of it's own since floating does not force content width. In order words: A table is and should not by default take 100% width. if a cell has a few words in it the cell expand to a width to contain those words on a single line. But if an element floats most browsers don't force the width so it will still come on it's own line above the paragraph.

See below:

{| class="wikitable"
! word <span>[s]</span>
! word <span>[s]</span>
|- 
| cont
| tent
|}

renders:

{| class="wikitable"
! <span style="float:right">[s]</span> <p>
word 
! <span style="float:right">[s]</span> <p>
word 
|- 
| <p>
cont
</p>
| <p>
tent
</p>
|}

renders:

And aside from this the point is not that this particlar sortable-script could be fixed. The point is that there are many gadgets, many extension and user scripts both within the 100s of wikis by the Wikimedia Foundation and outside of it and scripts working with tables and dynamic insertion/hiding of content are not rare.

So in my opinion this commit does not fix anything that is currently broken, nor does it enable things that can't be done now (albeit not always as clean in wikitext)

– rather this commit breaks existing code and causes paragraphs in tables (which are almost never wanted, tables are for tabular data, there is no semantically correct  usecase for tabular data that requires being wrapped in paragraphs)  and also extra code (bandwidth?).

Anyway, these are just my opinions and conclusions based on what I see on TranslateWiki and a local wiki running trunk with some of the bigger articles on Wikipedia.

I think people understand that block level elements start on a new line, meaning if they want to start a heading, table or list in a table they enter a new line first. That's what they currently do:

head

more


content


* list
* here
Heading

not to mention the bots running on Wikipedia that look for certain pieces of content, if from now on blocklevel elements can start inside a table cell without a new line... (such as the following, which renders two list items and a heading on the current trunk)

{|
! * test
* test
|-
| == hi ==
|}

http://translatewiki.net/w/i.php?title=Sandbox&oldid=2643446

.. that means several rules will have to be rewritten.

Consistency in the parser existed:

• Block level elements start on a new line (regardless whether in a table)
• Without a new line characters are treated inline (#, *, = etc.)

if there's any place in the parser the above two points are not the case, I think we have a valid bug.

So in my opinion this commit does not fix anything that is currently broken

That is manifestly not true; try expanding == {{PAGENAME}} == on en:Talk:*-algebra. The 'fix broken things' part of this bug is to undo the hack which inserted newlines at the start of template expansions if the template text contained block elements (bug 12974 contains a whole raft of places where this is unintuitive and in some cases unfixable). The table stuff is just trying to more carefully resolve the issue (bug 529) which prompted the hack to be put in in the first place.

Consistency in the parser existed

That's precisely what did not exist. Previously, "block level elements start on a new line", except when they are produced from a template, in which case they're block level elements whether or not they start on a new line. So neither of your bullets is correct in the context of template expansion, in the old case.

I'd be reasonably happy with just removing the newline hack without replacing it with the table stuff, but that breaks B/C much more substantially, in that wikitext which was formerly recognised would then not be recognised. Requiring third-party scripts to be updated is a much softer B/C break than requiring markup to be changed; we should prioritise maintaining B/C on a vanilla install.

causes paragraphs in tables (which are almost never wanted, tables are for tabular data, there is no semantically correct usecase for tabular data that requires being wrapped in paragraphs)

‎<td> elements contain flow content; paragraphs are flow content. There is absolutely no semantic reason why it may not be appropriate to put paragraphs inside tables. While you're right that tables shouldn't be used purely for layout, that in no way precludes table cells legitimately containing content which is more than monosyllabic. It's equally justifiable to have or not to have paragraphs inside table cells; ‎<p> tags are 'transparent' in the sense that they are flow content, and also contain flow content. What is definitely less clear and consistent is to sometimes have content inside tags and sometimes not, and especially to mix levels within the same table cell.

and also extra code (bandwidth?)

Oh please. You can save more bytes than this adds by spriting the SortableTables arrows, if you really care that much.

Even for strictly tabular data, I can easily image use cases where paragraphs breaks in table cells would be desired. (For example, the content might be a multiparagraph quotation.) Given that, it does seem inconsistent for table cell content to be wrapped in <p> tags only sometimes.

I did some quick tests, and it looks like adding

.wikitable p, .wikitable ul, .wikitable ol, .wikitable dl  { margin-bottom: 0.2em; margin-top: 0.2em; }

and replacing the current table cell padding rule with

.wikitable th, .wikitable td { padding: 0 0.2em; }

ought to solve the CSS issues (modulo conflicts with site styles and/or custom skins; it might be possible to minimize those by making the selector for the padding rule as specific as possible, e.g. table.wikitable tr th, table.wikitable tr td). This does make paragraph breaks narrower inside .wikitable tables than outside them, but IMO that actually looks better.

(Alternatively, it might perhaps be possible to hack the parser to "unwrap" the first child element of a table cell if it's a <p>, and then just set .wikitable p { margin-bottom: 0; }. This would avoid changing the HTML output for the most common use case, and should thus minimize breakage.)

Fixing the sortable and collapsible table JS to handle <p> tags in table cells shouldn't be hard, and would be beneficial in any case (since they can occur already).

@Happy-melon: The fact that {{PAGENAME}} puts characters that affect wikitext on to page page is a seperate bug. It should encode all characters (all wikitext characters are either forbidden in titles or they are escaped, such as quotes: http://commons.wikimedia.org/wiki/User:Krinkle/%27%27%27test%27%27helo%27%27lo ). In this case a star/asterisk should be encoded as ⋆, just like simple single quotes are escaped through magic words like '.

Regarding "Consistency in the parser existed:". I know those two points are not true at the moment, and that's unfortunate.

If it's absolutely sure the parser will always but block level elements inside a table cell (be it the element created through wikitext (headings, lists, tables etc.)) then we could use CSS to remove the padding tables have from nature, but how about the 100s of templates that use style="" to add a certain amount padding to tables for messages, infoboxes, talkpage notices etc. those would all get an additional paragraph and would require not only altering site css, also any and all occurenses of inline styling.

Aside from having to fix all those wikitext, there's still the point that users are taught and bots are programmed to look only for blocklevel elements on new lines (ie. when I write == foobar == in this sentence or some *stars* and #hashes# they are not recognized), inside a table cell right after the pipe, that is inline context and should be treated as such, not broken up more.

I agree with you that fixing the parser where it forces block level context where it's inline (ie. templates inside tables) is even more problematic in case of breakage. So, the best outcome imho is to do neither: keep it the way it was: A sucky annoying inconsistent parser that we all know and love.

{{PAGENAME}} is not {{PAGENAMEE}}. It shouldn't be doing any escaping.

But it does, because the parser is horrible and tries to parse everything in the page name, namely * and ''.

No it does not parse single quotes. It escapes them.

PAGENAMEE is for url encoding. PAGENAME is to display the page name and should not affect wikitext therefor it escapes single quotes, see also here.

Hmm, OK, I was wrong. It does call wf wfEscapeWikiText(). So the fix for bug 26781 would presumably be to add "*#;:" to the list of characters it escapes.

Yes please ;-)

And what do you want {{#ifeq:{{PAGENAME}}|*-algebra}} to return?

r80511 and r80512 handle the en:Talk:*-algebra {{PAGENAME}}

In this case a star/asterisk should be encoded as ⋆, just like simple single quotes are escaped through magic words like '.

That should've been:

In this case a star/asterisk should be encoded as ⋆, just like simple single quotes are escaped through magic words like '.

A star and an asterisk are not the same; en:⋆ and en:* are separate articles, and rightly so.

Anyway, whatever we do with table parsing and wfEscapeWikiText(), the parts of this rev that don't affect tables should be kept if at all possible. Parsing foo{{#if:x|*bar}} should not produce a bulleted list.

This fixes quirks as paragraphs appearing sometimes inside tables:

<td> Fooo
<p>Bar
</p>
</td>

If we are going to break table compatibility, it would be time for doing a more complete fixup.

I just attempted to fix a few gadgets and play with CSS rules regarding paragraphs in tables, but that didn't go very far.

• Tables created through ‎<table> and ‎<tr> in wikitext etc. don't get the added paragraphs
• Anything not coming from wikitext don't get the added paragraphs (forms with tables, extensions using tables (ie. Special:Translate), TablePager etc.) so assuming there is always a paragraph inside a table cell is wrong and even worse (ie. if we would remove the padding from table cells).

I still have to see why we would have to change the parser so that stuff in a table cell is "as if it were on a new line". Why would we want to accept:

• Lists starting on the same line as the pipe
• Headings and tables starting inline (in any other inline situation we dont do this either, why would we in a table ? )

{| 
! Head
! Ing
|-
| == heading works in trunk ==
| * list works in trunk
|}

Lorem ipsum == heading does not work ==
Dolor silet  * not a list.

Try explaining that to someone:

• Block level elements need to start on a new line, except in a table...; Why ?; Because when a template is put inside a table it was decided it should be treated as if on a new line, so we decided everything in a table should be forced to start as-if it were a new line;.

I think we should simply add the fact that templates in tables behave special to the list of inconsistencies of the current parser. When a new parser is introduced, then we can simply fix everything at once without having to look back (ie. the concept I read on wikitech-l where there would be a period of time with two parsers) - but either way a change breaking current wikitext is wrong because there could be wikis out there with 100s of pages depending on this.

The answer to all your questions is "because that's how it was before". I'm only consciously introducing two changes:

1. Brace constructs should not universally introduce a newline. Foo {{* Bar}} should not produce a bulleted list.
2. If a particular place is a block context, it should always be a block context; that means that anywhere where a * or # should produce a list, a plain string should produce ‎<p> tags.

As I said, I'm not at all averse to leaving out the first-table-cell stuff and WONTFIXING but 529, but I do not accept that we should all bow in worship before a mystical new parser which will also heal our children and do the washing up. A new parser is just as far away as it has ever been; the closest current contender covers ~80% of the markup in a compiled language and performs broadly the same as the existing one. It will be years before we can even consider a migration process, and in that time bug 12974 will continue to be one of the most-duped on bugzilla.

This was reverted in r81012, although without prejudice to its reintroduction.