Page MenuHomePhabricator

Allow skins to override mediawiki.page.ready initialisation to enable search JavaScript to be swapped
Closed, ResolvedPublic5 Estimated Story Points

Description

The module mediawiki.page.ready makes certain assumptions on behalf of a skin.
It enables "collapsible" elements, sortable tables and progressively enhances search.

Given the new Vector will use a new search implementation in Vue and given Minerva disables this module to workaround design challenges with sortable tables and collapsible elements (T111565 and T233340) this setup should be a skinScript that a skin can override.

This would unblock T233676 as well as integrating new search into Vue.js

Acceptance criteria

  • It's possible for Vector to replace the module loaded for search
  • It's possible for Minerva to load mediawiki.page.ready with sortable tables and collapsible disabled

Developer notes

After talking with @Catrope mw.config can be used to achieve the desired effect and use MakeGlobalVariablesScript to override those values or following the example of the hook ResourceLoaderJqueryMsgModuleMagicWord. In the latter example we may want to create a generalised hook and deprecate ResourceLoaderJqueryMsgModuleMagicWord to use it.

Proposed implementation

  • mediawiki.page.ready includes a configuration JSON config.json that is virtually imported. See module mediawiki.jqueryMsg for an example.
  • The JSON has a key search-module which defaults to mediawiki.searchSuggest.
  • The file resources/src/mediawiki.page.ready/ready.js requires the virtual config (require( './config.json') ) and passes the value of search-module to mw.loader replacing the line mw.loader.load( 'mediawiki.searchSuggest' );
  • A hook is executed ResourceLoaderPageReadyConfig (see ResourceLoaderJqueryMsgModuleMagicWord for an example) that allows skins/extensions to update the configuration values.

Minerva work

Once the above is done, work is needed in preparation for T257265:

  • The JSON should have a key collapsible which defaults to true.
  • The JSON should have a key sortable which defaults to true.
  • The code mw.hook( 'wikipage.content' ) should consult these values and only load and execute sortable/collapsible code if the configuration is true
  • Both variables can be modifed by the new hook ResourceLoaderPageReadyConfig

Sign off steps

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes
Jdlrobson added a subscriber: Krinkle.EditedMay 6 2020, 9:19 PM

@Catrope advises that we check in with @Krinkle before merging any solution relating to packageFiles here. If Timo hasn't got the time to input we should push ahead with the MakeGlobalVariablesScript approach for now. Both approaches would unblock Minerva's adoption of mediawiki.page.ready and Timo on T111565#5912584 :)

@Catrope advises that we check in with @Krinkle before merging any solution relating to packageFiles here. If Timo hasn't got the time to input we should push ahead with the MakeGlobalVariablesScript approach for now. Both approaches would unblock Minerva's adoption of mediawiki.page.ready and Timo on T111565#5912584 :)

  • If using packageFiles, its virtual definition form can allow you to effectively omit or replace it based on any RLContext factor (such as skin). That could work. This can also vary by a site Config variable. Much like what you described as the "alternate" plan, but done server-side.
  • If using skinScripts, we'll need to add support for that in the "new" model of skins being outside of core (e.g. a $wgResourceModuleSkinScripts, and for convenience also an attribute in skin.json to require no PHP coding, but for an MVP that's optional). : T250853.
  • Yet another way could be to introduce a temporary configuration variable, and let the mediawiki.page.ready definition vary based on that.
ovasileva set the point value for this task to 5.May 19 2020, 5:22 PM
Krinkle edited projects, added Performance-Team (Radar); removed Performance-Team.
Krinkle moved this task from Limbo to Watching on the Performance-Team (Radar) board.
Jdlrobson updated the task description. (Show Details)Jul 6 2020, 10:12 PM

Change 610752 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[mediawiki/core@master] Allow skins to override mediawiki.page.ready initialisation

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

Peter.ovchyn updated the task description. (Show Details)Jul 9 2020, 11:14 AM
Peter.ovchyn updated the task description. (Show Details)Jul 9 2020, 1:26 PM

Change 612569 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[mediawiki/skins/MinervaNeue@master] Disable sortable and collapsible during mediawiki.page.ready initialisation Add onSkinPageReadyConfig hook that overrides sortable and collapsible values.

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

Change 612570 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[mediawiki/skins/Vector@master] Replace searchModule to be used for search

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

Change 610752 had a related patch set uploaded (by Peter.ovchyn; owner: Peter.ovchyn):
[mediawiki/core@master] Allow skins to override mediawiki.page.ready initialisation

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

The collapsible/sortable options (unrelated to this task) LGTM. I do note that this is adding yet more temporary ad-hoc infrastructure and workarounds to support Minerva's exclusion of these core features (T111565). I don't see a good ROI on that, but I don't oppose it either.

