Page MenuHomePhabricator

[For visibility] Page previews should be externally extendable
Closed, ResolvedPublic1 Estimated Story Points

Description

Currently the only way to add support for a custom type is to add code to the Popups extension (e.g. https://gerrit.wikimedia.org/r/#/c/mediawiki/extensions/Popups/+/527547/).

Page previews should provide a generic API and a way to register additional code (e.g. https://www.mediawiki.org/wiki/Manual:Extension_registration#Attributes) so that extensions can host their own code.

QA steps

  1. Visit https://en.wikipedia.beta.wmflabs.org/wiki/Dog and hover over links. Preview should display (don't worry if the preview itself says "There was an issue..."
  2. Hover over a reference link e.g. [8] and check a preview shows with information on reference.
  3. Click the cog on one of the previews and disable "Enable reference previews" or "Enable previews". both previews should no longer show.

Related Objects

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

@Jdlrobson I have implemented a first rough draft along the lines of your proposal from above:

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Math/+/710329

I have some questions:

  1. Is there an interface for a gateway class that one can implement?

https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Math/+/710329/7/modules/ext.math.popup.js#50

In terms of testing, it would be good to make sure that the gateway-triple matches its desired signature.

  1. I have been using the new MW REST API which is almost identical to the RESTbase API. I am expecting config.endpoint to be the location of the rest.php endpoint in that particular wiki. I have no idea how this can be implemented.
  1. I realized that there is a UI bottom to disable popups (I think if you click on the settings wheel of the page preview popup). I think we need a hook to pass the message strings of the name of the math extension popup.
  1. Regarding step two: I do not understand how module.exports works outside node. I (ab)used the mw global instead.

... I have the impression we are at the beginning of a interface definition, and more questions will occur during the process. However, we now have something to start with.

I'm sorry @Physikerwelt I missed this last message because of a vacation. Will get back to you shortly. Sorry for the delay here.

Is there an interface for a gateway class that one can implement?

There's no interface right now.

In terms of testing, it would be good to make sure that the gateway-triple matches its desired signature.

We could realize this by creating a test in Special:JavaScript/qunit that tests the integration between page previews and Math extension. If this test fails we could block Page previews merges. That should get the desired result.

I have been using the new MW REST API which is almost identical to the RESTbase API. I am expecting config.endpoint to be the location of the rest.php endpoint in that particular wiki. I have no idea how this can be implemented.

I'm not too familiar with this. Hopefully @Pchelolo will be able to help?

I realized that there is a UI bottom to disable popups (I think if you click on the settings wheel of the page preview popup). I think we need a hook to pass the message strings of the name of the math extension popup.

Just to check I understand, you want to make it possible to disable Math previews in this dialog:

Screen Shot 2021-09-08 at 9.17.49 AM.png (302×786 px, 35 KB)

I think that's possible, but I'd suggest we do that in a second phase and focus on getting the minimum viable product deployed. For now I think it's fine to tie whether Math popups are on/off to the existing checkbox.

Regarding step two: I do not understand how module.exports works outside node. I (ab)used the mw global instead.

Using mw.popups is the correct solution here and will be better supported.

Is there an interface for a gateway class that one can implement?

There's no interface right now.

OK. Then this is done.

In terms of testing, it would be good to make sure that the gateway-triple matches its desired signature.

We could realize this by creating a test in Special:JavaScript/qunit that tests the integration between page previews and Math extension. If this test fails we could block Page previews merges. That should get the desired result.

Okay. Should this be done as a follow-up task or a preparation task?

I have been using the new MW REST API which is almost identical to the RESTbase API. I am expecting config.endpoint to be the location of the rest.php endpoint in that particular wiki. I have no idea how this can be implemented.

I'm not too familiar with this. Hopefully @Pchelolo will be able to help?

Ah I found that for the action api. We can start with that... I'll create a subtask to find a better solution. T290710

I realized that there is a UI bottom to disable popups (I think if you click on the settings wheel of the page preview popup). I think we need a hook to pass the message strings of the name of the math extension popup.

Just to check I understand, you want to make it possible to disable Math previews in this dialog:

Screen Shot 2021-09-08 at 9.17.49 AM.png (302×786 px, 35 KB)

I think that's possible, but I'd suggest we do that in a second phase and focus on getting the minimum viable product deployed. For now I think it's fine to tie whether Math popups are on/off to the existing checkbox.

To which one References or Links?

Regarding step two: I do not understand how module.exports works outside node. I (ab)used the mw global instead.

Using mw.popups is the correct solution here and will be better supported.

Ok this is resolved then too.

Jdlrobson added a subscriber: ovasileva.

@Jdlrobson Sorry for the delay. I finally find time to focus on this issue and have taken over Moritz's progress so far. I am keen to work on this now until its done.

Please have a short look on our progress so far, especially here: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Math/+/710329/10/modules/ext.math.popup.js

I think this should finish up our changes in the Math extension for now and finish suggested Step 1. The API exist and the Math extension is more or less ready to provide the popup extension with all necessary information.
The only issue I see, is the missing gateway you suggested.

popups.registerPreview( '#mw-content-text .mwe-math-element[data-qid]', function (ev, popupOptions) {
                            popups.requestPreview( '#math_<qid>', 'MATH', GATEWAY, popupOptions ); } );
    } )

