This would be an added feature and not a bug. It seems that edit recovery can implement a hook so other extensions can benefit from it

In T344928#9118397, @dmaza wrote:

This would be an added feature and not a bug. It seems that edit recovery can implement a hook so other extensions can benefit from it

I'm tending to disagree with this assessment based on some more digging, we appear to be saving field_wpFooterTextbox and field_wpHeaderTextbox which are both fields that are specific to the Page: namespace that are added by ProofreadPage. Maybe there is something ProofreadPage is doing that blocks this from being automatically recognized ?

I agree, all fields should be saved.

It looks like this might be an issue with PRP, where .prp-quality-widget is not infusable and so it's value can't be fetched. Edit Recovery is looking up the DOM to find the parent widget of an input element, and for the status radios it finds one but it's not infusable.

Should Edit Recovery handling this differently, or should we make the status radios into an infusable widget?

In T344928#9138939, @Samwilson wrote:

I agree, all fields should be saved.

It looks like this might be an issue with PRP, where .prp-quality-widget is not infusable and so it's value can't be fetched. Edit Recovery is looking up the DOM to find the parent widget of an input element, and for the status radios it finds one but it's not infusable.

Should Edit Recovery handling this differently, or should we make the status radios into an infusable widget?

I think making the status radios into a infusable OOUI widget is overdue :)

Edit recovery should probably fallback to doing something (rather than ignoring the field) if the field is not infusable (ex: if it is a raw textbox)

For form fields that are not OOUI widgets, it already works. For each field it looks to see if it's an OOUI input, and then searches for the closest parent widget:

if ( field.classList.contains( 'oo-ui-inputWidget-input' ) ) {
	try {
		inputFields[ field.name ] = OO.ui.infuse( field.closest( '.oo-ui-widget' ) );
	} catch ( e ) {
		// Ignore any non-infusable widget because we won't be able to set its value.
	}
} else {
	inputFields[ field.name ] = field;
}

This could probably look for a parent InputWidget rather than just any Widget, but yeah as you say it's a good idea to make a proper PageQualityInputWidget anyway.

Change 957381 had a related patch set uploaded (by Samwilson; author: Samwilson):

[mediawiki/extensions/ProofreadPage@master] Convert quality radio buttons to PageQualityInputWidget

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

The above patch is ready for review.

Change 957381 merged by jenkins-bot:

[mediawiki/extensions/ProofreadPage@master] Convert quality radio buttons to PageQualityInputWidget

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

The ProofreadPage widget is now infusable, but the way that Edit Recovery finds it is not working. This is because of the radio widgets inside the new PageQualityWidget, which are widgets inside a widget. The current logic is to take a set of form inputs, e.g.:

const $fields = $( 'textarea, select, input:not([type="hidden"], [type="submit"])' );

and for each of them see if it's an OOUI widget:

if ( $field.classList.contains( 'oo-ui-inputWidget-input' ) ) {
    var widget = OO.ui.infuse( field.closest( '.oo-ui-widget' ) );
}

This needs to go up another level of widget for the PageQualityWidget, but in a way that works for all OOUI widgets. The HTML structure is e.g.:

<div class="prp-pageQualityInputWidget oo-ui-widget oo-ui-widget-enabled oo-ui-inputWidget oo-ui-radioSelectInputWidget" id="ooui-php-7" data-ooui="">
    <input class="oo-ui-element-hidden oo-ui-inputWidget-input" name="wpQuality" value="4">
    <div class="oo-ui-widget oo-ui-widget-enabled oo-ui-selectWidget oo-ui-selectWidget-unpressed oo-ui-radioSelectWidget" role="radiogroup" tabindex="0" aria-activedescendant="ooui-4">
        <label class="oo-ui-widget oo-ui-widget-enabled oo-ui-optionWidget oo-ui-radioOptionWidget" aria-checked="false" tabindex="-1" role="radio">
            <span class="oo-ui-widget oo-ui-widget-enabled oo-ui-inputWidget oo-ui-radioInputWidget prp-quality-radio quality0">
                <input type="radio" tabindex="-1" class="oo-ui-inputWidget-input" value="0" role="presentation"><span></span>
            </span>
            <span class="oo-ui-labelElement-label"></span>
        </label>
       <label class="oo-ui-widget oo-ui-widget-enabled oo-ui-optionWidget oo-ui-radioOptionWidget" aria-checked="false" tabindex="-1" role="radio">
           <span class="oo-ui-widget oo-ui-widget-enabled oo-ui-inputWidget oo-ui-radioInputWidget prp-quality-radio quality1">
           /* …etc. */

