Skip to content

Fix modal + form abuse #34921

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jul 1, 2025
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 14 additions & 14 deletions web_src/js/features/common-button.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,16 @@ export function initGlobalDeleteButton(): void {

fomanticQuery(modal).modal({
closable: false,
onApprove: async () => {
onApprove: () => {
// if `data-type="form"` exists, then submit the form by the selector provided by `data-form="..."`
if (btn.getAttribute('data-type') === 'form') {
const formSelector = btn.getAttribute('data-form');
const form = document.querySelector<HTMLFormElement>(formSelector);
if (!form) throw new Error(`no form named ${formSelector} found`);
modal.classList.add('is-loading'); // the form is not in the modal, so also add loading indicator to the modal
form.classList.add('is-loading');
form.submit();
return false; // prevent modal from closing automatically
}

// prepare an AJAX form by data attributes
Expand All @@ -62,12 +65,15 @@ export function initGlobalDeleteButton(): void {
postData.append('id', value);
}
}

const response = await POST(btn.getAttribute('data-url'), {data: postData});
if (response.ok) {
const data = await response.json();
window.location.href = data.redirect;
}
(async () => {
const response = await POST(btn.getAttribute('data-url'), {data: postData});
Copy link
Member

@delvh delvh Jul 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there a modal.classList.remove('is-loading') missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think no because :

  • when the request succeeds, the page gets reloaded
  • when the request fails, there is no message feedback, so we can't make the form re-submit.
    • this part of code is for the legacy "delete-button", there are already many problems in it, so I think no need to do more.

image

if (response.ok) {
const data = await response.json();
window.location.href = data.redirect;
}
})();
modal.classList.add('is-loading'); // the request is in progress, so also add loading indicator to the modal
return false; // prevent modal from closing automatically
},
}).modal('show');
});
Expand Down Expand Up @@ -158,13 +164,7 @@ function onShowModalClick(el: HTMLElement, e: MouseEvent) {
}
}

fomanticQuery(elModal).modal('setting', {
onApprove: () => {
// "form-fetch-action" can handle network errors gracefully,
// so keep the modal dialog to make users can re-submit the form if anything wrong happens.
if (elModal.querySelector('.form-fetch-action')) return false;
},
}).modal('show');
fomanticQuery(elModal).modal('show');
}

export function initGlobalButtons(): void {
Expand Down
1 change: 1 addition & 0 deletions web_src/js/features/comp/LabelEdit.ts
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,7 @@ export function initCompLabelEdit(pageSelector: string) {
return false;
}
submitFormFetchAction(form);
return false;
},
}).modal('show');
};
Expand Down
28 changes: 26 additions & 2 deletions web_src/js/modules/fomantic/modal.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,9 @@ const fomanticModalFn = $.fn.modal;
export function initAriaModalPatch() {
if ($.fn.modal === ariaModalFn) throw new Error('initAriaModalPatch could only be called once');
$.fn.modal = ariaModalFn;
$.fn.fomanticExt.onModalBeforeHidden = onModalBeforeHidden;
(ariaModalFn as FomanticInitFunction).settings = fomanticModalFn.settings;
$.fn.fomanticExt.onModalBeforeHidden = onModalBeforeHidden;
$.fn.modal.settings.onApprove = onModalApproveDefault;
}

// the patched `$.fn.modal` modal function
Expand All @@ -34,6 +35,29 @@ function ariaModalFn(this: any, ...args: Parameters<FomanticInitFunction>) {
function onModalBeforeHidden(this: any) {
const $modal = $(this);
const elModal = $modal[0];
queryElems(elModal, 'form', (form: HTMLFormElement) => form.reset());
hideToastsFrom(elModal.closest('.ui.dimmer') ?? document.body);

// reset the form after the modal is hidden, after other modal events and handlers (e.g. "onApprove", form submit)
setTimeout(() => {
queryElems(elModal, 'form', (form: HTMLFormElement) => form.reset());
}, 0);
}

function onModalApproveDefault(this: any) {
const $modal = $(this);
const selectors = $modal.modal('setting', 'selector');
const elModal = $modal[0];
const elApprove = elModal.querySelector(selectors.approve);
const elForm = elApprove?.closest('form');
if (!elForm) return true; // no form, just allow closing the modal

// "form-fetch-action" can handle network errors gracefully,
// so keep the modal dialog to make users can re-submit the form if anything wrong happens.
if (elForm.matches('.form-fetch-action')) return false;

// There is an abuse for the "modal" + "form" combination, the "Approve" button is a traditional form submit button in the form.
// Then "approve" and "submit" occur at the same time, the modal will be closed immediately before the form is submitted.
// So here we prevent the modal from closing automatically by returning false, add the "is-loading" class to the form element.
elForm.classList.add('is-loading');
return false;
}