The searchModule option is also fine if what we needed is a per-skin way to configure which module is lazy-loaded. Unfortunately, while that may be useful for third-parties in some cases, I don't see it helping Vector. It would require several compromises and added costs to make work which suggests to me we're going about this the wrong way, digging deeper into a path we should understand by now doesn't match what we were looking for going in.

The updated patch for Vector (link proposes exactly the compromises and costs that imho invalidates what we're trying to do by offering support for it in core. With this in place, core isn't supporting anything. It would imho literally be better not to add "support" in core in that case and do it directly Vector. There's an example below of what that could look like.

As I understand it:

  1. We need to continue as-is to not eagerly load search UI code upfront on page views.
  2. We need to continue loading "the right" UI code for search upon first interaction. This means that upon first interaction, the event handler fires and by the time that event returns, the right code is on its way to the user. Not two or three additional roundtrips away.
  3. We want skins besides Minerva/Vector to not require any changes. Let core keep providing the current default for them without any additional migration work.
  4. We want Minerva to effectively behave the same today, but we could afford a minor change to how it does that if it helps unlock New Vector's search.
  5. We want Legacy Vector to effectively behave the same as today, but we could afford a minor change to how it gets there if it helps unlock New Vector's search. This is also important as keeping the search as-is in Legacy Vector goes hand-in-hand with Vue Search actually being deployable (compatibility, data-driven decisions, A/B testing etc.)
  6. We want New Vector to load a Vue-based search UI instead of core's default. This will ship in Vector through 1 (one) additional non-default module.

The current approach from this patch is to make the search module name configurable per-skin via hook. That means Vector would have to override it in its entirety. The question then becomes, is it possible to point it at something and that is (or depends on) mediawiki.searchSuggest in legacy and is or depends-on vector.lazy-vue (name TBD) in modern mode – without introducing an additional delay through pointless client-side roundtrip? I don't see how.

The approach currently in the Vector patch is, I assume, not what we planned, but we got to afterward realizing it wouldn't work as we wanted. I think that should be a signal to look at the above again and come up with a simple uncompromising approach that "just" works.

With all the proposed complexity in place, I see only costs and no benefits. Perhaps there could be a benefit in re-using the three lines of code for "on focus, load a module". But at what cost? The configuration overhead we're adding, and the full client-server network+js parse+exec roundtrip delay, and additional module wrapper. This would be a net-less for page view perf (additional module, and additonal ready.js code), and a net-loss for Vector Search (both legacy and modern) due to delaying the code by a second or so due to having another roundtrip in the middle. Alltogether that added code is at least 5x the size of the three lines of code we're trying to avoid duplicating.

I'll detail a counter proposal, but note that this is not a recommendation. I'm hoping this demonstrates there's a way to do this easily and cheaply. Feel free to discard or modify to your liking if there's another way that meets the above points.

  1. mediawiki.config.ready (pre-existing page view module): Add a boolean config.search option, set to false for Vector.
  2. vector.js (pre-existing page view module): Inline conditional to determine searchModule for Legacy/Modern Vector. Then the three lines of code to on-focus -> load the module.
  3. Profit :)

Apologies for the lengthy comment, I did not have time to make it shorter and wanted to unblock you by EOD.

I'll detail a counter proposal, but note that this is not a recommendation.