Could you please explain how I should build this gateway so that we can use it on the popup extension side? If I understand your intention right, this gateway object should be our (in the Math extension) API endpoint, right?
The first time I implemented the gateway in the popup extension, I was using the preview/model.js to create a model. However, building the model is no longer available in the Math extension (since its part of the popup extension). So I am not sure how I should make this gateway so that it is of use for you.

Here is the first code I was using when created the gateway directly in the popup extension. As you can see, the fetchPreviewForTitle is the issue, because it uses convertPageToModel which in turn relies on the preview/model.js in the popup extension.

	function createMathGateway( config ) {
		var gatewayConfig = $.extend( {}, {
			language: config.get( 'wgPageContentLanguage' )
		} );
		return createMathApiGateway( $.ajax, gatewayConfig );
	}

	function createMathApiGateway( ajax, config ) {
		function fetch( qid ) {
			var endpoint = config.endpoint;

			return ajax( {
				url: endpoint + encodeURIComponent( qid ),
				headers: {
					Accept: 'application/json; charset=utf-8"',
					'Accept-Language': config.acceptLanguage
				}
			} );
		}

		function fetchPreviewForTitle( title, el ) {
			var qid = el.getAttribute( 'data-qid' );
			var xhr = fetch( qid );
			return xhr.then( function ( page ) {
				return convertPageToModel( page );
			} ).promise( {
				abort: function () {
					xhr.abort();
				}
			} );
		}

		function convertPageToModel( page ) {
			return createModel(
				page.title,
				page.canonicalurl,
				page.pagelanguagehtmlcode,
				page.pagelanguagedir,
				page.extract,
				page.type,
				page.thumbnail,
				page.pageid
			);
		}

		var newVar = {
			fetch: fetch,
			fetchPreviewForTitle: fetchPreviewForTitle,
			convertPageToModel: convertPageToModel
		};
		return newVar;
	}

I should have some bandwidth next week to boot up my page previews and try and work out what this would look like on the page previews side! Hopefully, then this can start falling into place.

Change 813347 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/Popups@master] [POC] Extensible previews

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

I spent the afternoon looking into this. I'm not 100% happy with the resulting architecture, but it helped me think through a few things and definitely seems feasible. I'd be interested in reviews from @thiemowmde and @phuedx who have the most familiarity with how page previews currently works to help guide us in the right direction there.

A few notes:

  • I underestimated the work here. The resulting architecture this would lead to, seems beneficial, as it cleans up a few things, but likely needs a bit more thought since we're expecting to expose this to gadgets and extensions.
  • Page previews was designed to preview pages. In this use case that concept is being broken somewhat, as Math's equations do not have associated links that you can click through. I'm not sure how to get round that, so would suggest that all your equations link somewhere - even if it's just a wikidata page or a Help page. A hard requirement that I don't see an easy way round without even more significant refactoring is tying the element that generates the page preview with a page title. This is needed to help distinguish different links
  • Math currently uses span elements. That could pose problems in capturing hovers on the right element. I found I couldn't capture hovers on span.mwe-math-element but could on span.mwe-math-element a (since the link fills up the entire span)

Here's a working example:
https://gist.github.com/jdlrobson/d555904f1739561a7e614d882acc0712
{F35315622}

