Page MenuHomePhabricator

Fix for incorrect column count CSS due to Parsoid generated HTML being different from PHP Parser output
Closed, ResolvedPublic

Description

As reported by @DarTar, on mobile web the "enwiki > 2016 Dyn cyberattack > Affected services" section looks like this:

But on the iOS app it looks like this:

It looks like mobile web has the following additional CSS which controls the column count:

.column-count, .references-column-count{
    -moz-column-count:1 !important;
    -webkit-column-count:1 !important;
    column-count:1 !important
}

I have a WIP which adds this CSS to our style overrides, but I want to check with Android first to see if they have the same issue. It may be we need to add this fix upstream so we both get the fix...

Details

Related Gerrit Patches:
mediawiki/extensions/MobileApp : masterUpdate: allows refs to span multiple columns
mediawiki/extensions/MobileFrontend : masterFix: set optimal width for column-count

Event Timeline

Mhurd created this task.Oct 28 2016, 1:50 AM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptOct 28 2016, 1:50 AM

@Niedzielski Ah! Interesting! Thanks for checking!

@dr0ptp4kt and I poked around for a couple minutes but we weren't able to find where the css which mobile web uses to control columns (below) comes from...

.column-count, .references-column-count{
    -moz-column-count:1 !important;
    -webkit-column-count:1 !important;
    column-count:1 !important
}

I can look more tomorrow, or if you have ideas let me know :) We may just be able to pull the source into both app's css... or I suppose just manually add it (like @Dbrant did for the fraction fix the other day).

Hey @Jdlrobson would you know where the mobile web column count override CSS (above) lives?

master x ~/git/core/extensions/MobileFrontend $ ag references-column-count
resources/skins.minerva.content.styles/hacks.less
82:.references-column-count {

This rule does not apply to Parsoid HTML as the HTML generated is different. The issue can be seen in the Weekipedia PWA as well:

https://trending.wmflabs.org/en.wikipedia/2016%20Dyn%20cyberattack#Affected_services

I'd suggest the fix should go in MobileFrontend given skins.minerva.content.styles is already included by the apps. Please schedule this work with @ovasileva

Jdlrobson renamed this task from Fix for incorrect column count CSS to Fix for incorrect column count CSS due to Parsoid generated HTML being different from PHP Parser output.Oct 28 2016, 8:36 AM
Jdlrobson added a project: Readers-Web-Backlog.

Let's please add this to MobileApp like we did with .visualhide.

Mhurd added a comment.Oct 28 2016, 6:10 PM

@Dbrant I think Jon's fix will mean the upstream CSS will be updated to deliver the needed CSS.

Mhurd removed Mhurd as the assignee of this task.Oct 28 2016, 7:30 PM

whoops! yes, totally missed that.

+1 to fixing this upstream

Mhurd added a comment.Oct 28 2016, 8:37 PM

@ovasileva

@Jdlrobson's suggestion that this fix go in MobileFrontend sounds like the way to go.

It's not super high-priority given that the issue has apparently existed for some time, but it would be very nice if it could be scheduled soon given that it sounds like a very quick fix :)

Maybe I myself can work on this? I'm trying to identify a few tasks in Webland for my 10% time and this has some overlap with Androidland. Let me know if there are any objections. Thanks!

@Niedzielski sounds like a good fit :)

JMinor triaged this task as Medium priority.Oct 28 2016, 10:33 PM

@Niedzielski that would be great and yes I think it would be a perfect fit. I'll watch out for a patch :)
It would also help T145219.

Change 322363 had a related patch set uploaded (by Niedzielski):
WIP: Fix: set optimal width for column-count

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

Change 322364 had a related patch set uploaded (by Niedzielski):
WIP: Update: allows refs to span multiple columns

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

With the above two patches applied:

Change 322363 merged by jenkins-bot:
Fix: set optimal width for column-count

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

Change 322364 merged by jenkins-bot:
Update: allows refs to span multiple columns

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

Mhurd added a comment.Jan 5 2017, 7:58 PM

Fixed in iOS app now as well:

( fix will be live in next release )

@JMinor is this resolved?

@Mhurd @Niedzielski I've removed patch for review as I see nothing to review here... but I think it's fixed?

All good on Android:

TheDJ added a subscriber: TheDJ.Apr 20 2017, 10:00 AM

As this appears to be deployed, i have removed a local en.wp hack that noted it should be removed after this is done:
https://en.wikipedia.org/w/index.php?title=MediaWiki:Mobile.css&diff=776329318&oldid=770119237

I hope it's still ok now :)

@TheDJ, the apps use Mobile and Common.css from MediaWiki.org and, unfortunately, an ancient copy of English Wikipedia's Mobile and Common.css. We're trying to fix this but your change shouldn't be seen until we do.

Not a web issue, but let us know if we can help!

Niedzielski closed this task as Resolved.Jun 29 2017, 8:18 PM

The comments indicate this issue was resolved.