Page MenuHomePhabricator

ResourceLoader 'target' filter not applied to "exempt" modules
Closed, ResolvedPublic2 Story Points

Description

Happens in Chrome 51.0.2704.84 (64-bit) / Mac OS X 10.11.6 (15G31).

It happens in the JS version of the menu too:

To reproduce go to https://en.m.wikipedia.beta.wmflabs.org/wiki/Main_Page and click on the hamburger icon.

The regression hasn't hit production yet. Here is a screenshot from https://en.m.wikipedia.org/wiki/Book

Event Timeline

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptAug 15 2016, 11:03 PM
Danny_B renamed this task from Regression: The "Disclaimers" link in Special:MobileMenu is misaligned to The "Disclaimers" link in Special:MobileMenu is misaligned.Aug 15 2016, 11:05 PM
Danny_B added a project: Regression.
Jdlrobson triaged this task as High priority.Aug 16 2016, 5:17 PM
Jdlrobson added a subscriber: Jdlrobson.

We need replication steps.

bmansurov updated the task description. (Show Details)Aug 16 2016, 5:18 PM
bmansurov updated the task description. (Show Details)Aug 16 2016, 5:32 PM

@bmansurov has the team reviewed this? If not, it could be estimated at standup.

@MBinder_WMF, yes the team has reviewed this. We didn't have a chance to estimate it though.

@bmansurov OK, if you're confident that there is a shared understanding of the task, and the team has evaluated it for potential hidden risk, please feel free to put a number on it.

bmansurov set the point value for this task to 0.5.Aug 16 2016, 8:02 PM
Jdlrobson moved this task from To Do to Doing on the Reading-Web-Sprint-79-Uh-oh board.

I'm a bit worried about this so I've switched focus here... more information to follow.

Jdlrobson raised the priority of this task from High to Unbreak Now!.Aug 16 2016, 10:05 PM
Jdlrobson added subscribers: Catrope, Krinkle, ori, greg.

It looks like the content of MediaWiki:Common.css is now being load on mobile and MediaWiki:Mobile.css is being ignored as of T87871 and this change. According to grafana, as a result of this on the beta cluster, I'm seeing a 6kb increase in the bytes of css we shift to users since 14th August for no obvious benefit.

Given this css is top loaded, this is likely to lead to a negative impact on first paint for all mobile users as so I'm slapping an unbreak now and making this a deployment blocker pending further investigation.

cc @ori @Krinkle @Catrope @greg

Restricted Application added subscribers: Luke081515, TerraCodes. · View Herald TranscriptAug 16 2016, 10:05 PM
Jdlrobson renamed this task from The "Disclaimers" link in Special:MobileMenu is misaligned to MediaWiki:Common.css loaded on mobile instead of MediaWiki:Mobile.css.Aug 16 2016, 10:05 PM
greg added a comment.Aug 16 2016, 10:13 PM

Plan of action? wmf.15 is only on testwikis right now. I assume we'll keep that and deploy a fix to wmf.15 asap before rolling further?

My preference would be to revert the patch from @Krinkle and amend it to take this into account but I'm not familiar with the patch and the trade offs of doing so. @Krinkle please advise.

Krinkle claimed this task.Aug 16 2016, 10:19 PM

My preference would be to revert the patch from @Krinkle and amend it to take this into account but I'm not familiar with the patch and the trade offs of doing so. @Krinkle please advise.

Reverting at this point would cost a lot more than simply updating MobileFrontend to play along with the new restriction. I'll check it out today.

Krinkle renamed this task from MediaWiki:Common.css loaded on mobile instead of MediaWiki:Mobile.css to ResourceLoader 'target' filter not applied to "exempt" modules.Aug 16 2016, 10:35 PM
Krinkle edited projects, added MediaWiki-ResourceLoader; removed MobileFrontend.

Change 305147 had a related patch set uploaded (by Krinkle):
OutputPage: Apply target and origin filter to exempt modules

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

Krinkle added a comment.EditedAug 16 2016, 10:58 PM

[..] simply updating MobileFrontend

No change needed in MobileFrontend actually. During the separation between OutputPage handling and ResourceLoader queue formatting, the filter was kept in OutputPage. This fixed several bugs, but meant that a handful of exempt modules (specifically, startup, site.styles and user.styles) were no longer subject to target filtering.

  • While the site module was still correctly filtered out on mobile, site.styles came through.
  • Since startup no longer had target=mobile set, it didn't get the registration for mobile.site. So when the bottom queue tried to load mobile.site (and its styles), it resolved to nothing.

The end result is that, for these two unrelated reasons, Common.css got loaded and Mobile.css did not.

Fixed in https://gerrit.wikimedia.org/r/305147.

Change 305147 merged by jenkins-bot:
OutputPage: Apply target and origin filter to exempt modules

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

Change 305153 had a related patch set uploaded (by Jdlrobson):
OutputPage: Apply target and origin filter to exempt modules

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

Jdlrobson changed the point value for this task from 0.5 to 2.Aug 16 2016, 11:26 PM

Change 305153 merged by jenkins-bot:
OutputPage: Apply target and origin filter to exempt modules

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

Krinkle closed this task as Resolved.Aug 17 2016, 4:11 AM
Restricted Application added a subscriber: Jay8g. · View Herald TranscriptOct 11 2016, 5:12 PM