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

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.

In T236097#5806715, @Niedzielski wrote:

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.

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

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

In T236097#6015717, @Jdlrobson wrote:

@zeljkofilipin am a bit stuck on https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/Popups/+/571465/

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

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.

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.

Bug report here: T249902: Page preview in Minerva show a cut off settings cog

In T236097#6045708, @ovasileva wrote:

Bug report here: T249902: Page preview in Minerva show a cut off settings cog

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

In T236097#6046929, @Jdlrobson wrote:

In T236097#6045708, @ovasileva wrote:

Bug report here: T249902: Page preview in Minerva show a cut off settings cog

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.