PageForms saves form only after two clicks on "Save page" button if VEforAll is used
Closed, ResolvedPublic
Actions

Assigned To

Authored By

	Urfiner
	Mar 26 2021, 5:04 PM

Description

PageForms saves form only after two clicks on "Save page" button if VEforAll is used

Probably caused by recent switch to OOUI. "'#wpSave" is now not button but span container, it has no onCLick event

Suggested fix (probably I use not the best way to select "button" element. Or even not the best place to fix the element selector because $( button ) is not button, it is span).

diff --git a/libs/PF_submit.js b/libs/PF_submit.js
index 600a96e1..036e258e 100644
--- a/libs/PF_submit.js
+++ b/libs/PF_submit.js
@@ -225,7 +225,7 @@
 									event.preventDefault();
 									mw.pageFormsActualizeVisualEditorFields( function () {
 										canSubmit = true;
-										$( button ).click();
+										$( button ).children().click();
 									} );
 								}
 							} );
-- 
2.18.1

Details

	Subject	Repo	Branch	Lines +/-
	Revert 7eca50a - get save working with VEForAll again	mediawiki/extensions/PageForms	master	+1 -1
	Fix for form saving with VE after OOUI addition	mediawiki/extensions/PageForms	master	+1 -1

Customize query in gerrit

Related Objects

Mentioned In: T298506: PageForms saves form only after two clicks on "Save page" button if VEforAll is used
Mentioned Here: T298506: PageForms saves form only after two clicks on "Save page" button if VEforAll is used

Event Timeline

Urfiner created this task.Mar 26 2021, 5:04 PM

Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMar 26 2021, 5:04 PM

Urfiner updated the task description. (Show Details)Mar 26 2021, 5:05 PM

Urfiner renamed this task from PageForms saves for only after two clicks on "Save page" button if VEforAll is used to PageForms saves form only after two clicks on "Save page" button if VEforAll is used.Mar 26 2021, 5:15 PM

Urfiner updated the task description. (Show Details)

Probably better to fix it here but I am still not sure whether it is ok to use .children()

libs/PF_submit.js
Line 218

-					var $formButtons = $( '#wpSave, #wpDiff' );
+					var $formButtons = $( '#wpSave, #wpDiff' ).children();

I admit that the current JS now looks a little strange, since it involves "clicking" on a span and not a button, but it still works fine for me - and I tested it in both Google Chrome and Opera. What browser are you using?

I am using chrome. My VeForAll is not the latest, maybe that is the reason

I'm assuming that this was an invalid bug, but feel free to re-open it if it's still an issue.

Reopening, because this is still an issue for me in Firefox 91.3.0 and Chrome 96.0.4664.45 with Page Forms and VEForAll.

I can confirm the initial analysis. The problem is that #wpSave and #wpDiff are <span> elements, but the actual submit <button> is a child of these spans. The user clicks on the button, and the event bubbles up to the #wpSave span, which stops the propagation initially to call pageFormsActualizeVisualEditorFields. In the callback, it runs $( button ).click(), but button is actually the span (instead of the submit <button>). This means the injected click event doesn't submit the form.

The fix is to trigger the click event on the <button>, for example:

$( button ).on( 'click', function ( event ) {
        if ( !canSubmit ) {
                event.preventDefault();
                mw.pageFormsActualizeVisualEditorFields( function () {
                        canSubmit = true;
                        $( button ).find("[type='submit']").click();    // <------
                } );
        }
} );

@M4rkusd89 - sorry for the delay. There are now three different suggested fixes in this bug report. Do you believe yours is the best of the three?

I found and fixed this issue in my local Wiki installation before I found this bug report. I added my version of the fix for completeness, without having any preference regarding which one is finally adopted. As long as it is fixed in whichever way you think is best, I am happy.

That being said, from a technical perspective, attaching and triggering the click event to the element with type=submit seems to me the best choice, because that is the element that submits the form as its default click behaviour.

Change 747925 had a related patch set uploaded (by Yaron Koren; author: Yaron Koren):

[mediawiki/extensions/PageForms@master] Fix for form saving with VE after OOUI addition

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

gerritbot added a project: Patch-For-Review.Dec 17 2021, 4:41 PM

Change 747925 merged by jenkins-bot:

[mediawiki/extensions/PageForms@master] Fix for form saving with VE after OOUI addition

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

Maintenance_bot removed a project: Patch-For-Review.Dec 17 2021, 5:10 PM

@Urfiner and @M4rkusd89 - thanks for your help with this. And sorry that it took so long to fix! Hopefully everything works fine now.

M4rkusd89 mentioned this in T298506: PageForms saves form only after two clicks on "Save page" button if VEforAll is used.Jan 4 2022, 9:42 AM

@Yaron_Koren It looks like I reopened this task by mistake. This was previously fixed in https://gerrit.wikimedia.org/r/c/mediawiki/extensions/PageForms/+/739566, but I used a slightly older Page Forms version that did not include this fix. Unfortunately this means the fix I proposed here breaks the Save button again. I have opened T298506 to revert the change. Apologies for the confusion!

Change 751194 had a related patch set uploaded (by Yaron Koren; author: Yaron Koren):

[mediawiki/extensions/PageForms@master] Revert 7eca50a

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

gerritbot added a project: Patch-For-Review.Jan 4 2022, 5:21 PM

Change 751194 merged by jenkins-bot:

[mediawiki/extensions/PageForms@master] Revert 7eca50a - get save working with VEForAll again