Page MenuHomePhabricator

Changing mobile gadget preferences disables gadgets that don't support Minerva
Closed, ResolvedPublicBUG REPORT

Description

Steps to replicate the issue (include links if applicable):

What happens?:
On https://commons.wikimedia.beta.wmflabs.org/wiki/Special:Preferences#mw-prefsection-gadgets the test gadget is now disabled.

What should have happened instead?:
The test gadget should still be enabled. (even if it won't load on mobile, changing mobile gadget prefs shouldn't disable the test gadget for desktop)

Other information (browser name/version, screenshots, etc.):

https://commons.wikimedia.beta.wmflabs.org/wiki/MediaWiki:Gadgets-definition

T341421testgadget[ResourceLoader|skins=vector,vector-2022,monobook,timeless]|T341421testgadget.js

https://bn.wikipedia.org/wiki/%E0%A6%AE%E0%A6%BF%E0%A6%A1%E0%A6%BF%E0%A6%AF%E0%A6%BC%E0%A6%BE%E0%A6%89%E0%A6%87%E0%A6%95%E0%A6%BF:Gadgets-definition

https://en.wikipedia.org/wiki/User_talk:Alexis_Jazz#EditNoticeOnMobile

Edit: changed reproduction steps to use test gadget on beta cluster instead of Twinkle on bnwiki

Event Timeline

SD0001 subscribed.

It's because Twinkle's gadget definition excludes it from minerva skin. So any pref changes made in minerva skin will reset preferences of non-minerva gadgets to the default value.

This is certainly a bug, though I'm not sure if it always has been the case or there's been some regression in MediaWiki-Core-Preferences. In case of the former, any fix would be closely linked to T65532.

Removed |skins=vector,vector-2022,monobook,timeless,modern,cologne blue for now.

Removed |skins=vector,vector-2022,monobook,timeless,modern,cologne blue for now.

You could use targets=desktop instead which has nearly the same effect. Nearly because Twinkle would still load on Minerva desktop (but nobody uses that) and it wouldn't load on the mobile site with alternative skins (which isn't supported anyway) like https://bn.m.wikipedia.org/wiki/%E0%A6%AA%E0%A7%8D%E0%A6%B0%E0%A6%A7%E0%A6%BE%E0%A6%A8_%E0%A6%AA%E0%A6%BE%E0%A6%A4%E0%A6%BE?useskin=monobook.

AlexisJazz renamed this task from Changing mobile gadget preferences on bnwiki disables Twinkle to Changing mobile gadget preferences disables gadgets that don't support Minerva.Jul 10 2023, 1:18 AM
AlexisJazz updated the task description. (Show Details)

It's because Twinkle's gadget definition excludes it from minerva skin. So any pref changes made in minerva skin will reset preferences of non-minerva gadgets to the default value.

This is certainly a bug, though I'm not sure if it always has been the case or there's been some regression in MediaWiki-Core-Preferences. In case of the former, any fix would be closely linked to T65532.

Maybe related to the changes made in T328610 ? There it is actually giving the advice to exclude the Minerva skin.

Maybe related to the changes made in T328610 ?

Yes, I think so.

You could use targets=desktop instead which has nearly the same effect.

“Nearly the same effect” includes causing this bug since 8af75aa

Maybe if the gadget is unavailable due to the current skin/target, the option should be disabled rather than hidden? It would show some gadgets that are planned for non-default skins and thus irrelevant for most people, though.

Or don’t show all gadgets unavailable due to the current skin, but do show gadgets that are, while unavailable on the current skin, are available on the skin set in the preferences (which is Vector even when using the mobile domain), and show gadgets regardless of their target settings (the target setting should go away soon anyway).

Maybe related to the changes made in T328610 ?

Yes, I think so.

You could use targets=desktop instead which has nearly the same effect.

“Nearly the same effect” includes causing this bug since 8af75aa

Maybe if the gadget is unavailable due to the current skin/target, the option should be disabled rather than hidden? It would show some gadgets that are planned for non-default skins and thus irrelevant for most people, though.

Or don’t show all gadgets unavailable due to the current skin, but do show gadgets that are, while unavailable on the current skin, are available on the skin set in the preferences (which is Vector even when using the mobile domain), and show gadgets regardless of their target settings (the target setting should go away soon anyway).

Maybe a toggle to show/hide (default hide) gadgets that won't load in the current skin? So you could enable/disable a mobile-only gadget on desktop and vice versa. This could be useful if a user has trouble changing their preferences on either platform. It should be a bit more transparent as you can get the same list of gadgets on both platforms. @Jdlrobson, what do you think? (feel free to wait to reply until after your vacation)

