Change form actions to fetch for submit review box (#25219)

Co-author: @wxiaoguang 

Close #25096 

The way to fix it in this PR is to change form submit to fetch using
formData, and add flags to avoid post repeatedly.
Should be able to apply to more forms that have the same issue after
this PR.

In the demo below, 'approve' is clicked several times, and then
'comment' is clicked several time after 'request changes' clicked.

After:


https://github.com/go-gitea/gitea/assets/17645053/beabeb1d-fe66-4b76-b048-4f022b4e83a0


Update: screenshots from /devtest

>
![image](https://user-images.githubusercontent.com/2114189/245680011-ee4231e0-a53d-4c2a-a9c2-71ccd98005cc.png)
> 
>
![image](https://user-images.githubusercontent.com/2114189/245680057-9215d348-63d8-406d-8828-17e171163aaa.png)
> 
>
![image](https://user-images.githubusercontent.com/2114189/245680148-89d7b3d1-d7b6-442f-b69e-eadaee112482.png)

---------

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
This commit is contained in:
HesterG 2023-06-14 16:01:37 +08:00 committed by GitHub
parent 6348823eab
commit a43ea22479
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
14 changed files with 208 additions and 36 deletions

View file

@ -132,6 +132,10 @@ func (b *Base) JSON(status int, content interface{}) {
} }
} }
func (b *Base) JSONRedirect(redirect string) {
b.JSON(http.StatusOK, map[string]any{"redirect": redirect})
}
// RemoteAddr returns the client machine ip address // RemoteAddr returns the client machine ip address
func (b *Base) RemoteAddr() string { func (b *Base) RemoteAddr() string {
return b.Req.RemoteAddr return b.Req.RemoteAddr

View file

@ -32,6 +32,16 @@ func List(ctx *context.Context) {
ctx.HTML(http.StatusOK, "devtest/list") ctx.HTML(http.StatusOK, "devtest/list")
} }
func FetchActionTest(ctx *context.Context) {
_ = ctx.Req.ParseForm()
ctx.Flash.Info(ctx.Req.Method + " " + ctx.Req.RequestURI + "<br>" +
"Form: " + ctx.Req.Form.Encode() + "<br>" +
"PostForm: " + ctx.Req.PostForm.Encode(),
)
time.Sleep(2 * time.Second)
ctx.JSONRedirect("")
}
func Tmpl(ctx *context.Context) { func Tmpl(ctx *context.Context) {
now := time.Now() now := time.Now()
ctx.Data["TimeNow"] = now ctx.Data["TimeNow"] = now

View file

@ -193,7 +193,7 @@ func SubmitReview(ctx *context.Context) {
} }
if ctx.HasError() { if ctx.HasError() {
ctx.Flash.Error(ctx.Data["ErrorMsg"].(string)) ctx.Flash.Error(ctx.Data["ErrorMsg"].(string))
ctx.Redirect(fmt.Sprintf("%s/pulls/%d/files", ctx.Repo.RepoLink, issue.Index)) ctx.JSONRedirect(fmt.Sprintf("%s/pulls/%d/files", ctx.Repo.RepoLink, issue.Index))
return return
} }
@ -214,7 +214,7 @@ func SubmitReview(ctx *context.Context) {
} }
ctx.Flash.Error(translated) ctx.Flash.Error(translated)
ctx.Redirect(fmt.Sprintf("%s/pulls/%d/files", ctx.Repo.RepoLink, issue.Index)) ctx.JSONRedirect(fmt.Sprintf("%s/pulls/%d/files", ctx.Repo.RepoLink, issue.Index))
return return
} }
} }
@ -228,14 +228,13 @@ func SubmitReview(ctx *context.Context) {
if err != nil { if err != nil {
if issues_model.IsContentEmptyErr(err) { if issues_model.IsContentEmptyErr(err) {
ctx.Flash.Error(ctx.Tr("repo.issues.review.content.empty")) ctx.Flash.Error(ctx.Tr("repo.issues.review.content.empty"))
ctx.Redirect(fmt.Sprintf("%s/pulls/%d/files", ctx.Repo.RepoLink, issue.Index)) ctx.JSONRedirect(fmt.Sprintf("%s/pulls/%d/files", ctx.Repo.RepoLink, issue.Index))
} else { } else {
ctx.ServerError("SubmitReview", err) ctx.ServerError("SubmitReview", err)
} }
return return
} }
ctx.JSONRedirect(fmt.Sprintf("%s/pulls/%d#%s", ctx.Repo.RepoLink, issue.Index, comm.HashTag()))
ctx.Redirect(fmt.Sprintf("%s/pulls/%d#%s", ctx.Repo.RepoLink, issue.Index, comm.HashTag()))
} }
// DismissReview dismissing stale review by repo admin // DismissReview dismissing stale review by repo admin

