Page MenuHomePhabricator

mw-empty-elt shouldn't render as visible li
Closed, ResolvedPublic1 Story Points

Description

Problem

Empty list elements are displayed at mobile pages (https://de.m.wikipedia.org/wiki/Benutzer:Aschroet/T129375). This is not done for the non-mobile page (https://de.wikipedia.org/wiki/Benutzer:Aschroet/T129375).

Analysis

The rendered HTMLs of and empty * in the wikitext are:

For mobile:

<ul>
<li>Item 1</li>
<li class="mw-empty-elt">
<li>Item 2</li>
</ul>

For non-mobile:

<ul>
<li>Item 1</li>
<li class="mw-empty-elt"></li>
<li>Item 2</li>
</ul>

Despite the fact that the mobile version does not close the empty <li> which is valid HTML it seems that for the mobile version there exists no display:none CSS property for the class mw-empty-elt as it is the case for the non-mobile page.

PS: Maybe mw-empty-li also needs to be considerd when working on this issue.

Event Timeline

Florian created this task.Mar 9 2016, 5:47 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 9 2016, 5:47 PM
Florian updated the task description. (Show Details)Mar 9 2016, 5:48 PM
Thoken added a subscriber: Thoken.Mar 9 2016, 10:55 PM
MBinder_WMF set the point value for this task to 1.May 18 2016, 7:48 PM
This comment was removed by Aschroet.
Aschroet updated the task description. (Show Details)Aug 17 2016, 6:47 AM

@Dbrant, @MBinder_WMF: i updated the task description after isolating the original problem. Now it is much simpler and the solution seems to be pretty clear. Maybe you could quickly browse over it.

Aschroet renamed this task from mw-empty-li shouldn't render as visible li to mw-empty-elt shouldn't render as visible li.Aug 17 2016, 6:57 AM
Aschroet updated the task description. (Show Details)
bearND added a subscriber: bearND.Aug 17 2016, 7:06 AM

@Florian (and maybe @Aschroet) It's not clear to me why this is assigned to the Android app project. Since this is CSS it looks more like a general mobile web issue to me. Maybe something like reading-web-backlog would be more appropriate?

@bearND, i think that Florian just saw this issue in his Android app when he created the ticket. And since he and me are not familiar with the software and the projects here we just did not know know what project is the one the issue needs to go. So feel free to change this.

@bearND in the first version I spotted the problem in the mobile app (android), only, without checking the mobile web interface. Now, that I know, that the mobile web interface is affected, too, I'll change the projects :P However, this is still a problem of both products (because both don't apply the css code, which normally handles the empty li elements.

@Florian Thanks for adding the web project. Once this is fixed in MFE CSS then the apps would update their CSS based on that and should be fixed as well. I've added iOS for completeness sake, but for now it's mostly tracking until MFE has updated its CSS.

@Florian, do you know why we keep those list items at all? Why not remove them when saving a revision?

Jhernandez triaged this task as Low priority.Aug 17 2016, 3:14 PM
Jhernandez moved this task from To Triage to Needs Analysis on the Readers-Web-Backlog board.
Jhernandez added a subscriber: Jhernandez.

We need to check where mw-empty-elts styles are coming from to determine how to include them in the mobile site if necessary.

@Florian, do you know why we keep those list items at all? Why not remove them when saving a revision?

Uhm, because it's the result of the source :P No, I'm pretty sure, this is intentional, as there could be valid cases, where a user want to add empty list items. So, the question would be, how the software would try to indicate, if the user _really_ wants to add an empty list item or not. So, this doesn't look like a case (at least for me), where the software should try to rule the user :-) If there's an empty list item, the software should output an empty list item. There was also T49673: Empty list items goes missing in the PHP parser when tidy is enabled, a bug with empty list items, when tidy is enabled. Tim Starling commited a change (eb40eb0f1834bd480f15cc9e14dd9c31bfe42e25), which perfectly explains the rationale :)

We need to check where mw-empty-elts styles are coming from to determine how to include them in the mobile site if necessary.

See: https://github.com/wikimedia/mediawiki/blob/0687f250d6fb4196a7aa60af4fe11c7d21f3c455/resources/src/mediawiki.skinning/content.css#L260 :) The RL module would be mediawiki.skinning.content. As they're marked as "according to legacy Tidy rules" in the comment, I'm even not sure, if we really _should_ add it in a new software product. Thinking about the case, that MobileFrontend isn't new, we should probably have the _same_ content styling as desktop, at least as much as possible.

I would've uploaded a change already, but there're 2 points where I'm a bit unsure:

  • I would feel the best, if we could use the desktop RL module, as we would avoid adding more and more things we need to maintenance twice (first desktop, 2nd mobile).
  • However, adding mediawiki.skinning.content to mobile web is also something I'm not very happy with, as we (iirc) doesn't do that for other common styles, and for good reasons.

So, I'm happy for input :P

Thanks @Florian, awesome comment.

I'm not sure how to proceed either and would like to hear more from others, cc @Jdlrobson

@Florian has captured everything perfectly from my opinion.

Given this is code generated inside mediawiki core the only actions here are:

  1. Include mediawiki.skinning on Minerva skin
  2. OR add a css rule to hide this in Minerva

Like @Florian I'm not happy with either but I think option 2 is the lesser evil.

Adding MobileFrontend since Minerva skin lives there

Florian claimed this task.Aug 24 2016, 9:05 PM

Ok, thanks for the comments! :)

Ok, it took some time for me to get tidy running on my dev environment (switched to hhvm recently :P), so the (simple) patch is coming in the next minutes :D

Change 306580 had a related patch set uploaded (by Florianschmidtwelzow):
Hide empty li elements according to tidy rules (like in mediawiki/core)

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

I'm wondering if the Mobile Content Service should remove the empty list items from the DOM. What's the benefit of keeping them?

Technically it would be more correct to display them, as they're in the code (it could also be a valid user input, that there's a list item). Hiding them was only a migration, until we remove this. Let me cite Tim Starling from the commit mentioned above:

It is desirable in terms of user-friendly syntax to display an empty
list item if the user adds one to the source. However, we suspect that
this change will break the rendering of existing templates. So, preserve
the empty <li> element, but style it with display:none so that there is
no user-visible change. Changes can then be observed with a user script,
then eventually the CSS can be removed so that the desired behaviour will
be user visible.

So, even if the "user script" part doesn't apply to the apps, I think it's highly recommended to _not_ remove the elements from the DOM and just use the same hiding rules as core (and remove them, if core does it). So we don't invest time to do something, which isn't ment to life forever :)

If you do want to remove them, please take the time to understand what they are used for and what the trade offs are. Personally I prefer deviating from core as little as possible to avoid future bug reports.

Change 306580 merged by jenkins-bot:
Hide empty li elements according to tidy rules (like in mediawiki/core)

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

If the apps make use of the ResourceLoader module skins.minerva.content.styles you simply need to update this now.

Jdlrobson reassigned this task from Florian to bearND.Aug 25 2016, 5:45 PM
Jdlrobson closed this task as Resolved.Aug 29 2016, 3:58 PM

On second thoughts please create a new task for the apps part of this work (if any).

In mobile view it seems to work now. For Wikipedia App on Android it is still the same.

The apps will need to update their CSS files and publish a new release before Florian's patch becomes effective for the apps. I've created to subtickets, one for Android and another one for iOS.