Feel free to experiment with https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Popups/+/813347 on the Math side and determine whether this approach could work out and let me know of any other issues you run into.

Hope this is helpful!

Thank you.

A few notes:

  • Page previews was designed to preview pages. In this use case that concept is being broken somewhat, as Math's equations do not have associated links that you can click through. I'm not sure how to get round that, so would suggest that all your equations link somewhere - even if it's just a wikidata page or a Help page. A hard requirement that I don't see an easy way round without even more significant refactoring is tying the element that generates the page preview with a page title. This is needed to help distinguish different links

We currently link to a special page such as https://en.wikipedia.org/w/index.php?title=Special:MathWikibase&qid=Q35875

  • Math currently uses span elements. That could pose problems in capturing hovers on the right element. I found I couldn't capture hovers on span.mwe-math-element but could on span.mwe-math-element a (since the link fills up the entire span)

It might use span or div elements. See

https://www.mediawiki.org/wiki/Extension:Math/Popups

for a demo.

It depends on the rendering mode inline vs displaystyle. In the future, beginning from next year, when Chrome eventually supports MathML we would hope to be able to show the popup for parts of the equation. That would be mrow elements.

@Jdlrobson I implemented everything on our side and it looks good so far:

Emc-Screenshot.jpeg (720×1 px, 146 KB)
Jacobi-Screenshot.jpeg (720×1 px, 158 KB)

I still need to implement some tests and I haven't been able to push my latest update but I will do that presumably over the weekend.

Now, you tagged your commit https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Popups/+/813347 as POC. What are the chances to get this live and how long do we need to wait for this?

Change 710329 had a related patch set uploaded (by AndreG-P; author: AndreG-P):

[mediawiki/extensions/Math@master] REST API endpoint for popups

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

Nice! Great feature!

Now, you tagged your commit https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Popups/+/813347 as POC. What are the chances to get this live and how long do we need to wait for this?

I'm not sure right now. I would ideally like to talk to @phuedx and @thiemowmde who have a lot of knowledge and understanding of this extension but they are in different teams from me. Hopefully I'll be able to get a clearer picture within the next week. I'll keep you updated!

I think that this is a great start! I think the mw.popups.register function is neato. I am particularly pleased by Popups dogfooding it and how easy it was for @Andreg-p to use it in https://gerrit.wikimedia.org/r/710329.

Now, you tagged your commit https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Popups/+/813347 as POC. What are the chances to get this live and how long do we need to wait for this?

I think we need to make a decision as to whether the user should be able to disable types of previews individually. If we do decide this, then we should consider adding a mechanism for registering types of previews on the server-side as logged-in users disable previews via Special:Preferences. We could use extension attributes as suggested in the task description:

mediawiki/extensions/Math/extension.json
{
  "Popups": {
    "PreviewTypes": {
      "Math": {
        "name": "math",
        "namemsg": "popups-math",
        "descriptionmsg": "popups-math-description",
        "module": "ext.math.popups"
      }
    }
  }
}

Would register a preview type with:

  • An internal name of math
  • An external name defined in the popups-math message
  • An description defined in the popups-math-description message
  • An associated definition (i.e. the mw.popups.register() call) in the ext.math.popups module

Where the external name and description will be shown to the user.

Thank you. I support this.

I think we need to make a decision as to whether the user should be able to disable types of previews individually. If we do decide this, then we should consider adding a mechanism for registering types of previews on the server-side as logged-in users disable previews via Special:Preferences. We could use extension attributes as suggested in the task description:

Just as an alternative, this could also be handled within the math settings section. This would also allow for individual adjustments. For example, the popup on <refs could have different options for formatting the references. It might be a good practice to place this option next to the option if one wants to enable or disable the popup altogether.

@phuedx Considering the popup extension will be opened up for potentially more types of popups, a more selective enable/disable mechanism might be the right choice. But from our Math perspective, it does not make much of a difference.

The suggested extension attributes look also good to me. Can you or @Jdlrobson realize this so that we can bring the math popups to live?

Can you or @Jdlrobson realize this so that we can bring the math popups to live?

Hi! I've just got back from vacation. I should have more availability in the latter half of August but ideally September. Right now, unfortunately wrapping up mediawiki.org/wiki/Desktop_improvements is stealing most of my time! I will circle back when I can. Thank you for your patience!

