Browsers without SubmitEvent.submitter support don't work with form-fetch-action mechanism well #28319

Closed wolfbeast closed 9 months ago

Description

I recently updated to 1.21.0 and am running into an issue now that I cannot close or re-open pull requests from the discussion page buttons at the bottom (neither with or without comment). I did not have this issue on our last version (1.19.4).

I am still able to open and close pull requests from the pull request listing (checking the boxes then clicking the close button).

I have not seen any error message be thrown in the UI.

Gitea Version

1.21.0

Can you reproduce the bug on the Gitea demo site?

Yes

Log Gist

No response

Screenshots

FireShot Pro Screen Capture #396 - '#1 - Update test md - bug-test-repo - Gitea_ Git with a cup of tea' - try gitea io_wolfbeast71_bug-test-repo_pulls_1

Git Version

No response

Operating System

CentOS 7

How are you running Gitea?

Binary download from gitea distr. natively installed (command-line/service)

Database

MySQL/MariaDB

What do you mean by Doesn't work? You can't click on it, or after clicking on it, nothing changes.

Sorry if I wasn't clear. I can click on it, the page refreshes, but nothing changes in the status of the pull request.

I'm having the same/similar issue. It's a Draft PR with one dependency.

You need to close all issues blocking this pull request before you can merge it

Tried closing via the PR listing, but then it says

Cannot batch close issues that you choose, because issue #115487 still has open dependencies

This seems backwards, a dependency should not prevent me from closing the PR.

I'm having the same/similar issue.

I don't think that's related. This issue occurs for me regardless of dependencies and does not throw any errors. It just reloads the page (as if the command would be processed) but the status of the PR does not change, no error given.

I'm having the same/similar issue.

I don't think that's related. This issue occurs for me regardless of dependencies and does not throw any errors. It just reloads the page (as if the command would be processed) but the status of the PR does not change, no error given.

Quote your issue:

Can you reproduce the bug on the Gitea demo site?

Yes

How to?

How to?

edit a file in a repo on a separate branch
create a PR from the branch comparison
attempt to close the PR from the comment tab on the PR (see screenshot)

On try.gitea.io https://try.gitea.io/wxiaoguang/test/pulls/35

1 & 2: edit a file in a repo on a separate branch, create a PR from the branch comparison