I believe this is what I proposed in the 2nd proposal (https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/612570/13#message-5ce4e5cfc0ab74d1fc466a37c724dc31651941b0)

I showed how this would work in Vector but pointed out it would need a tweak to the core code.
https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/612570/11/resources/skins.vector.js/skin.js

If that's your preferred route, I will coordinate with @Peter.ovchyn to pull out the search module handling from this patch and provide a follow up.

Change 616905 had a related patch set uploaded (by Jdlrobson; owner: Jdlrobson):
[mediawiki/core@master] Allow skins to alter the behaviour of search

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

Change 610752 merged by jenkins-bot:
[mediawiki/core@master] Allow skins to override mediawiki.page.ready initialisation

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

Jdlrobson updated the task description. (Show Details)Jul 28 2020, 9:01 PM

Change 616905 merged by jenkins-bot:
[mediawiki/core@master] mediawiki.page.ready: Allow skins to disable search lazy load

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

Change 612569 merged by jenkins-bot:
[mediawiki/skins/MinervaNeue@master] Load mediawiki.page.ready on Minerva

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

phuedx claimed this task.Aug 3 2020, 5:15 PM
phuedx added a subscriber: Peter.ovchyn.

Change 618280 had a related patch set uploaded (by Jdrewniak; owner: Jdrewniak):
[mediawiki/core@master] Followup to 34c1661: mediawiki.page.ready: Allow skins to disable search lazy load

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

Change 618280 merged by jenkins-bot:
[mediawiki/core@master] Followup to 34c1661: mediawiki.page.ready: Allow skins to disable search lazy load

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

Can this be backported to 1.35? T233677 removed the ability to stop mediawiki.searchSuggest from being loaded and there is no alternatives to do so in 1.35.

Legoktm added a subscriber: Legoktm.

Can this be backported to 1.35? T233677 removed the ability to stop mediawiki.searchSuggest from being loaded and there is no alternatives to do so in 1.35.

Please cherry-pick the necessary patches to the REL1_35 branch and then we can review and merge them.

Change 619157 had a related patch set uploaded (by Alistair3149; owner: Peter.ovchyn):
[mediawiki/core@REL1_35] Allow skins to override mediawiki.page.ready initialisation

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

@Legoktm I cherry-picked the patches required and merged them into a single one.
I am not sure if it is the right way to do it so please let me know if any changes needed to be made.
https://gerrit.wikimedia.org/r/619157

@Legoktm I cherry-picked the patches required and merged them into a single one.
I am not sure if it is the right way to do it so please let me know if any changes needed to be made.
https://gerrit.wikimedia.org/r/619157

We prefer that commits are cherry-picked just as they were merged in master. If you could cherry-pick it as a stack of patches that would be ideal.

Or if you could list the specific commits that need backporting I can take care of it.

Change 619157 abandoned by Alistair3149:
[mediawiki/core@REL1_35] Cherry-pick: Allow skins to override mediawiki.page.ready initialisation

Reason:
Would be recreated as separate commits

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

Change 619614 had a related patch set uploaded (by Alistair3149; owner: Peter.ovchyn):
[mediawiki/core@REL1_35] Allow skins to override mediawiki.page.ready initialisation

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

@Legoktm I cherry-picked the patches required and merged them into a single one.
I am not sure if it is the right way to do it so please let me know if any changes needed to be made.
https://gerrit.wikimedia.org/r/619157

We prefer that commits are cherry-picked just as they were merged in master. If you could cherry-pick it as a stack of patches that would be ideal.

Or if you could list the specific commits that need backporting I can take care of it.

I cherry-picked the first commit but it seems that it doesn't allow me to cherry-pick the second one.
Would you mind to look into it? Here's the remaining two commits that needed to be backported:
https://gerrit.wikimedia.org/r/c/mediawiki/core/+/616905
https://gerrit.wikimedia.org/r/c/mediawiki/core/+/618280

Change 619157 restored by Reedy:
[mediawiki/core@REL1_35] Cherry-pick: Allow skins to override mediawiki.page.ready initialisation

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

Change 620897 had a related patch set uploaded (by Reedy; owner: Jdrewniak):
[mediawiki/core@REL1_35] Followup to 34c1661: mediawiki.page.ready: Allow skins to disable search lazy load

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

Reedy added a subscriber: Reedy.Aug 18 2020, 9:17 AM

@Legoktm I cherry-picked the patches required and merged them into a single one.
I am not sure if it is the right way to do it so please let me know if any changes needed to be made.
https://gerrit.wikimedia.org/r/619157

We prefer that commits are cherry-picked just as they were merged in master. If you could cherry-pick it as a stack of patches that would be ideal.

Or if you could list the specific commits that need backporting I can take care of it.

I cherry-picked the first commit but it seems that it doesn't allow me to cherry-pick the second one.
Would you mind to look into it? Here's the remaining two commits that needed to be backported:
https://gerrit.wikimedia.org/r/c/mediawiki/core/+/616905
https://gerrit.wikimedia.org/r/c/mediawiki/core/+/618280

I've just done those two ontop of https://gerrit.wikimedia.org/r/619157

remote: SUCCESS        
remote: 
remote:   https://gerrit.wikimedia.org/r/c/mediawiki/core/+/619157 mediawiki.page.ready: Allow skins to disable search lazy load        
remote:   https://gerrit.wikimedia.org/r/c/mediawiki/core/+/620897 Followup to 34c1661: mediawiki.page.ready: Allow skins to disable search ... [NEW]
phuedx updated the task description. (Show Details)Aug 19 2020, 3:38 PM
phuedx updated the task description. (Show Details)Aug 19 2020, 5:20 PM

Change 619614 merged by jenkins-bot:
[mediawiki/core@REL1_35] Allow skins to override mediawiki.page.ready initialisation

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

Change 619157 merged by jenkins-bot:
[mediawiki/core@REL1_35] mediawiki.page.ready: Allow skins to disable search lazy load

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

Change 620897 merged by jenkins-bot:
[mediawiki/core@REL1_35] Followup to 34c1661: mediawiki.page.ready: Allow skins to disable search lazy load

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

phuedx closed this task as Resolved.Aug 21 2020, 12:28 PM

Thanks all!

Change 637048 had a related patch set uploaded (by Jdlrobson; owner: Peter.ovchyn):
[mediawiki/skins/MinervaNeue@REL1_35] Load mediawiki.page.ready on Minerva

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

Change 637048 abandoned by Jdlrobson:
[mediawiki/skins/MinervaNeue@REL1_35] Load mediawiki.page.ready on Minerva

Reason:

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