Thank you. I think sooner is better than later. If we can complete the entire thing it before Sep 11th, that would be awesome. I hope that we will get the basic API part from the Math part https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Math/+/710329 merged soon.

Change 710329 merged by jenkins-bot:

[mediawiki/extensions/Math@master] REST API endpoint for popups

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

I've reached out to @ovasileva today to find some time to focus on this. We're currently wrapping up the desktop improvements project and have a few absences so we'll struggling to find a reviewer to help push this forward. She will chat to engineering directors.

@ovasileva @Jdlrobson, thank you. Is there an update on that? For us, the code already on gerrit https://gerrit.wikimedia.org/r/c/813347 is good enough to unblock our progress. We are also happy to refactor this within a short period if a more mature solution is available.

Change 832710 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/extensions/Popups@master] Use new Popups API

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

@Jdlrobson looking at https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Popups/+/813347 gives the impression that tests are missing. Do you need help creating tests?

I am hoping to get time with @phuedx on Friday to finish this up.

Change 832710 abandoned by Jdlrobson:

[mediawiki/extensions/Popups@master] Use new Popups API

Reason:

Squashed into https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Popups/+/813347/8

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

@thiemowmde we (@phuedx and I) are planning to land this patch on 17th October. Would appreciate if you someone else who worked on references previews. In particular, we are a bit curious about the logic for reference links without a hash fragment.

@Physikerwelt hope this timeline works for you. Apologies this is dragging out longer than expected.

After talking to Sam I am going to make a few modifications this week. I was planning to do those today, but due to some personal circumstances am a bit behind. I'm super sorry for the delay here I know this has already taken much longer than necessary. Hoping to take a look tomorrow and appreciate your patience on this matter.

Jdlrobson renamed this task from Page previews should be externally extendable to [Interrupt] Page previews should be externally extendable.Oct 21 2022, 4:46 PM
Jdlrobson assigned this task to phuedx.
Jdlrobson renamed this task from [Interrupt] Page previews should be externally extendable to [For visibility] Page previews should be externally extendable.Oct 21 2022, 4:59 PM
Jdlrobson updated Other Assignee, added: Jdlrobson.
Jdlrobson set the point value for this task to 1.

A brief update:

@Jdlrobson and I have spoken and my concerns have very nearly all been addressed. I want to think through whether there's a way to not expose mw.popups.register() to everyone and only extension authors and I also need to test the patch end-to-end.

I'm not sure that's a good end state anyway. It would be nice to be able to rewrite the popups gadget to use the same core as page previews.

@Jdlrobson I somehow lost the overview here. Who needs to make the next turn? (I hope it is not me;-)

I'm not sure that's a good end state anyway. It would be nice to be able to rewrite the popups gadget to use the same core as page previews.

Neato. However, there are a couple of issues that would stop you from achieving it right away:

  1. Page Previews detects whether the user has that gadget enabled and disables itself – this behaviour can be removed when the gadget is being rewritten
  2. There is not method to deregister or override a registered preview type. The gadget conflicts with Page Previews enough that we added the behaviour described in #1

@Jdlrobson: Is supporting the goal of rewriting the gadget be considered in scope? If so, then we should revisit my comments about restricting access to the mw.popups#register() method.

I'm not sure that's a good end state anyway. It would be nice to be able to rewrite the popups gadget to use the same core as page previews.

Neato. However, there are a couple of issues that would stop you from achieving it right away [snip]

Yes, I made the comment solely because I didn't want to see a technical decision made that made that objective harder. :)

@Jdlrobson: Is supporting the goal of rewriting the gadget be considered in scope? If so, then we should revisit my comments about restricting access to the mw.popups#register() method.

Not right now (for the reasons given) but I've intentionally written the code in such a way that we could expose it at a future date with minimum technical effort (the API is exposed public by default, and then made private by Page previews).

Right now page previews maintainers don't have the bandwidth to support the implications of allowing gadgets access to this API. Supporting gadgets would increase the scope of this work considerably, and it's not clear to me if we did that, whether there would even be adoption from the gadget side. Right now our main priority is supporting the Math extension developers.