Change 961982 had a related patch set uploaded (by Samwilson; author: Samwilson):

[mediawiki/core@master] Edit Recovery: try to infuse grantparent OOUI widgets

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

@Soda the issue seems to be that the ext.proofreadpage.page.edit module (which adds the mw.proofreadpage.PageQualityInputWidget class) loads after the mediawiki.editRecovery.edit module and so the new widget isn't available for infusing. Do you have an opinion on the best way it should be done?

Change 963852 had a related patch set uploaded (by Samwilson; author: Samwilson):

[mediawiki/extensions/ProofreadPage@master] Allow wikitext in PageQualityInputWidget's field label

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

Change 963859 had a related patch set uploaded (by Samwilson; author: Samwilson):

[mediawiki/extensions/ProofreadPage@master] Get quality level from current textbox

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

Change 963852 merged by jenkins-bot:

[mediawiki/extensions/ProofreadPage@master] Allow wikitext in PageQualityInputWidget's field label

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

Change 963859 merged by jenkins-bot:

[mediawiki/extensions/ProofreadPage@master] Get quality level from current textbox

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

Change 965616 had a related patch set uploaded (by Samwilson; author: Samwilson):

[mediawiki/core@master] Edit Recovery: Make extensions handle their own form fields

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

Change 965617 had a related patch set uploaded (by Samwilson; author: Samwilson):

[mediawiki/extensions/ProofreadPage@master] Use new Edit Recovery hook to add all three form fields

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

Change 961982 abandoned by Samwilson:

[mediawiki/core@master] Edit Recovery: try to infuse grantparent OOUI widgets

Reason:

A new hook is a better approach for this issue: I30591f9f5eb6fbf2a24043e021c1a70d0dbd2ffb

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

The JS hook seems fine, but would it make sense to offer a PHP hook in addition to or instead of? Maybe we could even look for an extension attribute https://www.mediawiki.org/wiki/Manual:Extension_registration#Attributes

Then you need only a ResourceLoader virtual JSON file to feed the config setting or attributes into edit.js.

This isn't about no-JS (as Edit Recovery is only for JS users), rather just a cleaner way to hook into the system. Not all extensions that add form elements will even have a JS module at all, so you'd have to add a module solely to make use of Edit Recovery, potentially increasing the startup module size. Using PHP meanwhile will ensure you have all the selectors you need regardless of the order in which JS modules are loaded.

That's a great idea, thanks! Will do.

I've been experimenting a bit more with this, and it doesn't seem simple. attributes is only for other extensions to be able to get config from each other; my understanding is that if core needs a similar thing then it just adds a top-level schema item to extension.json. Doing that feels a bit much for Edit Recovery (but maybe that's fine; I'm not sure).

But also, I'm not sure that a JSON config for this would be flexible enough to do what we want. I think what we're aiming for is for extensions to be able to provide a set of instances of HTMLInputElement or OO.ui.InputWidget… for example, for ProofreadPage;

inputFields.wpHeaderTextbox = document.getElementById( 'wpHeaderTextbox' );
inputFields.wpFooterTextbox = document.getElementById( 'wpFooterTextbox' );
mw.proofreadpage.PageQualityInputWidget = PageQualityInputWidget;
inputFields.wpQuality = OO.ui.infuse( $editForm.find( '.prp-pageQualityInputWidget' )[ 0 ] );

The third one is the most involved, because the module for that has to have been loaded before this.

I'll keep digging into this.