Skip to content
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

Fix EOL handling in web editor #27141

Merged
merged 16 commits into from
Sep 24, 2023
Merged

Fix EOL handling in web editor #27141

merged 16 commits into from
Sep 24, 2023

Conversation

silverwind
Copy link
Member

@silverwind silverwind commented Sep 19, 2023

Fixes #27136.

This does the following for Monaco's EOL setting:

  1. Use editorconfig setting if present
  2. Use the file's dominant line ending as detected by monaco, which uses LF for empty file

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Sep 19, 2023
@pull-request-size pull-request-size bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Sep 19, 2023
@silverwind silverwind marked this pull request as draft September 19, 2023 21:08
@github-actions github-actions bot added the topic/ui Change the appearance of the Gitea UI label Sep 19, 2023
@silverwind
Copy link
Member Author

At least I can rule out golang from stripping the \r: https://go.dev/play/p/ZY2wW6BKXyV

@pull-request-size pull-request-size bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 19, 2023
@@ -105,14 +105,26 @@ export async function createMonaco(textarea, filename, editorOpts) {
monaco.languages.register({id: 'vs.editor.nullLanguage'});
monaco.languages.setLanguageConfiguration('vs.editor.nullLanguage', {});

// TODO: there must be a better way to preserve CRLF in the template rendering
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess there is no other way besides using JS.

HTML standard does play tricks with CRLF/LF for textarea value.

Copy link
Member Author

@silverwind silverwind Sep 20, 2023

Choose a reason for hiding this comment

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

I think it's a self-inflicted issue by gitea's HTML rendering. But no idea where the removal of \r happens.

Copy link
Contributor

@wxiaoguang wxiaoguang Sep 20, 2023

Choose a reason for hiding this comment

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

It is not related to Gitea.

For historical reasons, the element's value is normalized in three different ways for three different purposes.
The raw value is the value as it was originally set. It is not normalized.
The API value ... It is normalized so that line breaks use U+000A LINE FEED (LF) characters.

textarea.value is the API value IIRC.

Copy link
Member Author

@silverwind silverwind Sep 20, 2023

Choose a reason for hiding this comment

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

I also tried passing as data-value and the \r was stripped as well there. So it's not related to textarea.value. It must be something in our HTML renderer.

Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see so. I think everything just works well.

image

Copy link
Contributor

Choose a reason for hiding this comment

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

The standard has clarified that there is no CR but only LF when parsing HTML. I do not know how to explain more.

Nothing is unexpected, everything is right from HTML/Golang/GoTemplate/Gitea side.

The only wrong thing is that you shouldn't emit "\r" into HTML directly if you want to keep it.

Copy link
Contributor

Choose a reason for hiding this comment

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

2. I fail to see how that is related to the problem. I have shown that regular go template does not have the problem, but ours rendering does, so it's a bug in our rendering.

Your code is wrong. I have already shown that our code renders "\r\n" correctly: #27141 (comment)

Copy link
Member Author

@silverwind silverwind Sep 21, 2023

Choose a reason for hiding this comment

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

Ah, you mean the browser discards it during HTML parsing stage? I will check with curl to confirm. If this is true, then yes, we need to encode it.

I'm also wondering which encoding would be best in terms of output size and parsing speed. Maybe JSON-encoding the string might be best.

Copy link
Member Author

Choose a reason for hiding this comment

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

curl confirms, it's output correctly. Damn browsers and their silly parsers:

curl -s http://localhost:3500/devtest/gitea-ui | grep data-foo= | xxd
00000000: 3c64 6976 2063 6c61 7373 3d22 7061 6765  <div class="page
00000010: 2d63 6f6e 7465 6e74 2064 6576 7465 7374  -content devtest
00000020: 2075 6920 636f 6e74 6169 6e65 7222 2064   ui container" d
00000030: 6174 612d 666f 6f3d 2261 0d0a            ata-foo="a..

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed with f394554.

@silverwind silverwind marked this pull request as ready for review September 21, 2023 09:55
@silverwind
Copy link
Member Author

It's ready now. Data is passed through JSON encoding from backend to frontend now.

@silverwind silverwind added backport/v1.21 This PR should be backported to Gitea 1.21 type/bug labels Sep 21, 2023
Copy link
Contributor

@wxiaoguang wxiaoguang left a comment

Choose a reason for hiding this comment

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

PR description also needs to update.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Sep 21, 2023
@silverwind
Copy link
Member Author

Removed the WIP note in description.

@silverwind silverwind merged commit 3a187ea into go-gitea:main Sep 24, 2023
25 checks passed
@GiteaBot GiteaBot added this to the 1.22.0 milestone Sep 24, 2023
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Sep 24, 2023
Fixes go-gitea#27136.

This does the following for Monaco's EOL setting:

1. Use editorconfig setting if present
2. Use the file's dominant line ending as detected by monaco, which uses
LF for empty file
@GiteaBot GiteaBot added backport/done All backports for this PR have been created and removed reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. labels Sep 24, 2023
@silverwind silverwind deleted the eol branch September 24, 2023 20:30
silverwind added a commit that referenced this pull request Sep 24, 2023
Backport #27141 by @silverwind

Fixes #27136.

This does the following for Monaco's EOL setting:

1. Use editorconfig setting if present
2. Use the file's dominant line ending as detected by monaco, which uses
LF for empty file

Co-authored-by: silverwind <[email protected]>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Sep 25, 2023
* giteaofficial/main:
  Add missing public user visibility in user details page (go-gitea#27246)
  Use mask-based fade-out effect for `.new-menu` (go-gitea#27181)
  [skip ci] Updated translations via Crowdin
  Fix z-index on markdown completion (go-gitea#27237)
  Update database-preparation and add note re: MariaDB (go-gitea#27232)
  cleanup locale function usage (go-gitea#27227)
  Fix EOL handling in web editor (go-gitea#27141)
  Fix PushEvent NullPointerException jenkinsci/github-plugin (go-gitea#27203)
  fix issues on action runners page (go-gitea#27226)
  Fix Fomantic UI dropdown icon bug when there is a search input in menu (go-gitea#27225)
  Update go-enry to 2.8.5 (go-gitea#27215)
  Update nodejs installation method in release container (go-gitea#27207)
  Quote table `release` in sql queries (go-gitea#27205)
  Fix push mirror, wrong timestamp format (go-gitea#27153)
  Allow copying issue comment link on archived repos and when not logged in (go-gitea#27193)
  fix: text decorator on issue sidebar menu label (go-gitea#27206)
  Update JS and Poetry dependencies and eslint (go-gitea#27200)
  Remove some dead code (go-gitea#27196)

# Conflicts:
#	templates/repo/issue/view_content/context_menu.tmpl
@testdasi
Copy link

The fixes have broken it. All my files are LF and when edit in browser, it always changes to Windows. This is very frustrating. Were the fixes regression--tested at all?

I can't possibly be the only person editing files in a Windows browser. Both Firefox and Chrome are affected.

@wxiaoguang
Copy link
Contributor

If there is no quick fix, let's revert this PR.

lng2020 added a commit to lng2020/gitea that referenced this pull request Nov 17, 2023
@lunny
Copy link
Member

lunny commented Nov 17, 2023

If there is no quick fix, let's revert this PR.

I have also encounter this problem.

@delvh
Copy link
Member

delvh commented Nov 17, 2023

Were the fixes regression--tested at all?

By the way, as you've noticed yourself now, it isn't quite easy to catch a Windows bug when most maintainers are using Linux/Mac… 😉

lunny pushed a commit that referenced this pull request Nov 22, 2023
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Nov 22, 2023
silverwind pushed a commit that referenced this pull request Nov 22, 2023
@kuon
Copy link

kuon commented Nov 22, 2023

Yes I just encountered the bug where all LF got converted to CRLF, with v 1.21.0. The repository has no .gitattribute.

@testdasi
Copy link

Yes I just encountered the bug where all LF got converted to CRLF, with v 1.21.0. The repository has no .gitattribute.

You might want to try 1.21.0 nightly for the time being. I'm using nightly and the change was reverted there.

I still can't understand the rationale behind using a user's OS to force CRLF EOL even when the editor correct detected non-Windows files.

@wxiaoguang
Copy link
Contributor

I still can't understand the rationale behind using a user's OS to force CRLF EOL even when the editor correct detected non-Windows files.

It's not related to OS, but it is a changed behavior caused by backend code for how to use HTML textarea content.

@go-gitea go-gitea locked as resolved and limited conversation to collaborators Dec 23, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created backport/v1.21 This PR should be backported to Gitea 1.21 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. topic/ui Change the appearance of the Gitea UI type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Gitea should not convert newlines when editing a file through the web interface
9 participants