Page MenuHomePhabricator
Create Task
Maniphest T189312

Move special-case app styling from the MobileApp extension to PCS
Closed, DeclinedPublic
Actions

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?

Details

Show related patches Customize query in gerrit

Related Objects
Search...

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
• Mholloway triaged this task as High priority.Mar 9 2018, 4:51 PM
• 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 moved this task from To Do to Doing on the Product-Infrastructure-Team-Backlog-Deprecated (Kanban) board.Mar 12 2018, 5:18 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.

gerritbot subscribed.Mar 13 2018, 3:42 AM

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

gerritbot added a project: Patch-For-Review.Mar 13 2018, 3:42 AM
gerritbot added a comment.Mar 13 2018, 3:45 AM

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

gerritbot added a comment.Mar 13 2018, 3:52 AM

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

gerritbot added a comment.Mar 13 2018, 4:07 AM

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

gerritbot added a comment.Mar 13 2018, 4:31 AM

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

gerritbot added a comment.Mar 13 2018, 12:48 PM

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 added a comment.Mar 13 2018, 1:23 PM

https://github.com/wikimedia/wikipedia-ios/pull/2108

• 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
gerritbot added a comment.Mar 14 2018, 1:38 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

gerritbot added a comment.Mar 14 2018, 1:55 PM

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

gerritbot added a comment.Mar 14 2018, 2:11 PM

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

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

gerritbot added a comment.Mar 14 2018, 2:30 PM

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

gerritbot added a comment.Mar 14 2018, 2:41 PM

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

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

gerritbot added a comment.Mar 14 2018, 2:51 PM

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

ReleaseTaggerBot added a project: MW-1.31-release-notes (WMF-deploy-2018-03-20 (1.31.0-wmf.26)).Mar 14 2018, 3:00 PM
gerritbot added a comment.Mar 14 2018, 4:09 PM

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
gerritbot added a comment.Mar 14 2018, 5:06 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!

Mhurd added a comment.Mar 14 2018, 11:29 PM

@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.

gerritbot added a comment.Mar 15 2018, 4:31 PM

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

gerritbot added a comment.Mar 15 2018, 4:39 PM

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

gerritbot added a comment.Mar 15 2018, 4:41 PM

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

gerritbot added a comment.Mar 15 2018, 6:30 PM

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

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

gerritbot added a comment.Mar 15 2018, 6:30 PM

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

gerritbot added a comment.Mar 15 2018, 6:46 PM

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

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

ReleaseTaggerBot edited projects, added MW-1.31-release-notes (WMF-deploy-2018-03-13 (1.31.0-wmf.25)); removed MW-1.31-release-notes (WMF-deploy-2018-03-20 (1.31.0-wmf.26)).Mar 15 2018, 7:00 PM
gerritbot added a comment.Mar 15 2018, 8:43 PM

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

ReleaseTaggerBot edited projects, added MW-1.31-release-notes (WMF-deploy-2018-03-20 (1.31.0-wmf.26)); removed MW-1.31-release-notes (WMF-deploy-2018-03-13 (1.31.0-wmf.25)).Mar 15 2018, 9:01 PM
gerritbot added a comment.Mar 16 2018, 9:19 PM

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?

gerritbot added a comment.Mar 20 2018, 5:07 PM

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

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

gerritbot added a comment.Mar 20 2018, 6:44 PM

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

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

• Mholloway moved this task from Doing to Blocked on the Product-Infrastructure-Team-Backlog-Deprecated (Kanban) board.Mar 22 2018, 6:39 PM
• 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 Medium.
• Mholloway added a subtask: T190569: Most images are not centered at mobile screen widths in Parsoid HTML.
• Mholloway added a comment.Mar 23 2018, 8:33 PM

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

gerritbot added a comment.Mar 26 2018, 4:55 PM

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

• Mholloway mentioned this in rAPIOS3c7d5598a51a: Remove unneeded styles.Mar 26 2018, 7:23 PM
gerritbot added a comment.Mar 26 2018, 7:53 PM

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

gerritbot added a comment.Mar 27 2018, 1:44 PM

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

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

ReleaseTaggerBot edited projects, added MW-1.31-release-notes (WMF-deploy-2018-03-27 (1.31.0-wmf.27)); removed MW-1.31-release-notes (WMF-deploy-2018-03-20 (1.31.0-wmf.26)).Mar 27 2018, 2:00 PM
gerritbot added a comment.Mar 27 2018, 2:04 PM

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

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

Dbrant moved this task from Needs Triage to Tracking on the Wikipedia-Android-App-Backlog board.Mar 27 2018, 3:35 PM
• Mholloway mentioned this in rAPIOS21aec94e208d: Remove unneeded styles.Mar 27 2018, 5:33 PM
LGoto subscribed.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 mentioned this in rAPIOSa696d0e73e51: Remove unneeded styles.Apr 6 2018, 9:47 PM
• Mholloway edited projects, added Product-Infrastructure-Team-Backlog-Deprecated; removed Patch-For-Review, Product-Infrastructure-Team-Backlog-Deprecated (Kanban).Apr 12 2018, 2:39 PM
• Mholloway moved this task from Needs triage to Upcoming on the Product-Infrastructure-Team-Backlog-Deprecated board.
• 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 Medium to Low.Jul 6 2018, 11:19 AM
• Jhernandez moved this task from Upcoming to Backlog on the Product-Infrastructure-Team-Backlog-Deprecated board.
• Jhernandez subscribed.

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?

• Mholloway added a comment.Aug 9 2018, 8:00 PM

@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.

• Jhernandez added a comment.Aug 10 2018, 12:52 PM

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

• Mholloway added a comment.Aug 10 2018, 3:50 PM

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:

In T189312#4044716, @Mholloway wrote:

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.

In T189312#4494609, @Mholloway wrote:
  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 Medium.Mar 14 2019, 4:12 PM
• Jhernandez lowered the priority of this task from Medium to Low.Sep 11 2019, 12:01 PM
• Jhernandez removed a parent task: T188919: Integrate common CSS into the Page Content Service.Sep 11 2019, 12:41 PM
• Jhernandez unsubscribed.Apr 2 2020, 6:46 PM
LGoto closed this task as Declined.Oct 9 2020, 4:50 PM
Content licensed under Creative Commons Attribution-ShareAlike (CC BY-SA) 4.0 unless otherwise noted; code licensed under GNU General Public License (GPL) 2.0 or later and other open source licenses. By using this site, you agree to the Terms of Use, Privacy Policy, and Code of Conduct. · Wikimedia Foundation · Privacy Policy · Code of Conduct · Terms of Use · Disclaimer · CC-BY-SA · GPL