Page MenuHomePhabricator

Update WMF-deployed extensions to use mw.config checks instead of manual m-dot URL hacks
Closed, ResolvedPublic

Description

Problem

A handful of recently-developed MediaWiki extensions appear to scrape the address bar, looking for m. to activate some mobile-specific behaviour (Codesearch).

Bad (JavaScript)
location.hostname.includes( '.m.' );
Bad (PHP)
MobileContext->hasMobileDomain();

MobileContext->usingMobileDomain();

These are suboptimal today because:

  • .. likely conflates MobileFrontend and the Minerva skin. Once upon a time these were the same thing, but since 2016, the Minerva skin has been its own thing. In most cases, conditionals are related to differences in skin HTML/CSS, which means they should apply even when use use the Minerva skin on desktop. MobileFrontend is much thinner than it used to be, largely focussed on post-processing of article body content, such as moving the lead paragraph above the infobox.
  • .. it makes testing harder, since location is a complex global variable that is difficult to mock.
  • .. you can select the Minerva skin in your preferences even on desktop.
  • .. en.wikipedia.org/?useformat=mobile activates MobileFrontend but is not an m-dot URL.
  • .. en.wikipedia.org/?useskin=minerva activates the Minerva skin but is not an m-dot URL.
  • .. en.m.wikipedia.org/?useskin=vector activates MobileFrontend with the Vector skin, from an m-dot URL.

While these are relatively minor limitations today, with T214998: RFC: Serve mobile and desktop variants through the same URL (unified mobile routing) this unsupported mechanism will become ineffective, as most mobile views will then take place on standard domains instead of a mobile subdomain.

Background

There are standard and reliable mechanisms that should be used instead. These have existed for many years, and the majority of our code bases requires no changes, because they already use one of these mechanisms. To our knowledge, there are no circumstances in which an extension or gadget can't make use of one of these mechanisms.

If a caller needs to style their component differently to match or accomodate the surrounding skin, you can do this directly in CSS:

Recommendation 1 (CSS)
.skin-minerva .something {
  /* .. */
}

Or if your component should only be generated, or generated differently depending on the skin, you can detect that explicitly in JavaScript via mw.config. You can detect the Vector or Timeless skins this way as well.

Recommendation 2 (JavaScript)
if ( mw.config.get( 'skin' ) === 'minerva' ) {
 // ...
}

Or to detect specifically that MobileFrontend is active on the current pageview (no matter how it is activated, e.g. by URL, user-agent, query param, etc), that can be done via mw.config as well:

Recommendation 3 (JavaScript)
if ( mw.config.get( 'wgMFMode' ) ) {
 // ...
}

For server-side code, likewise, avoid checking against the mobile "domain" but rather check whether the mobile mode is active (regardless of how it was activated):

Recommendation 4 (PHP)
MobileContext->shouldDisplayMobile();

Scope

Blocking:

  • ContentTranslation: mw.cx.SiteMapper.js, can this use use mw.config instead?
  • Metrics Platform : client_platform_family in correctly set in the JavaScript client, but the PHP client uses the internal MobileContext->usingMobileDomain() function directly, instead of MobileContext->shouldDisplayMobile().

Non-blocking:

  • Wikistories: convertUrlToMobile.js.
    • This is okay-ish. It will continue to work fine. Once the m-dot becomes legacy, the URLs generated by this function will become redirects, at which point this code can be safely removed.
    • Currently, this exists behind a few layers of re-inventions and assumptions that are close enough to work on Wikipedia, but break elsewhere. This particular function appears to be working around the fact that imageinfo.descriptionshorturl returns URLs like https://commons.wikimedia.org/w/index.php?curid=5750750 which are non-canonical, less-cached, and indeed don't qualify for known-safe automatic redirects to the m-dot URL. This should perhaps use concept URIs from WikibaseMediaInfo instead, e.g. https://commons.wikimedia.org/entity/M137720198 which redirect the canonical URL like https://commons.wikimedia.org/wiki/File:Tuscany_Pitigliano.jpg and display the mobile version.
  • operations/mediawiki-config: missing.php. This is used to activate a "m-dot domain not found" error page, e.g. when WMF is amidst setting up a new domain name for a wiki site. This is a rare case where checking the domain is the appropiate and correct thing to do, since this relates to mistakes or incomplete DNS and Apache VirtualHost config, specific to m-dot domains. Since the m-dot subdomain will continue to exist, albeit as a mere redirect, this error handler seems fine to keep. Although it could also be removed anytime in favour a general "domain not found" error page.

