Page MenuHomePhabricator

Move special-case app styling from the MobileApp extension to PCS
Open, NormalPublic

Description

I've been doing some consolidation work in the MobileApp repo while breaking ground on the new Page Content Service CSS endpoint. For this initial iteration the goal is just to get the bundle behind an endpoint so the apps can have their styles updated agilely. Further, we want to serve the same bundle to both apps if at all possible. I think this is achievable. Here are some open questions blocking additional consolidation:

  • Historically we've had different 'pagestyles' and 'preview' modules, with the 'preview' modules effectively just omitting some styles. Does anyone know why we omit certain styles for previews? Could we just use the same CSS bundle in all cases?

Event Timeline

Mholloway triaged this task as High priority.Mar 9 2018, 4:51 PM
Mholloway created this task.
Mholloway updated the task description. (Show Details)Mar 9 2018, 4:55 PM
Mholloway updated the task description. (Show Details)
Mholloway updated the task description. (Show Details)Mar 9 2018, 5:06 PM
Mholloway updated the task description. (Show Details)Mar 12 2018, 2:13 PM
Mholloway added a comment.EditedMar 12 2018, 8:54 PM

Here's a silly question for anyone who was around when early app architecture choices were being made. Why do the apps bundle CSS at all? Links to the stylesheet(s) needed for a page could have instead been exposed through action=mobileview, for example. Was the idea to save on network requests? Or that many of the modules ResourceLoader identified as relevant to a page wouldn't actually be needed in the apps? Maybe both?

Mholloway updated the task description. (Show Details)Mar 12 2018, 9:44 PM
Mholloway added a comment.EditedMar 13 2018, 3:41 AM

First CSS style commit for Android: https://github.com/wikimedia/apps-android-wikipedia/commit/98c3a4db9e3d87d51cfc66d7776b3d9380c16897
No discussion of CSS style architecture for the apps appears to be documented anywhere. The MobileApp extension was created and styles moved there in January 2014.

Separate preview styles added here: https://github.com/wikimedia/mediawiki-extensions-MobileApp/commit/91e66fe7a1d2e33752096c8ea99572c9a08810cb
Looks like the point was to hide the edit button. After some digging it looks like that was the only real reason for the distinction.

Change 419102 had a related patch set uploaded (by Mholloway; owner: Mholloway):
[apps/android/wikipedia@master] Stop bundling frwiki-specific CSS snippets

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

Change 417936 had a related patch set uploaded (by Mholloway; owner: Mholloway):
[mediawiki/extensions/MobileApp@master] Consolidate common app styles

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

Change 419103 had a related patch set uploaded (by Mholloway; owner: Mholloway):
[mediawiki/extensions/MobileApp@master] Remove frwiki.less and the mobile.app.pagestyles.android module

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

Change 419104 had a related patch set uploaded (by Mholloway; owner: Mholloway):
[mediawiki/extensions/MobileApp@master] Eliminate mobile.app.pagestyles

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

Change 419105 had a related patch set uploaded (by Mholloway; owner: Mholloway):
[apps/android/wikipedia@master] Stop bundling eliminated mobile.app.pagestyles bundle and update CSS

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

Change 419167 had a related patch set uploaded (by Mholloway; owner: Mholloway):
[mediawiki/services/mobileapps@master] Remove /css/pageview route to reflect ongoing app CSS consolidation

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

Mholloway updated the task description. (Show Details)Mar 13 2018, 1:30 PM
Mholloway updated the task description. (Show Details)Mar 13 2018, 3:16 PM
Mholloway updated the task description. (Show Details)Mar 13 2018, 11:22 PM

Change 419423 had a related patch set uploaded (by Mholloway; owner: Mholloway):
[mediawiki/extensions/MobileApp@master] Remove IPA button icon and Android-specific IPA-hiding styles

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

Change 419430 had a related patch set uploaded (by Mholloway; owner: Mholloway):
[apps/android/wikipedia@master] Remove IPA hiding styles

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

Change 417936 merged by jenkins-bot:
[mediawiki/extensions/MobileApp@master] Consolidate common app styles

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

Change 419103 merged by jenkins-bot:
[mediawiki/extensions/MobileApp@master] Remove frwiki.less and the mobile.app.pagestyles.android module

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

