Page MenuHomePhabricator

Page previews should be externally extendable
Open, LowestPublic

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.

Event Timeline

ovasileva triaged this task as Medium priority.Jun 9 2021, 3:39 PM
LGoto added a subscriber: LGoto.

This task was closed as part of backlog upkeep. If you believe it was closed in error, please respond on the ticket.

Jdlrobson added a subscriber: Jdlrobson.

We are unlikely to work on this task without a clear use case as it entails a lot of work by the core Page previews maintenance team on what should be extendable and how. My assumption was this was covered by T208758 which has far more detail, provides a clear use case.

I don't see much value in this particular ticket for achieving that as it doesn't add much value. What is the value of keeping it open?

Aklapper lowered the priority of this task from Medium to Lowest.Jul 20 2021, 2:16 PM

@Jdlrobson: @Andreg-p implemented a fully working version for the math extension in 2018 and are trying to figure out since then who feels responsible to allow us to display popups for math in the same way as it was done for references (which are also not blue). This is extremely frustrating. I am still hesitant to add another js library to display popups just because nobody feels responsible. And unlike WMDE (https://phabricator.wikimedia.org/T208758#6476942) we don't have a team that can push things forcefully forward.

@Physikerwelt I understand your frustration. Writing on a Saturday as a volunteer, I am also frustrated that I've not been able to find a way forward for you after so long. I think the challenge here is there are not enough design resources or engineering resources to deeply think about and architect the code required to support this use case from the WMF or WMDE side. In short, this is not going to happen without a push from your side.

I want to help you make that happen, and help you in a way that reduces the involvement of all the people that have so far been tagged. While I can't write the patches needed to resolve this task and your larger Math task, I can certainly guide you better and provide code review assistance.

From an engineering perspective, Andreg-p's patch changed how Popups worked in a significant way which made me and possibly others uncomfortable. Previously all previews were trigged by anchor tags (A) and the patch changed that to also apply to elements that were not anchor tags ('#mw-content-text .mwe-math-element[data-qid]'. It also adds an API that I am not familiar with.

I would not be comfortable maintaining such code, so for me a requirement is that this code lives inside the Math extensions.

With that in mind, In terms of the patch at https://gerrit.wikimedia.org/r/c/mediawiki/extensions/Popups/+/527547/2/src/index.js my suggestion of actionable steps here to get this done are as follows:

Step 1

Move all the code in Andre's patch to a Gerrit patch for the Math extension. To get you started, the Math extension should be responsible for wiring up the preview and triggering the preview like so:

popups.registerPreview( '#mw-content-text .mwe-math-element[data-qid]', function (ev, popupOptions) {
                            popups.requestPreview( '#math_<qid>' /* something unique for each math preview */, 'MATH' /*type */, gateway, popupOptions ); } );
    } )
  • The registerPreview or requestPreview methods obviously don't exist... yet.

I would note, this feature should have a feature flag so that it can be enabled/disabled easily in production wikis. We've done this for all the previews so far, and it's important given the scale of our projects that we have a switch we can use to turn it off in cases of API volume problems.

Step 2

To check the Math extension is wired up correctly, let's get ext.popups.main exporting a module.

