Page MenuHomePhabricator

Dev: Popups modules should set targets to mobile and desktop
Closed, ResolvedPublic3 Estimated Story Points

Description

A request from the performance team to help spearhead their efforts in a performance initiative.

As part of the performance effort to dismantle the targets system (T127268), we should set targets to mobile and desktop for Popups module ext.popups using the following pattern:

if (  ExtensionRegistry::isLoaded( 'MobileFrontend' ) ) {
  $ctx = MediaWikiServices::getInstance()->getService( 'MobileFrontend.Context' );
  if ( !$ctx->shouldDisplayMobileView() ) {
     $out->addModule('ext.popups')
  } 
} else {
   // MobileFrontend is not installed.
   $out->addModule('ext.popups')
}

Acceptance criteria

  • Add the mobile target to ext.popups module
  • Make sure the module doesn't load on the mobile domain.

QA steps

QA Results - Beta

QA Results - Prod

Event Timeline

Sounds good to me! See T235712#5593272 on the details though.

Jdlrobson updated the task description. (Show Details)Oct 22 2019, 12:03 AM
Jdlrobson triaged this task as Medium priority.Oct 22 2019, 12:08 AM
Jdlrobson updated the task description. (Show Details)
Jdlrobson renamed this task from Popups modules should set targets to mobile and desktop to Dev: Popups modules should set targets to mobile and desktop.Oct 22 2019, 9:36 PM
Jdlrobson assigned this task to ovasileva.
Jdlrobson updated the task description. (Show Details)
Jdlrobson moved this task from Incoming to Needs Prioritization on the Readers-Web-Backlog board.
Jdlrobson moved this task from Inbox to Next up on the User-Jdlrobson board.

The team expressed a little concern about extensions/skins having knowledge of MobileFrontend.

An alternative solution would be to always load ext.popups but only load the remaining code if hover is available similar to so:

if (window.matchMedia( "(hover: none)" ).matches) {
   // hover unavailable
}

I've asked @Niedzielski and @pmiazga to chip in on T235712 and T127268 with their thoughts and concerns.

T158181 sounds like a nice objective. For this task specifically, I like @Jdlrobson's generic hover proposal much better than what appears in the description.

On the backend side we could use the Hooks system to allow other extensions to disable PagePreviews. The code would look like sth like that:

$shouldSendModuleToUser = true;
Hooks::run( 'PopupsInitialization', [ &$shouldSendModuleToUser ] );

if ($shouldSendModuleToUser) {
    $out->addModule('ext.popups')
}

and then on the MobileFrontend side we could listen to PopupsInitialization and return false to block extension from ext.popups module registration. With that approach Popups would not have any logic related to MobileFrontend, especially it wouldn't call $ctx->shouldDisplayMobileView(). Any calls to MobileContext should be executed only inside MobileFrontend extension.

T158181 sounds like a nice objective. For this task specifically, I like @Jdlrobson's generic hover proposal much better than what appears in the description.

I should note I don't know how feasible it is. I don't believe it can be tested that way in older browsers. We'd need to do some research into the feasibility. We could also use screen width as an indicator but that adds risk that we might disable page previews on desktop by mistake.

Change 571465 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/extensions/Popups@master] Enable Popups module in mobile, use feature detection to enable

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