Change 419104 merged by jenkins-bot:
[mediawiki/extensions/MobileApp@master] Eliminate mobile.app.pagestyles

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

Change 419443 had a related patch set uploaded (by Mholloway; owner: Mholloway):
[mediawiki/services/mobileapps@master] Stop hiding IPAs in mobile-sections page content

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

Change 419102 merged by jenkins-bot:
[apps/android/wikipedia@master] Stop bundling frwiki-specific CSS snippets

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

Mholloway updated the task description. (Show Details)Mar 14 2018, 4:36 PM

Change 419105 merged by jenkins-bot:
[apps/android/wikipedia@master] Stop bundling eliminated mobile.app.pagestyles bundle and update CSS

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

Mholloway updated the task description. (Show Details)Mar 14 2018, 5:58 PM
Mholloway updated the task description. (Show Details)
Mholloway closed this task as Resolved.Mar 14 2018, 6:00 PM

Created a separate task, T189715, for discussion of showing IPAs in Android app article views, since there are some open questions around it. Since we know we can work toward that as a goal, I'm closing out this task. Thanks for the reviews, @Dbrant!

@Mholloway

Historically we've had different 'pagestyles' and 'preview' modules, with the 'preview' modules effectively just omitting some styles. Does anyone know why we omit certain styles for previews? Could we just use the same CSS bundle in all cases?

I think this should be ok. I tested a quick edit & preview using the normal styles instead of the preview styles and it looked fine.

Here's a silly question for anyone who was around when early app architecture choices were being made. Why do the apps bundle CSS at all? Links to the stylesheet(s) needed for a page could have instead been exposed through action=mobileview, for example. Was the idea to save on network requests? Or that many of the modules ResourceLoader identified as relevant to a page wouldn't actually be needed in the apps? Maybe both?

iirc it was to ensure saved pages always had everything they needed regardless of web view components internal (sometimes opaque) rules for clearing their respective internal caches. We moved article HTML, images and CSS to caches we control. In the CSS case this was easiest to accomplish with a link tag pointing to a bundled CSS file.

Change 419776 had a related patch set uploaded (by Mholloway; owner: Mholloway):
[mediawiki/extensions/MobileApp@wmf/1.31.0-wmf.25] Consolidate common app styles

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

Change 419784 had a related patch set uploaded (by Mholloway; owner: Mholloway):
[mediawiki/extensions/MobileApp@wmf/1.31.0-wmf.25] Remove frwiki.less and the mobile.app.pagestyles.android module

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

Change 419785 had a related patch set uploaded (by Mholloway; owner: Mholloway):
[mediawiki/extensions/MobileApp@wmf/1.31.0-wmf.25] Eliminate mobile.app.pagestyles

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

Change 419776 merged by MaxSem:
[mediawiki/extensions/MobileApp@wmf/1.31.0-wmf.25] Consolidate common app styles

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

Change 419784 merged by MaxSem:
[mediawiki/extensions/MobileApp@wmf/1.31.0-wmf.25] Remove frwiki.less and the mobile.app.pagestyles.android module

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

Change 419785 merged by MaxSem:
[mediawiki/extensions/MobileApp@wmf/1.31.0-wmf.25] Eliminate mobile.app.pagestyles

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

Change 419423 merged by Mholloway:
[mediawiki/extensions/MobileApp@master] Remove IPA button icon and Android-specific IPA-hiding styles

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

Change 419167 merged by jenkins-bot:
[mediawiki/services/mobileapps@master] Remove /css/pageview route to reflect ongoing app CSS consolidation

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

Mholloway reopened this task as Open.EditedMar 19 2018, 3:57 PM

One more question for the app teams. The MobileApp extension files issues.less and disambig.less both appear not to be needed any longer since page issues and disambiguation messages have both been split off into native components ("Content issues" and "Similar pages," respectively). Can you confirm that these styles can be dropped?

Change 419443 merged by jenkins-bot:
[mediawiki/services/mobileapps@master] Stop hiding IPAs in mobile-sections page content

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

Change 419430 merged by jenkins-bot:
[apps/android/wikipedia@master] Remove IPA hiding styles

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