![image](https://github.com/go-gitea/gitea/assets/2114189/dd63c3cd-bc00-4404-a45c-f6b395000255) ![image](https://github.com/go-gitea/gitea/assets/2114189/2bfea74b-f0f2-4c3f-862c-ebf99d55f003)

attempt to close the PR from the comment tab on the PR (see screenshot)

![image](https://github.com/go-gitea/gitea/assets/2114189/7a89b8b8-a118-4ffb-a0a6-68a67d54ae84)

Everything seems fine.

I'm actually seeing the same simply with an open issue. can't close it with the button. I'll see if I can get a log.

Double confirm: are you sure you can reproduce it on try.gitea.io? If yes, is it browser related? If no, is it custom-template related?

I can't see anything in our gitea log. Nothing in the browser console either. no errors on the page.

The behavior when checking the network request seems to be the following:

POST request to "comments"
response is a redirect: to the issue/pr URL.
POST request to /-/fetch-redirect URL.
response is 303 to the issue/PR page
GET request to the issue/PR page
response is the issue/PR page with further loading of the page

has anything in that respect changed since 1.19?

are you sure you can reproduce it on try.gitea.io?

Yes absolutely. i just tested and verified. created a new issue, tried to close it. nada.

OK, seems Blink-based browsers have no problem (tested with edge and it works), so there may be something specific interop-wise with Pale Moon as a browser.

EDIT: verified it's not a regression in Pale Moon as an older installation displays the exact same behaviour on the test instance.

has anything in that respect changed since 1.19?

I think it's related to this one: https://github.com/go-gitea/gitea/pull/25258

Looks like that might be the problem yes. Clearly there are some issues depending on browser implementations in use as the fetch spec is pretty ambiguous when it comes to CORS, preflight and redirects and it's not guaranteed to work. You may want to revert that.

Looks like that might be the problem yes. Clearly there are some issues depending on browser implementations in use as the fetch spec is pretty ambiguous when it comes to CORS, preflight and redirects and it's not guaranteed to work.

Sorry I didn't see it ever used any CORS/pre-flight/fetch-redirects (the /-/fetch-redirect is a traditional POST FORM request to help to redirect, no JS fetch redirect)

See the requests from Firefox:

![image](https://github.com/go-gitea/gitea/assets/2114189/c5c68b71-88d3-44d0-832a-01292154c0b0)

Everything there is using standard browser implementations. I don't see anything tricky. Any modern browser should work well with it.

You may want to revert that.

That change benefits a lot and is the future. I do not see any reason to revert.

Sorry I didn't see it ever used any CORS/pre-flight/fetch-redirects (the /-/fetch-redirect is a traditional POST FORM request to help to redirect, no JS fetch redirect)

I was assuming it was a fetch-redirect based on the response names. Please let me know exactly what requests you're making if you're convinced this is a web client issue. note that "standard browser implementations" in general means "Blink" but not everyone is using Blink.

I do not see any reason to revert.

Well as far as I know Goanna/UXP/Pale Moon is following the Fetch spec correctly! This is a problem as we do use gitea for our code development. If there is an issue with the implementation of Fetch in Pale Moon then please tell me what is wrong with it, as Gitea is literally the only WebUI where this problem seems to occur. Fetch is used everywhere and there have not been any reports otherwise. What exactly are the fetch API calls used here?

FTR, here are the network requests in Pale Moon. It seems to be exactly the same as Firefox you posted.

Sorry I didn't see it ever used any CORS/pre-flight/fetch-redirects (the /-/fetch-redirect is a traditional POST FORM request to help to redirect, no JS fetch redirect)

I was assuming it was a fetch-redirect based on the response names. Please let me know exactly what requests you're making if you're convinced this is a web client issue. note that "standard browser implementations" in general means "Blink" but not everyone is using Blink.

About /-/fetch-redirect:

it is here: https://github.com/go-gitea/gitea/pull/25258/files#diff-ed8bf2e03a68c1b59f6fd0d9a4b0a7055de85b9f0c5f92b065afeef7843e27eaR64-R77
and there are comments for this case: https://github.com/go-gitea/gitea/pull/25258/files#diff-4b30210cf960a3b22da443914158a447e26a71161ba15c0f2d7fd6d1e87ac990R12-R26

About: note that "standard browser implementations" in general means "Blink" but not everyone is using Blink.

I also showed Firefox above, which uses Gecko, the same as PaleMoon

I do not see any reason to revert.

Well as far as I know Goanna/UXP/Pale Moon is following the Fetch spec correctly! This is a problem as we do use gitea for our code development. If there is an issue with the implementation of Fetch in Pale Moon then please tell me what is wrong with it, as Gitea is literally the only WebUI where this problem seems to occur. Fetch is used everywhere and there have not been any reports otherwise. What exactly are the fetch API calls used here?

I think the Gitea code is also using Fetch spec correctly, and Gitea also has been using fetch widely for long time.

So, maybe the problem is not directly related to fetch. You could compare the requests and responses line by line to see the difference.

I tested with PaleMoon, the problem is that PaleMoon doesn't send the expected form content:

The left one is firefox, the right one is palemoon

I think the root problem is here. PaleMoon doesn't support "SubmitEvent.submitter"

Pale Moon does NOT use Gecko. It is forked from Gecko and has been independently developed for the last decade. Gecko in 2023 is pretty much equal to Blink because Google pays their bills to be the controlled "second vote" for any spec requests.

I looked at your quoted code, and noticed the lines you pointed out do not include the "status" attribute for the form submitted. I don't see how that is being transferred to Gitea:

// doRedirect does real redirection to bypass the browser's limitations of "location"
// more details are in the backend's fetch-redirect handler
function doRedirect(redirect) {
  const form = document.createElement('form');
  const input = document.createElement('input');
  form.method = 'post';
  form.action = `${appSubUrl}/-/fetch-redirect`;
  input.type = 'hidden';
  input.name = 'redirect';
  input.value = redirect;
  form.append(input);
  document.body.append(form);
  form.submit();
}

You synthesize a new form with "redirect:" and nothing else, so it would make sense that "status" isn't part of that submitted form...

How are you handling the form submission code, exactly? maybe this has less to do with the fetch redirect and more to do with submission. Are you triggering it from JS and if so, how?

You synthesize a new form with "redirect:" and nothing else, so it would make sense that "status" isn't part of that submitted form...

How are you handling the form submission code, exactly? maybe this has less to do with the fetch redirect and more to do with submission. Are you triggering it from JS and if so, how?

They are really not related. See my comment above, the problem is PaleMoon lacks SubmitEvent.submitter support.

The status key/value should come from SubmitEvent.submitter when you click the "Close" or "Reopen" button.

I think the root problem is here. PaleMoon doesn't support "SubmitEvent.submitter"

Ah. that would do it, yes. We don't have the SubmitEvent interface at all, I think. I can see about implementing that... but that won't be short term. Could you perhaps implement a workaround in Gitea in the interim?

EDIT: This by the way underlines the problem using ?. everywhere -- any errors get silenced.

I think the root problem is here. PaleMoon doesn't support "SubmitEvent.submitter"

Ah. that would do it, yes. We don't have the SubmitEvent interface at all, I think. I can see about implementing that... but that won't be short term. Could you perhaps implement a workaround in Gitea in the interim?

TBH, I don't know how to polyfill it .... how could the JS code know the real "submitter"? The low-level logic is:

For a tradtional form, when a users clicks a form button, its value will be also included in the form request
Now Gitea is using more and more fetch (AJAX) forms, to improve user experiences. So when a user clicks a "fetch form" button, the JS code must know the button and use its name/value to fill the form.

EDIT: This by the way underlines the problem using ?. everywhere -- any errors get silenced.

I think it is the correct usage of ?. because the submitter could be null in many cases IIRC. See the MDN: https://developer.mozilla.org/en-US/docs/Web/API/SubmitEvent/submitter If the submission was not triggered by a button of some kind, the value of submitter is null.

If the submission was not triggered by a button of some kind, the value of submitter is null.

I don't understand how that could be a valid scenario here. Your action is linked exclusively to a button, no? Anyway that's not really relevant to this issue.

I've made a priority issue to get this implemented now since you say you don't know how to pass values to the form submission otherwise and don't want to use normal form submission and this is literally breaking our devops (and reverting gitea isn't really an option anymore since we've worked several weeks in 1.21 already). I hope there won't be any big hurdles porting this code :/

If the submission was not triggered by a button of some kind, the value of submitter is null.

I don't understand how that could be a valid scenario here. Your action is linked exclusively to a button, no?

The form could also be submitted by: JS code, Enter, etc. So in many cases submitter could be null, the framework should tolerate (work with) these cases, they are valid cases.

don't want to use normal form submission

Because tradition POST FORM causes too many problem (duplicate content, content lost, etc). You can take a look at these related issues/PRs.

I hope there won't be any big hurdles porting this code :/

For myself, I also agree (and am doing my best) to keep compatibility. For this case, I already checked the MDN, it shows that all recent browsers have supported such feature for long time. I couldn't figure out which feature works with PaleMoon while which don't ahead ......

For myself, I also agree (and am doing my best) to keep compatibility. For this case, I already checked the MDN, it shows that all recent browsers have supported such feature for long time. I couldn't figure out which feature works with PaleMoon while which don't ahead ......

Hey I fully understand. The problem is that many specs these days are moving targets created almost exclusively by Google who benefit greatly from an implementation-first ecosystem, and we're often explicitly not included in any spec discussions or notifications (because we apparently pissed off the wrong people by existing independently, and Mozilla apparently hates us and has tried their best to cancel us -- including defamatory statements by Mozilla employees in official Mozilla chatrooms) so we can get blindsided by the sudden use of a less well-known addition that has had almost zero documentation or implementation notes attached (like this one). We're a small team, the whole of web specs is obscenely large, and have to be reactive as a result. Conversely, the interesting part is that if something works on Pale Moon, it's pretty much guaranteed to work everywhere because we stick to established standards where we can.

I absolutely understand you're doing your best as well to stay compatible as broadly as possible, and that the previous method apparently caused issues for you (although we've never seen problems with duplicate or lost content in Gitea from Pale Moon...) so we're having to find a way around it. I just recapped the situation; it wasn't a complaint so please don't see it that way.

I've managed to find a tricky approach to polyfill.

Try "Polyfill SubmitEvent for PaleMoon #28441"

It's not perfect because it doesn't work well with "focus", while I guess most people used to click/touch the buttons on the UI 🤣

Thanks for trying to polyfill! So far Ive not had much luck porting stuff across on our end so it's probably going to be tricky because of our divergence from Gecko.

Try "Polyfill SubmitEvent for PaleMoon https://github.com/go-gitea/gitea/pull/28441"

I'm sorry but I don't how I could try that. I know nothing about Go...

You don't need to know anything about it to test it. Install Go and npm, and the rest is

git clone git@github.com:go-gitea/gitea.git
cd gitea
git fetch origin pull/28441/heads:28441-palemoon-submitevent
git switch 28441-palemoon-submitevent
TAGS="bindata sqlite sqlite_unlock_notify" make build

Thanks for the pointers. After struggling a little through installing from source (needing gcc (for CGO_ENABLED=1) for sqlite and all that jazz...) I can confirm that the 28441 branch works properly for closing/reopening issues from the comments page! 💙

Thanks for the quick turnaround here! I'm assuming this will be uplifted to 1.21.3? what's the planned release date for that?

You can use the nightly build.

@techknowlogick Would you like to use "1.21-nightly" for the directory name? Then it doesn't need to explain again and again, and the FAQ can be removed.

https://docs.gitea.com/help/faq?_highlight=nightly#difference-between-1x-and-1xx-downloads-how-can-i-get-latest-stable-release-with-bug-fixes

Okay got it. I wasn't aware that "1.21" was "1.21-release-nightly". I generally don't want to run nightly builds on production but this is of course an exception. I'll update to 1.21.3 when it's out in due course.

Good news. That'll be soon.

I wasn't aware that "1.21" was "1.21-release-nightly"

I just don't want to repeat that "1.21 is 1.21-nightly" again and again, it confuses many users.

I generally don't want to run nightly builds on production but this is of course an exception.

There is no difference for stability between 1.21-nighty and 1.21.3. Actually, 1.21-nightly will become 1.21.3.

go-gitea / gitea