Following on from our previous estimation of the team concerned that Popups was aware of MobileFrontend (T236097#5806293) I only see one other way to do this to aid the performance team initiative which is to use feature detection (here the non-availability of ontouchstart in the browser).

While being a much better technical solution, this is slightly more risky than the other approach as it has potential for disabling Popups in any browsers where ontouchstart event is enabled. http://www.stucox.com/blog/you-cant-detect-a-touchscreen/ has a good lowdown on that risk. On the other hand understanding the sorts of browsers our users use is a good learning opportunity. Such a solution also needs product sign off as it will enable page previews for desktop users who are using the mobile site (however I'd argue that's a good thing)

I'd rather not prolong this conversation so I think we have 2 solutions here and should pick a preference depending on the team's risk tolerance

  1. Use ExtensionRegistry::isLoaded( 'MobileFrontend' ) and incur a little tech debt
  2. Use ontouchstart event detection

@Jdlrobson why not use hooks? It doesn't introduce tech deb, you define new Hook called sth like PopupsInitialization, and allow other extensions to listen to that hook and returm false. See https://phabricator.wikimedia.org/T236097#5809640
IMHO it's much cleaner approach and it doesn't introduce any tech debt. What more, if there is any other Popups like extension, it will be also able to easily disable PagePreviews so it doesn't conflict.

With this approach we could also move the navigational_popups gadget detection to Gadgets extension, and move that navigational_popups gadget logic away from Popups extension.

It doesn't introduce tech deb, you define new Hook called sth like PopupsInitialization, and allow other extensions to listen to that hook and returm false. See https://phabricator.wikimedia.org/T236097#5809640

I hadn't considered such an approach. Yes this could work. Popups would not need to know about MobileFrontend but would be providing a way for other things to disable it / abuse it. MobileFrontend would subscribe to that hook and check if mobile view is enabled. However would we want hooks for every situation where we do this? If all extensions are defining what are essentially kill switches, I wonder If so is that any better than the current setup?

Another consideration with this is that although it will retain the existing behaviour. It would also mean that page previews loads on touch devices. When I use Minerva desktop or Vector desktop on a touch screen device for example I get page previews. Not sure if that's a feature or a bug :)

Personally despite the risk from a technical perspective I prefer "Use ontouchstart event detection" solution. It feels the most clean. I'll be sure to include this 3rd option when we discuss.estimate as team and get back to you.

With this approach we could also move the navigational_popups gadget detection to Gadgets extension, and move that navigational_popups gadget logic away from Popups extension.

Putting aside the cumbersome moving of wgPopupsConflictingNavPopupsGadgetName from Popups to the Gadget extensioon, I don't think Gadgets should know about the gadgets that are installed and have bespoke behavior in this fashion. I consider the code in Popups to that effect technical debt. Ideally we wouldn't have code like this. For example a 3rd party wiki vanilla install has no need for it.

ovasileva set the point value for this task to 3.Mar 3 2020, 6:28 PM
Jdlrobson added a subscriber: zeljkofilipin.

@zeljkofilipin am a bit stuck on https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/Popups/+/571465/ - can you help? Is there a way to change the characteristics of a browser? In this case I want to disable touch events so that it emulates a desktop browser rather than a phone.

Jdlrobson added a subscriber: ovasileva.

Change 571465 merged by jenkins-bot:
[mediawiki/extensions/Popups@master] Enable Popups module in mobile, use feature detection to enable

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

Apologies for being slow on this, but looks like the problem is already resolved.

Jdlrobson updated the task description. (Show Details)Apr 8 2020, 12:13 AM

Test Result - Beta

Status: ✅ PASS
Environment: beta
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: iPhoneX

Test Artifact(s):

QA steps

✅ AC1: Visit https://en.wikipedia.beta.wmflabs.org/wiki/Christopher_Nixon_Cox#/random on a desktop browser. Confirm that hovering over "President of the United States" shows a very badly formatted Popup (don't worry about its contents). If it doesn't check your preferences/the footer to ensure Popups is enabled and try again.


✅ AC2: Visit https://en.wikipedia.beta.wmflabs.org/wiki/Christopher_Nixon_Cox#/random on a mobile browser (if redirected click "desktop view" at the bottom. Confirm that it's not possible to show a page preview when you hover over "President of the United States"
No page preview appeared.
✅ AC3: Visit https://en.m.wikipedia.beta.wmflabs.org/wiki/Christopher_Nixon_Cox#/random on a desktop browser. Confirm that hovering over "President of the United States" shows a very badly formatted Popup (don't worry about its contents)

✅ AC4: Visit https://en.m.wikipedia.beta.wmflabs.org/wiki/Christopher_Nixon_Cox#/random on a mobile browser. Confirm that it's not possible to show a page preview when you hover over "President of the United States"
No page preview appeared.

Test Result - Prod

Status: ✅ PASS
Environment: enwiki
OS: macOS Catalina
Browser: Chrome
Device: MBP
Emulated Device: iPhoneX

Test Artifact(s):

QA steps

✅ AC1: Visit https://en.wikipedia.org/wiki/Christopher_Nixon_Cox#/random on a desktop browser. Confirm that hovering over "President of the United States" shows a very badly formatted Popup (don't worry about its contents). If it doesn't check your preferences/the footer to ensure Popups is enabled and try again.


✅ AC2: Visit https://en.wikipedia.org/wiki/Christopher_Nixon_Cox#/random on a mobile browser (if redirected click "desktop view" at the bottom. Confirm that it's not possible to show a page preview when you hover over "President of the United States"
No page preview appeared.
✅ AC3: Visit https://en.m.wikipedia.org/wiki/Christopher_Nixon_Cox#/random on a desktop browser. Confirm that hovering over "President of the United States" shows a very badly formatted Popup (don't worry about its contents)

✅ AC4: Visit https://en.m.wikipedia.org/wiki/Christopher_Nixon_Cox#/random on a mobile browser. Confirm that it's not possible to show a page preview when you hover over "President of the United States"
No page preview appeared.

Edtadros reassigned this task from Edtadros to ovasileva.Apr 10 2020, 6:20 AM
Edtadros updated the task description. (Show Details)
Edtadros added a subscriber: Edtadros.
ovasileva closed this task as Resolved.Apr 10 2020, 9:04 AM

Resolved. Opening a task to fix the cog but it's definitely low priority

Just to clarify this is not related to this task and likely s bug for some time.

Just to clarify this is not related to this task and likely s bug for some time.

Yes, I'm aware. Just opening it now since the QA here brought it to light.