Page MenuHomePhabricator

closeElement causes UI breakage when $wgWellFormedXml = false;, does not conform to spec
Closed, ResolvedPublic

Description

Screenshot of where the border is broken

After almost 2 hours of debugging I finally figured out what caused the tabs in Monobook to sometimes cause the border to be misaligned (see attachment).

For some reason the <a> inside the <li> was smaller than the <li>, which is odd since the size of the <li> is solely determined by its content.

More over, it displays right on mediawiki.org and beta.wmflabs.org, but broken on translatewiki.net and my local wiki.

The DOM (when inspected) looks completely the same, and no CSS styles applied differently.

Then I looked at the DOM and notice that on the "good" wikis, the output is:

<h2>Navigation menu</h2>
<div id="p-cactions" class="portlet" role="navigation">
	<h3>Views</h3>
	<div class="pBody">
		<ul>
			<li id="ca-nstab-main" class="selected"><a href="/wiki/Sandbox" ..>Page</a></li>
			<li id="ca-talk"><a href="/wiki/Talk:Sandbox" ..>Discussion</a></li>

And on the "bad" wikis, the output is:

<h2>Navigation menu</h2>
<div id="p-cactions" class="portlet" role="navigation">
	<h3>Views</h3>
	<div class="pBody">
		<ul>
			<li id=ca-nstab-main class=selected><a href="/wiki/Main_Page" ..>Page</a>
			<li id=ca-talk class=new><a href="/w/index.php?title=Talk:Main_Page&amp;action=edit&amp;redlink=1" ..>Discussion</a>

After ruling out Tidy, I narrowed it down to $wgWellFormedXml.

Or more precisely by Html::closeElement (called from SkinTemplate::makeListItem, called from SkinMonobook::cactions()), where it omits the closing tag of an <li>.

As a result, the browser only closes the <li> when it encounters the next opening tag, and as such includes a text node between </a> and (the implied) </li>.

This extra space causes the list item to be wider than the <a> it holds.


Attached:

Screen_Shot_2013-07-29_at_8.17.11_AM.png (658×1 px, 238 KB)

Details

Reference
bz52210

Event Timeline

bzimport raised the priority of this task from to Medium.Nov 22 2014, 1:59 AM
bzimport set Reference to bz52210.

Created attachment 13000
Screenshot of where border isn't broken.

Attached:

Screen_Shot_2013-07-29_at_8.18.27_AM.png (644×1 px, 206 KB)

Reproduced the bug in Chrome 28 (stable), Firefox 22 (stable) and Chrome 30 (canary), Mac.

It also does not conform to the Living Standard (the same source it cites) (http://www.whatwg.org/html/syntax.html#optional-tags). There are all sorts of rules about when you can omit the tag. E.g.

"A head element's end tag may be omitted if the head element is not immediately followed by a space character or a comment."

"An li element's end tag may be omitted if the li element is immediately followed by another li element or if there is no more content in the parent element."

Since closeElement does not take into account context, it does not follow these rules. In some cases, the DOM might be invalid anyway if you don't follow these rules (haven't looked into it), but in other's, it's clearly valid. For example, it's perfectly fine to have a comment immediately after the head element; but closeElement can't know that, so it violates the spec.

I'm going to put up a patch to just remove this whole thing.

Change 152778 had a related patch set uploaded by Mattflaschen:
Html::closeElement: Don't omit closing tags.

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

Change 152778 merged by jenkins-bot:
Html::closeElement: Don't omit closing tags.

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

Krinkle set Security to None.