Page MenuHomePhabricator

Gadget with target=mobile should not be enqueued on desktop (Warning from mw.loader.load)
Closed, ResolvedPublic

Description

I've created a gadget that only targets mobile. When I enabled this gadget, I see in my browser console on Desktop:

[Error] Error: Unknown dependency: ext.gadget.MobileMaps
Error: Unknown dependency: ext.gadget.MobileMaps
sortDependencies — load.php:159:215
resolveStubbornly — load.php:160:377
load — load.php:171:140
(anonymous function) — index.php:13
startUp — load.php:79:750
onreadystatechange — load.php:80:313
	logError (load.php:177:344)
	handler (load.php:155:846)
	fire (load.php:45:128)
	fireWith (load.php:46:436)
	fire (load.php:46:488)
	track (load.php:155:634)
	resolveStubbornly (load.php:160:434)
	load (load.php:171:140)
	(anonymous function) (index.php:13)
	startUp (load.php:79:750)
	onreadystatechange (load.php:80:313)

Details

Related Gerrit Patches:
mediawiki/extensions/Gadgets : masterOnly queue gadget module if relevant on the current target
mediawiki/extensions/Gadgets : masterRL: Apply gadgets target filter

Related Objects

Event Timeline

TheDJ created this task.Jul 20 2017, 3:30 PM
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptJul 20 2017, 3:30 PM
Krinkle triaged this task as High priority.EditedJul 22 2017, 2:05 AM
Krinkle moved this task from Inbox to Backlog on the MediaWiki-ResourceLoader board.
Krinkle added a subscriber: Krinkle.

@TheDJ Thanks. This is actually an issue in the Gadgets extension. It always enqueues the enabled gadgets, even if they are not intended for the current target.

As part of working toward removing the targets system (T127268), we're adding some detection for when currently modules are enqueued on an incompatible target.

Server-side logging was added to the filter filter as part of T140675: Log modules enqueued with invalid module target. So I assume there will already have been warnings about this in Logstash from MediaWiki PHP.

I didn't mean to add client-side logging for bad-target loads specifically (and can't since they just appear as unknown modules after the filter). However, as part of solving T36853 I did add client-side logging for modules that failed to load because they are not registered (rMW12cca1c68e98: mw.loader: Log unknown modules in load() to console).