View file

@ -1411,6 +1411,7 @@ func registerRoutes(m *web.Route) {
if !setting.IsProd { if !setting.IsProd {
m.Any("/devtest", devtest.List) m.Any("/devtest", devtest.List)
m.Any("/devtest/fetch-action-test", devtest.FetchActionTest)
m.Any("/devtest/{sub}", devtest.Tmpl) m.Any("/devtest/{sub}", devtest.Tmpl)
} }

View file

@ -0,0 +1,42 @@
{{template "base/head" .}}
<div class="page-content devtest ui container">
{{template "base/alert" .}}
<div>
<h1>link-action</h1>
<div>
Use "window.fetch" to send a request to backend, the request is defined in an "A" or "BUTTON" element.
It might be renamed to "link-fetch-action" to match the "form-fetch-action".
</div>
<div>
<button class="link-action" data-url="fetch-action-test?k=1">test</button>
</div>
</div>
<div>
<h1>form-fetch-action</h1>
<div>Use "window.fetch" to send a form request to backend</div>
<div>
<form method="get" action="fetch-action-test?k=1" class="form-fetch-action">
<button name="btn">submit get</button>
</form>
<form method="post" action="fetch-action-test?k=1" class="form-fetch-action">
<div><textarea name="text" rows="3" class="js-quick-submit"></textarea></div>
<div><label><input name="check" type="checkbox"> check</label></div>
<div><button name="btn">submit post</button></div>
</form>
<form method="post" action="/no-such-uri" class="form-fetch-action">
<div class="gt-py-5">bad action url</div>
<div><button name="btn">submit test</button></div>
</form>
</div>
</div>
</div>
<style>
.ui.message.flash-message {
text-align: left;
}
.form-fetch-action {
margin-bottom: 1em;
border: 1px red dashed; /* show the border for demo purpose */
}
</style>
{{template "base/footer" .}}

View file

@ -89,6 +89,17 @@
<div><span data-tooltip-content="test tooltip" data-tooltip-interactive="true">text with interactive tooltip</span></div> <div><span data-tooltip-content="test tooltip" data-tooltip-interactive="true">text with interactive tooltip</span></div>
</div> </div>
<div>
<h1>Loading</h1>
<div class="is-loading small-loading-icon gt-border-secondary gt-py-2"><span>loading ...</span></div>
<div class="is-loading gt-border-secondary gt-py-4">
<p>loading ...</p>
<p>loading ...</p>
<p>loading ...</p>
<p>loading ...</p>
</div>
</div>
<div> <div>
<h1>GiteaOriginUrl</h1> <h1>GiteaOriginUrl</h1>
<div><gitea-origin-url data-url="test/url"></gitea-origin-url></div> <div><gitea-origin-url data-url="test/url"></gitea-origin-url></div>

View file

@ -1,12 +1,15 @@
<style> {{template "base/head" .}}
@media (prefers-color-scheme: dark) {
:root {
color-scheme: dark;
}
}
</style>
<ul> <ul>
{{range .SubNames}} {{range .SubNames}}
<li><a href="{{AppSubUrl}}/devtest/{{.}}">{{.}}</a></li> <li><a href="{{AppSubUrl}}/devtest/{{.}}">{{.}}</a></li>
{{end}} {{end}}
</ul> </ul>
<style>
ul {
line-height: 2em;
}
</style>
{{template "base/footer" .}}

View file

@ -6,7 +6,7 @@
</button> </button>
<div class="review-box-panel tippy-target"> <div class="review-box-panel tippy-target">
<div class="ui segment"> <div class="ui segment">
<form class="ui form" action="{{.Link}}/reviews/submit" method="post"> <form class="ui form form-fetch-action" action="{{.Link}}/reviews/submit" method="post">
{{.CsrfTokenHtml}} {{.CsrfTokenHtml}}
<input type="hidden" name="commit_id" value="{{.AfterCommitID}}"> <input type="hidden" name="commit_id" value="{{.AfterCommitID}}">
<div class="field gt-df gt-ac"> <div class="field gt-df gt-ac">

View file

@ -4,20 +4,22 @@
} }
.is-loading { .is-loading {
background: transparent !important;
color: transparent !important;
border: transparent !important;
pointer-events: none !important; pointer-events: none !important;
position: relative !important; position: relative !important;
overflow: hidden !important; overflow: hidden !important;
} }
.is-loading > * {
opacity: 0.3;
}
.is-loading::after { .is-loading::after {
content: ""; content: "";
position: absolute; position: absolute;
display: block; display: block;
width: 4rem;
height: 4rem; height: 4rem;
max-height: 50%;
aspect-ratio: 1 / 1;
left: 50%; left: 50%;
top: 50%; top: 50%;
transform: translate(-50%, -50%); transform: translate(-50%, -50%);
@ -28,18 +30,24 @@
border-radius: 100%; border-radius: 100%;
} }
.is-loading.small-loading-icon::after {
border-width: 2px;
}
.markup pre.is-loading, .markup pre.is-loading,
.editor-loading.is-loading, .editor-loading.is-loading,
.pdf-content.is-loading { .pdf-content.is-loading {
height: var(--height-loading); height: var(--height-loading);
} }
/* TODO: not needed, use "is-loading small-loading-icon" instead */
.btn-octicon.is-loading::after { .btn-octicon.is-loading::after {
border-width: 2px; border-width: 2px;
height: 1.25rem; height: 1.25rem;
width: 1.25rem; width: 1.25rem;
} }
/* TODO: not needed, use "is-loading small-loading-icon" instead */
code.language-math.is-loading::after { code.language-math.is-loading::after {
padding: 0; padding: 0;
border-width: 2px; border-width: 2px;
@ -47,11 +55,6 @@ code.language-math.is-loading::after {
height: 1.25rem; height: 1.25rem;
} }
#oauth2-login-navigator.is-loading::after {
width: 40px;
height: 40px;
}
@keyframes fadein { @keyframes fadein {
0% { 0% {
opacity: 0; opacity: 0;

View file

@ -29,6 +29,12 @@
color: var(--color-text); color: var(--color-text);
} }
.tippy-box[data-theme="form-fetch-error"] {
border-color: var(--color-error-border);
background-color: var(--color-error-bg);
color: var(--color-error-text);
}
.tippy-content { .tippy-content {
position: relative; position: relative;
padding: 1rem; padding: 1rem;

View file

@ -7,6 +7,7 @@ import {handleGlobalEnterQuickSubmit} from './comp/QuickSubmit.js';
import {svg} from '../svg.js'; import {svg} from '../svg.js';
import {hideElem, showElem, toggleElem} from '../utils/dom.js'; import {hideElem, showElem, toggleElem} from '../utils/dom.js';
import {htmlEscape} from 'escape-goat'; import {htmlEscape} from 'escape-goat';
import {createTippy} from '../modules/tippy.js';
const {appUrl, csrfToken, i18n} = window.config; const {appUrl, csrfToken, i18n} = window.config;
@ -60,6 +61,81 @@ export function initGlobalButtonClickOnEnter() {
}); });
} }
async function formFetchAction(e) {
if (!e.target.classList.contains('form-fetch-action')) return;
e.preventDefault();
const formEl = e.target;
if (formEl.classList.contains('is-loading')) return;
formEl.classList.add('is-loading');
if (formEl.clientHeight < 50) {
formEl.classList.add('small-loading-icon');
}
const formMethod = formEl.getAttribute('method') || 'get';
const formActionUrl = formEl.getAttribute('action');
const formData = new FormData(formEl);
const [submitterName, submitterValue] = [e.submitter?.getAttribute('name'), e.submitter?.getAttribute('value')];
if (submitterName) {
formData.append(submitterName, submitterValue || '');
}
let reqUrl = formActionUrl;
const reqOpt = {method: formMethod.toUpperCase(), headers: {'X-Csrf-Token': csrfToken}};
if (formMethod.toLowerCase() === 'get') {
const params = new URLSearchParams();
for (const [key, value] of formData) {
params.append(key, value.toString());
}
const pos = reqUrl.indexOf('?');
if (pos !== -1) {
reqUrl = reqUrl.slice(0, pos);
}
reqUrl += `?${params.toString()}`;
} else {
reqOpt.body = formData;
}
let errorTippy;
const onError = (msg) => {
formEl.classList.remove('is-loading', 'small-loading-icon');
if (errorTippy) errorTippy.destroy();
errorTippy = createTippy(formEl, {
content: msg,
interactive: true,
showOnCreate: true,
hideOnClick: true,
role: 'alert',
theme: 'form-fetch-error',
trigger: 'manual',
arrow: false,
});
};
const doRequest = async () => {
try {
const resp = await fetch(reqUrl, reqOpt);
if (resp.status === 200) {
const {redirect} = await resp.json();
formEl.classList.remove('dirty'); // remove the areYouSure check before reloading
if (redirect) {
window.location.href = redirect;
} else {
window.location.reload();
}
} else {
onError(`server error: ${resp.status}`);
}
} catch (e) {
onError(e.error);
}
};
// TODO: add "confirm" support like "link-action" in the future
await doRequest();
}
export function initGlobalCommon() { export function initGlobalCommon() {
// Semantic UI modules. // Semantic UI modules.
const $uiDropdowns = $('.ui.dropdown'); const $uiDropdowns = $('.ui.dropdown');
@ -114,6 +190,8 @@ export function initGlobalCommon() {
if (btn.classList.contains('loading')) return e.preventDefault(); if (btn.classList.contains('loading')) return e.preventDefault();
btn.classList.add('loading'); btn.classList.add('loading');
}); });
document.addEventListener('submit', formFetchAction);
} }
export function initGlobalDropzone() { export function initGlobalDropzone() {
@ -182,7 +260,7 @@ function linkAction(e) {
const $this = $(e.target); const $this = $(e.target);
const redirect = $this.attr('data-redirect'); const redirect = $this.attr('data-redirect');
const request = () => { const doRequest = () => {
$this.prop('disabled', true); $this.prop('disabled', true);
$.post($this.attr('data-url'), { $.post($this.attr('data-url'), {
_csrf: csrfToken _csrf: csrfToken
@ -201,7 +279,7 @@ function linkAction(e) {
const modalConfirmHtml = htmlEscape($this.attr('data-modal-confirm') || ''); const modalConfirmHtml = htmlEscape($this.attr('data-modal-confirm') || '');
if (!modalConfirmHtml) { if (!modalConfirmHtml) {
request(); doRequest();
return; return;
} }
@ -220,7 +298,7 @@ function linkAction(e) {
$modal.appendTo(document.body); $modal.appendTo(document.body);
$modal.modal({ $modal.modal({
onApprove() { onApprove() {
request(); doRequest();
}, },
onHidden() { onHidden() {
$modal.remove(); $modal.remove();

View file

@ -1,17 +1,24 @@
import $ from 'jquery'; import $ from 'jquery';
export function handleGlobalEnterQuickSubmit(target) { export function handleGlobalEnterQuickSubmit(target) {
const $target = $(target); const form = target.closest('form');
const $form = $(target).closest('form'); if (form) {
if ($form.length) { if (!form.checkValidity()) {
form.reportValidity();
return;
}
if (form.classList.contains('form-fetch-action')) {
form.dispatchEvent(new SubmitEvent('submit', {bubbles: true, cancelable: true}));
return;
}
// here use the event to trigger the submit event (instead of calling `submit()` method directly) // here use the event to trigger the submit event (instead of calling `submit()` method directly)
// otherwise the `areYouSure` handler won't be executed, then there will be an annoying "confirm to leave" dialog // otherwise the `areYouSure` handler won't be executed, then there will be an annoying "confirm to leave" dialog
if ($form[0].checkValidity()) { $(form).trigger('submit');
$form.trigger('submit');
}
} else { } else {
// if no form, then the editor is for an AJAX request, dispatch an event to the target, let the target's event handler to do the AJAX request. // if no form, then the editor is for an AJAX request, dispatch an event to the target, let the target's event handler to do the AJAX request.
// the 'ce-' prefix means this is a CustomEvent // the 'ce-' prefix means this is a CustomEvent
$target.trigger('ce-quick-submit'); target.dispatchEvent(new CustomEvent('ce-quick-submit', {bubbles: true}));
} }
} }

View file

@ -111,7 +111,7 @@ function showLineButton() {
hideOnClick: true, hideOnClick: true,
content: menu, content: menu,
placement: 'right-start', placement: 'right-start',
interactive: 'true', interactive: true,
onShow: (tippy) => { onShow: (tippy) => {
tippy.popper.addEventListener('click', () => { tippy.popper.addEventListener('click', () => {
tippy.hide(); tippy.hide();

View file

@ -3,6 +3,11 @@ import tippy from 'tippy.js';
const visibleInstances = new Set(); const visibleInstances = new Set();
export function createTippy(target, opts = {}) { export function createTippy(target, opts = {}) {
const {role, content, onHide: optsOnHide, onDestroy: optsOnDestroy, onShow: optOnShow} = opts;
delete opts.onHide;
delete opts.onDestroy;
delete opts.onShow;
const instance = tippy(target, { const instance = tippy(target, {
appendTo: document.body, appendTo: document.body,
animation: false, animation: false,
@ -13,9 +18,11 @@ export function createTippy(target, opts = {}) {
maxWidth: 500, // increase over default 350px maxWidth: 500, // increase over default 350px
onHide: (instance) => { onHide: (instance) => {
visibleInstances.delete(instance); visibleInstances.delete(instance);
return optsOnHide?.(instance);
}, },
onDestroy: (instance) => { onDestroy: (instance) => {
visibleInstances.delete(instance); visibleInstances.delete(instance);
return optsOnDestroy?.(instance);
}, },
onShow: (instance) => { onShow: (instance) => {
// hide other tooltip instances so only one tooltip shows at a time // hide other tooltip instances so only one tooltip shows at a time
@ -25,18 +32,19 @@ export function createTippy(target, opts = {}) {
} }
} }
visibleInstances.add(instance); visibleInstances.add(instance);
return optOnShow?.(instance);
}, },
arrow: `<svg width="16" height="7"><path d="m0 7 8-7 8 7Z" class="tippy-svg-arrow-outer"/><path d="m0 8 8-7 8 7Z" class="tippy-svg-arrow-inner"/></svg>`, arrow: `<svg width="16" height="7"><path d="m0 7 8-7 8 7Z" class="tippy-svg-arrow-outer"/><path d="m0 8 8-7 8 7Z" class="tippy-svg-arrow-inner"/></svg>`,
role: 'menu', // HTML role attribute, only tooltips should use "tooltip" role: 'menu', // HTML role attribute, only tooltips should use "tooltip"
theme: opts.role || 'menu', // CSS theme, we support either "tooltip" or "menu" theme: role || 'menu', // CSS theme, we support either "tooltip" or "menu"
...opts, ...opts,
}); });
// for popups where content refers to a DOM element, we use the 'tippy-target' class // for popups where content refers to a DOM element, we use the 'tippy-target' class
// to initially hide the content, now we can remove it as the content has been removed // to initially hide the content, now we can remove it as the content has been removed
// from the DOM by tippy // from the DOM by tippy
if (opts.content instanceof Element) { if (content instanceof Element) {
opts.content.classList.remove('tippy-target'); content.classList.remove('tippy-target');
} }
return instance; return instance;