module.exports = {
  registerPreview: function () { alert('Preview registered'); },
  requestPreview(: function () { alert('Click handler working'); }
};

Step 3

is the bit I'd come to help you with this. What we're looking for is the smallest patch I can help merge to possibly unblock you. You'll want to refactor the existing code to provide those wrapper functions above:

function  registerPreview( selector, handler ) {
             $( document )
                    .on( 'mouseover keyup', selector, function ( event ) {
                      ....
                       handler( event, {
                measures, generateToken, event
            } );
.                     .... on( 'mouseout blur
}

registerPreview( validLinkSelector, function ( ev, popupOptions ) {
            const mwTitle = titleFromElement( this, mw.config );
            if ( !mwTitle ) {
                return;
            }
            const type = getPreviewType( this, mw.config, mwTitle );
            let gateway;

            switch ( type ) {
                case previewTypes.TYPE_PAGE:
                    gateway = pagePreviewGateway;
                    break;
                case previewTypes.TYPE_REFERENCE:
                    gateway = referenceGateway;
                    break;
                default:
                    return;
            }
            requestPreview( title, type, gateway, popupOptions );
} );

module.exports = { registerPreview,  requestPreview };
// (or window.Popups if necessary)

Hopefully, at the end of this we'll have a patch that works as Andre implemented, that my team is happy to support.

Does that make sense? Any questions? Does that seem achievable?

@Jdlrobson thank you very much.

I think this is a great approach. However, the API between step 1/3 is not entirely clear to me. So step 3 would be code that lives inside the popups extension? Maybe we can clarify the API before we start with the implementation? So there might be an intermediate version between step 2/3 that just checked if the API is implemented correctly?

Moreover, I think before we start to introduce more JS code in the math extension codebase, we would want to setup https://www.mediawiki.org/wiki/Manual:JavaScript_unit_testing , would you agree?

Note, that there is also an a-ement for math, e.g., https://en.wikipedia.org/wiki/Schr%C3%B6dinger_equation

<span class="mwe-math-element" data-qid="Q165498">
<a href="/w/index.php?title=Special:MathWikibase&amp;qid=Q165498" style="color:inherit;"><span class="mwe-math-mathml-inline mwe-math-mathml-a11y" style="display: none;"><math xmlns="http://www.w3.org/1998/Math/MathML" alttext="{\displaystyle i\hbar {\frac {d}{dt}}\vert \Psi (t)\rangle ={\hat {H}}\vert \Psi (t)\rangle }">
  <semantics>
    <mrow class="MJX-TeXAtom-ORD">
      <mstyle scriptlevel="0" displaystyle="true">
        <mi>i</mi>
        <mi class="MJX-variant">ℏ<!-- ℏ --></mi>
        <mrow class="MJX-TeXAtom-ORD">
          <mfrac>
            <mi>d</mi>
            <mrow>
              <mi>d</mi>
              <mi>t</mi>
            </mrow>
          </mfrac>
        </mrow>
        <mo fence="false" stretchy="false">|</mo>
        <mi mathvariant="normal">Ψ<!-- Ψ --></mi>
        <mo stretchy="false">(</mo>
        <mi>t</mi>
        <mo stretchy="false">)</mo>
        <mo fence="false" stretchy="false">⟩<!-- ⟩ --></mo>
        <mo>=</mo>
        <mrow class="MJX-TeXAtom-ORD">
          <mrow class="MJX-TeXAtom-ORD">
            <mover>
              <mi>H</mi>
              <mo stretchy="false">^<!-- ^ --></mo>
            </mover>
          </mrow>
        </mrow>
        <mo fence="false" stretchy="false">|</mo>
        <mi mathvariant="normal">Ψ<!-- Ψ --></mi>
        <mo stretchy="false">(</mo>
        <mi>t</mi>
        <mo stretchy="false">)</mo>
        <mo fence="false" stretchy="false">⟩<!-- ⟩ --></mo>
      </mstyle>
    </mrow>
    <annotation encoding="application/x-tex">{\displaystyle i\hbar {\frac {d}{dt}}\vert \Psi (t)\rangle ={\hat {H}}\vert \Psi (t)\rangle }</annotation>
  </semantics>
</math></span><img src="https://wikimedia.org/api/rest_v1/media/math/render/svg/4e59451da64dff4be22db56d1adf9968e2a32d9c" class="mwe-math-fallback-image-inline" aria-hidden="true" style="vertical-align: -2.005ex; width:22.18ex; height:5.509ex;" alt="{\displaystyle i\hbar {\frac {d}{dt}}\vert \Psi (t)\rangle ={\hat {H}}\vert \Psi (t)\rangle }"></a></span>

so modifying the special page is also an option... maybe it can return a title element? I am just thinking about this with the hope to find a way that math is not more special than anything else, which will simplify maintenance in the future.

I think this is a great approach. However, the API between step 1/3 is not entirely clear to me. So step 3 would be code that lives inside the popups extension? Maybe we can clarify the API before we start with the implementation? So there might be an intermediate version between step 2/3 that just checked if the API is implemented correctly?

Yep it would be a bit chicken / egg in that way. We'd want to define a public function in page previews that supports extension for the Math use case. It would be informed by step 1. My expectation right now is that for page previews to be extendable it MUST export a function that allows registering a page previews, and it MAY export a function that allows rendering a page preview related to a certain element (to support use cases different from the current one e.g. that might be showing a page preview in somewhere we currently don't support e.g. special pages).

Moreover, I think before we start to introduce more JS code in the math extension codebase, we would want to setup https://www.mediawiki.org/wiki/Manual:JavaScript_unit_testing , would you agree?

I think unit testing in the Math codebase would definitely be a good idea to help you (and us) maintain the solution. Presumably any test in the Math extension should be run alongside Popups Gerrit patches to make sure nothing gets broken.

Note, that there is also an a-ement for math, e.g., https://en.wikipedia.org/wiki/Schr%C3%B6dinger_equation

Right. The reason I suggested a render page preview function is because of current expectations around links. For each eligible link https://github.com/wikimedia/mediawiki-extensions-Popups/blob/master/src/index.js#L201 we obtain a title that is used to define the page preview type. The current URL is not eligible in that sense because it does contain a hash fragment or point to a page that is valid for a preview. We'd need to think this through during step 3. I think trying to define the simplest API for extension is the problem we're trying to solve to get to this end result, and why this ticket stalled like it did.

@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.