@Jdlrobson @phuedx For now, it is not our goal to be able to rewrite the gadget. In fact, we just want to utilize the popups as they are right now and show additional information about a mathematical expression (similar to the previews for links). In the far future, it might be interesting to have more possibilities in order to make finer, task-specific adjustments on the popups but that's all out of scope for now. In fact, the little WIP demo by @Jdlrobson entirely fits our needs already.

Hence, I vote for not overcomplicating things in this early stage. But that's of course my very biased opinion :)

@Physikerwelt I haven't. Thanks for pointing that out. I will check if that change works tomorrow.

@Jdlrobson I updated our patch according to your changes and was able to make it work again (based on your patchset 20).

Unfortunately, I was not able to make it work with your suggestions in: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Math/+/845640
When I use your changes, I got a weird error on initialization:

Exception in module-execute in module ext.math.popup: load.php:1:864
TypeError: mw.Rest is not a constructor
    js jQuery
    runScript http://localhost:8080/w/load.php?lang=en&modules=startup&only=scripts&raw=1&skin=vector:12
    execute http://localhost:8080/w/load.php?lang=en&modules=startup&only=scripts&raw=1&skin=vector:13
    doPropagation http://localhost:8080/w/load.php?lang=en&modules=startup&only=scripts&raw=1&skin=vector:6
    setAndPropagate http://localhost:8080/w/load.php?lang=en&modules=startup&only=scripts&raw=1&skin=vector:7
    implement http://localhost:8080/w/load.php?lang=en&modules=startup&only=scripts&raw=1&skin=vector:18
    <anonymous> jQuery

I am unable to find the cause of this rather weird error. After analyzing your patches, I simply changed the mw.popups.register method to your new model structure. See also: https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Math/+/818294/9..10

mw.popups.register( {
	type: previewType,
	selector: '.mwe-math-element[data-qid] img',
	gateway: {
		fetch,
		fetchPreviewForTitle
	}}
);

And this worked.

Your suggested module.exports and changes in extension.json, however, lead to the error above. I have still struggle understanding the entirety of extension.json. but I believe, something is missing here that our extension requires. @Physikerwelt do you maybe know what causes this issue? Is there a different (new way) to register an mw.Rest depenedency maybe?

I suspect that using mw.popups.register, as I do right now, is not really what @Jdlrobson wanted since I directly register our popups and the extension.json still contains the ext.popups.main dependency.

I left you a note on your patch.

@Jdlrobson Thank you very much. That was the missing piece. From our site, everything is running again. Following your changes, it seems we are close to an end on your site too?

Change 813347 merged by jenkins-bot:

[mediawiki/extensions/Popups@master] Extensible previews

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

Test Result - Beta

Status: ✅ PASS
Environment: beta
OS: macOS Ventura
Browser: Chrome
Device: MBP
Emulated Device:NA

Test Artifact(s):

QA Steps

Visit https://en.wikipedia.beta.wmflabs.org/wiki/Dog and hover over links. Preview should display (don't worry if the preview itself says "There was an issue..."
Hover over a reference link e.g. [8] and check a preview shows with information on reference.
✅ AC1: Click the cog on one of the previews and disable "Enable reference previews" or "Enable previews". both previews should no longer show.

Screen Recording 2022-12-06 at 7.10.10 PM.mov.gif (746×1 px, 3 MB)

Edtadros subscribed.

Test Result - Prod

Status:
Environment: enwiki
OS: macOS Ventura
Browser: Chrome
Device: MBP
Emulated Device:NA

Test Artifact(s):

QA Steps

Visit https://en.wikipedia.beta.wmflabs.org/wiki/Dog and hover over links. Preview should display (don't worry if the preview itself says "There was an issue..."
Hover over a reference link e.g. [8] and check a preview shows with information on reference.
❌ AC1: Click the cog on one of the previews and disable "Enable reference previews" or "Enable previews". both previews should no longer show.
The previews appear, but there is only a cog on the page preview, not the reference preview. Additionally, when clicking the cog, it takes me to the preferences not to an overlay/dialog where I can select the option.

@Edtadros it looks like it's working for me.. Perhaps we need a quick 10 minutes together?

Setting work as expected, resolving.

Can this feature be utilized by gadgets? The public API seems incompatible with gadgets and mw.popups.register is not available publicly.

No, this feature is currently not a stable API. It needs significant work to be useful for gadgets.