Page MenuHomePhabricator

[Regression] Not possible for logged in users to disable page previews
Closed, ResolvedPublic

Description

From QA in T219434

Expected: No page preview. It's disabled!
Actual: A page preview despite being disabled!

Event Timeline

ovasileva added a subscriber: ovasileva.

Is this related to recent changes to references preview?

This only seems to impact beta cluster not production (it works fine on English Wikipedia)

I can confirm this is an issue on Beta Cluster - I cannot reproduce that on production Reference Previews are enabled on German Wikipedia, but everything works there.

The user preferences are saved correctly. After hitting preferences save the popups key is set to 0, therefore preferences are disabled.
When I check the user preferences on the JS side I get "0".

console.log( mw.user.options.values.popups ) => "0"

But the ext.popups module is present and loaded

console.log( mw.loader.moduleRegistry["ext.popups"].state ) => "ready"

even it shouldn't be.

Code responsible for adding popups module to a pageview:
https://github.com/wikimedia/mediawiki-extensions-Popups/blob/e341ec8268cb5d70e2820d8af970efb41d8e2401/includes/PopupsContext.php#L126

I assume that something else is loading the ext.popups module on beta cluster.
A note that is worth taking - we do not ship any arePopupsEnabled config. If the "Popups" feature is not enabled, we do not ship the ext.popups module.

An additional datapoint: There haven't been any significant changes to the Popups codebase since 23rd August 2019 (see https://github.com/wikimedia/mediawiki-extensions-Popups/commits/master/)

ovasileva lowered the priority of this task from High to Medium.Sep 23 2019, 9:49 AM

Moved to "Blocked on others" as it would be more valuable for the team who created Reference Previews to look at it (as it may be related).

/cc @awight @MichaelSchoenitzer_WMDE

@Jdlrobson two minor questions about the context of this report:

  • Is this for a logged-in or anonymous user?
  • "Hover over link"—what type of link? Redlink, page link or reference footnote?

I think this is a real bug, just trying to understand what you're seeing. The shouldSendModuleToUser function linked above is expected deliver ext.Popups to any anonymous user, who will then see reference previews but not page previews. The situation is especially bad on the beta cluster because reference previews is enabled by default, but without the beta feature setting there's no way to disable our feature.

Happily, we're doing some work in the next few weeks which should resolve the issue by explicitly splitting the Page Previews and Reference Previews preferences: T233813: Separate user preferences for Page previews and Reference previews for Beta

@awight - I can see popups as a logged-in user who disabled Page Previews on the Preferences page. It happens when I hover a regular page link.

Reproduction steps:

Expected behavior: No popup is shown
Current behavior: The popup is visible.

@awight - I can see popups as a logged-in user who disabled Page Previews on the Preferences page. It happens when I hover a regular page link.

+1 thanks for the details, I see the same thing with my logged-in user. I have no clues yet, I've toggled my preference, hard-refreshed, and disabled extraneous gadgets.

We'll take a look after T233813, and hopefully we can provide browser tests for the matrix of preferences and configurations.

Jdlrobson added a subscriber: Seddon.

Okay mystery solved... I've worked this out:
The issue is this banner

It has a dependency for ext.popups - but doing that is bad - if the code is loaded, Popups will run regardless of the user preference.
cc @Seddon @AndyRussG

Instead of doing that it should check the state of Popups before pulling it in:

if ( mw.loader.getState( 'ext.popups' ) !== 'registered' ) { mw.loader.using( 'ext.popups')....

The issue is this banner

Great find!

I just want to add that, after T233813 it should be perfectly safe to load ext.popups to reuse its functionality, because both "page previews" and "reference previews" behaviors will be guarded by more specific configuration and options. But still not a best practice perhaps, since the minified code is > 40k.

Ping fundraising - is there anything we can do to help fix this. It's impacting our ability to test the feature confidently.

@Jdlrobson I made the edit you suggested to the banner - diff. Has that fixed it?

That did it! Thanks!

Wow!! Good work everybody! Special shout out to @Edtadros for the great find and @Jdlrobson for the great investigation!