An unintended bonus of this is that we can also have client-side warnings for modules that "exist", but in a different target. (Although the message regrettably can't tell the difference.)

Krinkle renamed this task from Error: Unknown dependency in desktop, when using mobile only gadgets to Gadget with target=mobile should not be enqueued on desktop (Warning from mw.loader.load).Jul 22 2017, 2:06 AM
TheDJ added a comment.Aug 19 2017, 9:18 PM

Just to get this clear, this is an Error exception that is being caught, so is currently harmless, but will pollute the console logging of Desktop users.

TheDJ added a comment.Sep 6 2017, 1:26 PM

After https://gerrit.wikimedia.org/r/#/c/376036/ is live, these will be warnings and not errors, which will make this problem less disruptive.

Niedzielski added a subscriber: Niedzielski.

Any idea when this will be deployed to production enwiki? I'm looking forward to this improvement!

Restricted Application added a project: Performance-Team. · View Herald TranscriptJan 3 2018, 4:16 PM

Any idea when this will be deployed to production enwiki? I'm looking forward to this improvement!

The change mentioned by @TheDJ which changes this log message from "error" to "warning" has already been deployed to all wikis a few months ago.

However, the eventual goal of this task (don't load gadgets intended for other targets), requires modifications to the Gadget extension. That work has not been assigned yet. So for that part, it's not yet a question of deploying, but first doing the actual work.

Looks like maybe Gadgets doesn't implement the requirements for Targets completely. It added support for setting the module object properties but doesn't read it back out when adding modules to the page.
Either that, or the bug is in MediaWiki core OutputPage, where the style queue is missing support for the target restriction (the stylesheet urls lacks target=). This is one of various aspects of the Target system that wasn't fully considered when it was implemented. However this will be obsolete with T127268, so I don't expect these bugs to be addressed in core (unless volunteered).
Per T127268, I'd encourage you to plan ahead for when the targets system doesn't exist anymore.
For example, you may want to use the skins filter instead (e.g. skins=minerva).

Niharika added a subscriber: Niharika.

Not sure how Community-Tech is relevant here. What am I missing?

The project tag was copied over from T127147, which Andre tagged as such in 2016. Anyhow, per Maintainers looks like this is meant to be Contributors/Editing.

(Possible explanation: I think once Community-Tech stated that they might take a look at specific gadget issues on wikis, at that time...)

Change 449005 had a related patch set uploaded (by TheDJ; owner: TheDJ):
[mediawiki/extensions/Gadgets@master] RL: Only add gadget if it supports the target

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

Change 449006 had a related patch set uploaded (by TheDJ; owner: TheDJ):
[mediawiki/extensions/Gadgets@master] RL: Apply gadgets target filter

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

Change 449006 abandoned by TheDJ:
RL: Apply gadgets target filter

Reason:
superflous

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

TheDJ added a comment.EditedJul 29 2018, 8:51 AM

I've been looking into this a bit, and the reason to me that this is a problem seems to be that there simply is no such thing as target=desktop. The only use of setTarget that I could find is by MobileFrontend and that sets 'mobile'. In all other situations, the target == NULL. (PS. what does this mean for PageOutput::filterModules ? )

Another problem I noticed is that MF uses the beforepagedisplay hook to set the target... And the gadgets are added from beforepagedisplay as well. That means that this targets patch can fail because the gadget hook is executed before the mobilefrontend.

This is a mess ;(

Majr added a comment.Jul 29 2018, 11:19 AM

I've been looking into this a bit, and the reason to me that this is a problem seems to be that there simply is no such thing as target=desktop. The only use of setTarget that I could find is by MobileFrontend and that sets 'mobile'. In all other situations, the target == NULL. (PS. what does this mean for PageOutput::filterModules ? )

Which is why I made T140863 so we can fix the skins option in gadgets to enable mobile-only gadgets by specifying targets=mobile|skins=minerva (targets is necessary while gadgets/RL still supports targets).

I've been looking into this a bit, and the reason to me that this is a problem seems to be that there simply is no such thing as target=desktop. The only use of setTarget that I could find is by MobileFrontend and that sets 'mobile'. In all other situations, the target == NULL.

Yeah, in OutputPage we only warn if target is non-null and not in the module's targets list. This means in practice that we warn for desktop-only modules loaded on mobile, but not vice-versa. This is intentional per T140675, T127268, and how the target filter has always worked.

Another problem I noticed is that MF uses the beforepagedisplay hook to set the target... And the gadgets are added from beforepagedisplay as well. That means that this targets patch can fail because the gadget hook is executed before the mobilefrontend.

Aye, OutputPage's BeforePageDisplay hook seems like a much too late point for MobileFrontend to change the target. It should be set much earlier.

Krinkle changed the task status from Open to Stalled.Aug 6 2018, 1:07 AM

Aye, OutputPage's BeforePageDisplay hook seems like a much too late point for MobileFrontend to change the target. It should be set much earlier.

I marked this task as stalled and mentioned on what, but forgot to file a task for it. Sorry about that. Filed now, at T213242.

Krinkle changed the task status from Stalled to Open.Sep 29 2019, 8:02 PM
Krinkle edited projects, added Readers-Web-Backlog; removed Editing-team.
Krinkle added a project: Patch-For-Review.

Change 449005 had a related patch set uploaded (by Krinkle; owner: TheDJ):
[mediawiki/extensions/Gadgets@master] Only queue gadget module if relevant on the curent target

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

Patch is now ready for testing and review.

Change 449005 merged by jenkins-bot:
[mediawiki/extensions/Gadgets@master] Only queue gadget module if relevant on the current target

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

pmiazga claimed this task.Oct 9 2019, 5:04 PM

Looks good, on Monday I'll check couple things and then I'll resolve it.

pmiazga closed this task as Resolved.Oct 17 2019, 4:26 PM