Page MenuHomePhabricator

GrowthExperiments shouldn't be setting the AMC mode
Closed, ResolvedPublic

Description

HomePageHooks in the GrowthExtension reuses the talk_on_top feature to show link to the "My homepage" page. To make it happen the SkinOptions::OPTIONS_TALK_AT_TOP and SkinOptions::OPTIONS_TABS_ON_SPECIALS has to be set. Sadly, because of some reason, HomepageHooks also enables the OPTION_AMC, which enables the Advanced Mobile Contributions mode on mobile.

The SkinOptions::OPTION_AMC should not be enabled, as it additionally enables new features like UserMenu (visible UI changes).

Referenced code: HomepageHooks::onSkinMinervaOptionsInit()

Event Timeline

pmiazga created this task.Jul 25 2019, 4:39 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 25 2019, 4:39 PM
Jdlrobson moved this task from Incoming to Needs Prioritization on the Readers-Web-Backlog board.
pmiazga triaged this task as High priority.Jul 25 2019, 5:09 PM

The reason is in the line directly above the one referenced in this task description, namely that this results in correct styles being applied to the AMC menu.

As far as I can tell the fix would be that if one is rendering the AMC menu, the necessary styles should be loaded and those should not be behind the AMC enabled flag.

Niedzielski renamed this task from GrowthExtension shouldn't be setting the AMC mode to GrowthExperiments shouldn't be setting the AMC mode.Jul 25 2019, 6:53 PM

Change 525630 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/skins/MinervaNeue@master] Enable AMC styles if only one of the AMC properties is enabled

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

@kostajh I think something like https://gerrit.wikimedia.org/r/#/c/mediawiki/skins/MinervaNeue/+/525630 would be best here - can you confirm with that change you wouldn't need to enable AMC mode?

Also can you confirm this only applies for logged in users?

Jdlrobson reassigned this task from Jdlrobson to kostajh.Jul 25 2019, 8:21 PM
Jdlrobson added a subscriber: Jdlrobson.

Change 525656 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[mediawiki/extensions/GrowthExperiments@master] Remove setting of OPTION_AMC to true

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

can you confirm with that change you wouldn't need to enable AMC mode?

Yes something like that would work, but I commented on the patch with a change request.

Also can you confirm this only applies for logged in users?

That's right, this is just for logged-in users.

Change 525670 had a related patch set uploaded (by Kosta Harlan; owner: Kosta Harlan):
[mediawiki/skins/MinervaNeue@master] Split getSkinStyles into separate class, add unit test

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

Change 525630 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Enable AMC styles if only one of the AMC properties is enabled

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

Change 525656 merged by jenkins-bot:
[mediawiki/extensions/GrowthExperiments@master] Remove setting of OPTION_AMC to true

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

kostajh added a subscriber: Etonkovidova.

The SkinOptions::OPTION_AMC should not be enabled, as it additionally enables new features like UserMenu (visible UI changes).

@Etonkovidova the relevant bit for QA is this (users with homepage enabled and _not_ opted into AMC should not see UserMenu) and that the tabs on Special:Homepage display with the AMC styles, as they did before.

Jdlrobson lowered the priority of this task from High to Medium.Jul 30 2019, 12:36 AM

@kostajh related discussion kicked off on T229295

@pmiazga will take a look at this today.

Etonkovidova closed this task as Resolved.Jul 31 2019, 1:27 AM

Tested in betalabs - enabling Homepage (from Special:Preferences or by default for a new user) doesn't enable AMC. I checked the absence of AMC according to list AMC-related features T229295.