Event Timeline

Change #1133590 had a related patch set uploaded (by Krinkle; author: Krinkle):

[mediawiki/extensions/NavigationTiming@master] Remove unreachable `wgMFMode == desktop` branch

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

Change #1133590 merged by jenkins-bot:

[mediawiki/extensions/NavigationTiming@master] Remove unreachable `wgMFMode == desktop` branch

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

This could probably use a sister User-notice task for script and gadget authors.

Change #1134738 had a related patch set uploaded (by Sbisson; author: Sbisson):

[mediawiki/extensions/ContentTranslation@master] Produce mobile agnostic URL for redirection

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

I don't think CX mw.cx.SiteMapper.js is actually a blocker here.

CX is building URL for various redirection scenarios between different wikis and different tools and this code is only trying to include the .m in the new URLs if it's already present in the current URL. Actual detection of mobile mode for decision purposes is already based on mw.config.get( 'wgMFMode' ) and MobileContext->shouldDisplayMobile();

Nevertheless, I have made a patch to clean it up and get ourselves into the habit of using something else than the .m to force mobile mode on desktop during development and testing.

Change #1134738 merged by jenkins-bot:

[mediawiki/extensions/ContentTranslation@master] Produce mobile agnostic URL for redirection

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

I have filed T395079 about one of these recommendations.

Hey @Krinkle can we recommend something other than mw.config.get( 'wgMFMode' ) going forward - particularly since T395079: wgMFMode does not appear to be documented suggests there is a need for community members to use this too?

document.body.classList.contains('mw-mf') was added with this use case in mind but I understand there are concerns around forced style calculation in certain situations so I'm open to a new approach here but I don't think it should be wgMFMode.

This configuration variable was added for the MobileFrontend beta feature before we had virtual packageFiles and it got repurposed for detection, but I was hoping to remove it as soon as we have capacity as part of T385509: Remove beta code from MobileFrontend. We can keep a configuration variable instead of the class if we feel that's preferable, but I would prefer we renamed it something more explicit and make it a boolean. e.g. wgMFUsingMobileSite for example.

Side note: as part of T385509 we would handle migrations to the new variable, but I would prefer not to encourage more usages or these in the mean time.

I understand there are concerns around forced style calculation in certain situations

To write JavaScript at scale, you generally want to treat the DOM as an output-only surface. If you're reading from the DOM to tell you the state of your own code, something has gone wrong.

particularly since T395079 suggests there is a need for community members to use this too?

Documenting a variable is trivial. Renaming a widely used mechanism that does exactly what we need, and exists at the appropriate abstraction level, does not sound like a worthwhile investment. It's not a migration that improves maintainability or intuition. It's literally a rename at the same abstraction level. I would recommend against a breaking change over purely minor change to the name, in a way that others are unlikely to benefit from.

You know that "mode" used to refer to "beta mode", but most other people don't. They might just think of it as the mode of MediaWiki being in "MobileFrontent mode": if ( wgMFMode ) { … } reads quite well.

I don't think we're helping anyone by naming it to wgMFUsingMobileSite. No-one will know or guess the new name wihtout having seen it. Short of that, they'll find it in other code, or on a documentation page that we write. Can we do better than that? To allow for easy re-discovery, you generally want to follow existing naming conventions, like starting with wgMF, and use only easy-to-remember and ubiquitous English words. wgMFMode fits that bill perfectly.

I was hoping to remove it as soon as we have capacity as part of T385509: Remove beta code from MobileFrontend

I would certainly support replacing it with a boolean or frozen string constant. I don't think the name of the variable matters much. I'd also support the AMC-related clean up resulting in MobileFrontend setting it from a more appropiate place in the codebase. But, I see no need for a breaking change or for AMC-related maintenance to block, delay, add risk to T214998: RFC: Serve mobile and desktop variants through the same URL (unified mobile routing).

Hey @Krinkle

To write JavaScript at scale, you generally want to treat the DOM as an output-only surface

Yes that makes sense and I agree we don't need to block, delay the removal of the m-dot subdomain. I don't think what I'm asking for is doing that. I'm just saying there are better ways consistent with the other guidance you are giving. I think encourage MFMode adoption would actually slow this down and it will slow down plans that Roan and I have been discussing about decomissioning/majorly simplifying MobileFrontend in the next 12 months (decision brief to be shared sometime soon).