I'm actually unsure how I feel about deprecating target as MobileFrontend can work with non-Minerva skins and Minerva can be used on desktop.

Jdlrobson added a subscriber: jsn.sherman.

@jsn.sherman could this issue relate to the changes you made for mobile view of preferences? It seems like saving on mobile resets certain preferences on desktop.

@jsn.sherman could this issue relate to the changes you made for mobile view of preferences? It seems like saving on mobile resets certain preferences on desktop.

I'll need to check; mobile preferences largely uses the same form as desktop preferences, just with a different wrapper. We do munge some checkboxes to display them as toggles, so I'll have a look there and report back.

@Jdlrobson
I think we can rule out the mobile.js code.

I commented out all of the checkbox mutation code from addFields( $descriptor ) in includes/specials/forms/PreferencesFormOOUI.php so that it just returns the original checkbox fields. I'm still seeing the same behavior locally, where I have two gadgets with one of them defined with [ResourceLoader|skins=vector,vector-2022,monobook,timeless] and one just [ResourceLoader]

It then occured to me that I was making things more complicated than necessary. Just defining a gadget with [ResourceLoader|skins=vector] and changing the skin preference between vector and any other skin demonstrates the same behavior in desktop. The absent checkbox is treated like an unchecked box.

Looking at the gadget extension code, it looks like the preference description for enabling the gadget is not defined when the skin isn't supported.
https://gerrit.wikimedia.org/r/plugins/gitiles/mediawiki/extensions/Gadgets/+/refs/heads/master/includes/Hooks.php#190

From my understanding of preferences, I believe the preference should be hidden rather than undefined. I'll test that locally and share a gadget patch if it solves the problem.

Change 955311 had a related patch set uploaded (by Jsn.sherman; author: Jsn.sherman):

[mediawiki/extensions/Gadgets@master] Preserve Gadget prefs when then can't be enabled

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

Change 955311 merged by jenkins-bot:

[mediawiki/extensions/Gadgets@master] Preserve Gadget prefs when they can't be enabled

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

Maybe a toggle to show/hide (default hide) gadgets that won't load in the current skin? So you could enable/disable a mobile-only gadget on desktop and vice versa. This could be useful if a user has trouble changing their preferences on either platform. It should be a bit more transparent as you can get the same list of gadgets on both platforms.

I think it’s a good idea and worth looking into after the above patch stops data loss.

I'm actually unsure how I feel about deprecating target as MobileFrontend can work with non-Minerva skins and Minerva can be used on desktop.

If a gadget doesn’t/shouldn’t work on mobile, it’s usually because of the Minerva Neue incompatibility, not the MobileFrontend incompatibility, so the skins= option is more appropriate than the targets= option.

Change 955792 had a related patch set uploaded (by Jdlrobson; author: Jsn.sherman):

[mediawiki/extensions/Gadgets@wmf/1.41.0-wmf.25] Preserve Gadget prefs when they can't be enabled

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

Change 955792 merged by jenkins-bot:

[mediawiki/extensions/Gadgets@wmf/1.41.0-wmf.25] Preserve Gadget prefs when they can't be enabled

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

Mentioned in SAL (#wikimedia-operations) [2023-09-07T20:45:22Z] <thcipriani@deploy1002> Started scap: Backport for [[gerrit:955792|Preserve Gadget prefs when they can't be enabled (T341421)]], [[gerrit:955791|Fix settings button not working on reference previews (T345829)]]

Mentioned in SAL (#wikimedia-operations) [2023-09-07T20:46:48Z] <thcipriani@deploy1002> jdlrobson and thcipriani: Backport for [[gerrit:955792|Preserve Gadget prefs when they can't be enabled (T341421)]], [[gerrit:955791|Fix settings button not working on reference previews (T345829)]] synced to the testservers mwdebug2001.codfw.wmnet, mwdebug1001.eqiad.wmnet, mwdebug2002.codfw.wmnet, mwdebug1002.eqiad.wmnet, and mw-debug kubernetes deployment (accessible via k8s-experimental XWD o

Mentioned in SAL (#wikimedia-operations) [2023-09-07T20:56:35Z] <thcipriani@deploy1002> Finished scap: Backport for [[gerrit:955792|Preserve Gadget prefs when they can't be enabled (T341421)]], [[gerrit:955791|Fix settings button not working on reference previews (T345829)]] (duration: 11m 12s)

Jdlrobson claimed this task.

Thanks for the patch here @jsn.sherman ! I've now backported the fix so this should be resolved. Please let me know if that's not the case.

Thanks for the patch here @jsn.sherman ! I've now backported the fix so this should be resolved. Please let me know if that's not the case.

No problem: thanks for looping me in and handling the backport!

I can't reproduce this anymore on betacommons so seems good to me. :-)