Mholloway renamed this task from App style consolidation open questions to Eliminate special-case app styling.Mar 23 2018, 8:18 PM
Mholloway lowered the priority of this task from High to Normal.

Added new subtask T190569, which blocks removal of the last remaining bit of Parsoid-specific styling in MediaWiki-extensions-MobileApp.

Change 421947 had a related patch set uploaded (by Mholloway; owner: Mholloway):
[mediawiki/extensions/MobileApp@master] Remove additional outdated and unnecessary styles

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

Change 421984 had a related patch set uploaded (by Mholloway; owner: Mholloway):
[apps/android/wikipedia@master] Remove additional outdated and unnecessary styles

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

Change 421947 merged by jenkins-bot:
[mediawiki/extensions/MobileApp@master] Remove additional outdated and unnecessary styles

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

Change 421984 merged by jenkins-bot:
[apps/android/wikipedia@master] Remove additional outdated and unnecessary styles

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

LGoto added a subscriber: LGoto.Apr 2 2018, 8:01 PM

Hi @Mholloway is this still blocked? Do you need anything? Thanks!

Mholloway added a comment.EditedApr 2 2018, 8:07 PM

Hi @LGoto, this is blocked on T190569, which I think is on the backlog for everyone involved; there's also a new bit of app-specific CSS that was actually added for T189715. This is still a secondary goal in the medium to long term, but maybe it belongs on the backlog for now since there's no clear path forward and it's no longer blocking our current goals.

Mholloway renamed this task from Eliminate special-case app styling to Move special-case app styling to PCS.Apr 17 2018, 7:40 PM
Mholloway renamed this task from Move special-case app styling to PCS to Move special-case app styling from the MobileApp extension to PCS.Apr 17 2018, 7:42 PM
Mholloway removed Mholloway as the assignee of this task.Jun 19 2018, 12:02 AM
Jhernandez lowered the priority of this task from Normal to Low.Jul 6 2018, 11:19 AM
Jhernandez moved this task from Upcoming to Backlog on the Reading-Infrastructure-Team-Backlog board.
Jhernandez added a subscriber: Jhernandez.

Hey @Mholloway do you think we could move the CSS to be included in some of the /data/css endpoints? Then when apps reference those endpoints instead of using the MobileApp one we could clean it up finally.

Does that sound reasonable?

@Jhernandez Yes, that could be easily done after both apps switch to consuming the /data/css endpoints. That said, the /data/css/base response assumes the client will be applying it to Parsoid HTML, which the iOS app isn't using yet. I'm not sure when that switchover is slated to happen, but I'm guessing it won't be until after Parsoid language variant support is complete.

bearND added a comment.Aug 9 2018, 8:02 PM

I think the iOS app would not switch to using mobile-sections. Instead they would go directly to mobile-html, but after Android has switched. IOW, I think it's going to take a while.

Maybe it makes sense for this to be in pagelib CSS instead of the base endpoint.

That seems reasonable although I know that the pagelib maintainers have been pretty strict (a good thing!) about what does and doesn't belong in pagelib.

There are three remaining snippets:

  1. Special styling for images with class image_overflow_x_container
  2. A snippet to remove the (listen) parenthetical from IPAs
  3. A snippet to center images and videos wrapped in <figure> elements (Parsoid-specific, but shouldn't hurt the iOS app since the MediaWiki PHP parser doesn't emit figure tags and the rule will never be activated).

@Mhurd Am I correct that (1) can be discarded, and could (2) (a small transform) and (3) (a style override) be added to pagelib?

bearND added a comment.Mar 5 2019, 5:49 AM

Reading through old comments and adding my 2 cents:

Here's a silly question for anyone who was around when early app architecture choices were being made. Why do the apps bundle CSS at all? Links to the stylesheet(s) needed for a page could have instead been exposed through action=mobileview, for example.

The apps currently build up the HTML DOM from the pieces of section text in the action=mobileview. The <head> element which contains the links to the CSS stylesheets come from the bundled index.html. action=mobileview only contains the contents.

  1. A snippet to remove the (listen) parenthetical from IPAs

That could be also done in mobile-html.

Jhernandez raised the priority of this task from Low to Normal.Mar 14 2019, 4:12 PM