I think it's already a huge anti-pattern that people are using wgMFMode in code. It is equivalent to recommending developers use userAgent sniffing. We should be feature detecting before running code. For example - if you want to know if section collapsing is enabled, you should be detecting that and that's the only major difference between desktop and mobile experience at this point. I've been trying to push people to using mw.config.get('skin') and think about skins rather than whether the user is on the mobile domain or not. People using it right now are setting themselves up for future breakage as we slowly decommission MobileFrontend - we are already moving a lot the functionality in MobileFrontend upstream so it can be used in Vector 2022 on Wikifunctions which wants a responsive version of that skin.

T395157: Echo shouldn't make use of wgMFMode. illustrates a real world example of where choosing wgMFMode was the wrong choice.

I would say it's preferable to recommend either use:

  1. mw.config.get('skin')
  2. feature detection of the ResourceLoader module code to check if it loaded
  3. Use a window.matchMedia to detect mobile

For extensions as you've already pointed out they can use MobileContext->shouldDisplayMobile where needed. I agree with this.

Practically speaking:

  • @SBisson already pointed out ContentTranslations parsing was a mistake and unnecessary.
  • For instrumentation, I saw your patch to NavigationTiming. NavigationTiming shouldn't need to write a custom field for this. My assumption was that WikimediaEvents could be updated to send a global mobile field along with skin (preferably on all skins by default) using the a hook to check if mobile was enabled. That would actually be the quickest way to migrate all our instrumentation which currently relies on the domain. Updating all code to use wgMFMode is going to take considerably longer. I can help write code for that to speed this along if that's helpful, as I think we are going to struggle to get all the existing instrumentation to update otherwise.
  • I do not feel comfortable actively encouraging gadget developers to switch to mw.config by suggesting it is best practice. If encouraging it we should encourage it as as last resort as usually there are better options out there and these should be considered as they approach this. Happy to talk off ticket async if you disagree to find a compromise we can both agree with so that we don't send out mixed messaging here.

I think it's already a huge anti-pattern that people are using wgMFMode in code. […] I've been trying to push people to using mw.config.get('skin') and think about skins rather than whether the user is on the mobile domain or not. […] T395157 illustrates a real world example of where choosing wgMFMode was the wrong choice.

I would say it's preferable to recommend either use:

  1. mw.config.get('skin')
  2. Use a window.matchMedia to detect mobile

[…]

I think we're conflating two things: One is "how" to detect MobileFrontend, the other is "whether" one tries to detect MF in the first place. This task is first and foremost about "how".

I agree detection should be avoided, and not the first thing to try for developers. Fortunately, the fact that there are only a dozen or so instance of this pattern across the entire platform and thousands of extensions and tens of thousands of gadgets, tells me that staff and volunteers alike, already write things in a way that is almost always agnostic to device or skin. Our platform encourages that and makes it easy to do. Yay!

On top of that, exactly as you say, the task description reminds devs of this and encourages them to move away from detection entirely if possible. I've put down wgMFMode detection as number 3, with the recommended number 1 indeed to check the skin (i.e. if the underlying reason for the switch was related to skin layout, rather than someting else MF does).

But there is no getting around the fact that MobileFrontend exists today, and it has code in its repo that does stuff, which other stuff may need to interact with or be aware of. So long as MF exists, even once future plans have started, until the day after the last install of MobileFrontend on a WMF wiki is turned down, there will be a need to detect its presence in some use cases. I recommend we simply keep using wgMFMode for that purpose, and invest any resources we have to spare toward those future goals, rather than toward moving or optimising the names of things that apparently have plans to be obsoleted. Again, I agree completely that it is worthwhile reminding and encouraging devs to avoid MF detection where it is feasible to do so.

Thanks @Krinkle for the edit here.

Given some confusion elsewhere subscribers please note the advice in the title here is for WMF-deployed extensions not gadgets.

tstarling subscribed.
  • Metrics Platform : client_platform_family in correctly set in the JavaScript client, but the PHP client uses the internal MobileContext->usingMobileDomain() function directly, instead of MobileContext->shouldDisplayMobile().

The code for this is in extensions/EventLogging/MetricsPlatform not extensions/MetricsPlatform. Metrics Platform is probably the wrong project tag, given the description of that project.

Change #1156649 had a related patch set uploaded (by Tim Starling; author: Tim Starling):

[mediawiki/extensions/EventLogging@master] Avoid references to the "mobile domain"

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

Change #1156649 merged by jenkins-bot:

[mediawiki/extensions/EventLogging@master] Avoid references to the "mobile domain"

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

Krinkle assigned this task to tstarling.