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

Multinline comments in the start break syntax highlighting #6057

Closed
2 tasks done
oscarlofwenhamn opened this issue Feb 13, 2019 · 20 comments · Fixed by #7741
Closed
2 tasks done

Multinline comments in the start break syntax highlighting #6057

oscarlofwenhamn opened this issue Feb 13, 2019 · 20 comments · Fixed by #7741
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented type/bug

Comments

@oscarlofwenhamn
Copy link
Contributor

oscarlofwenhamn commented Feb 13, 2019

Description

When starting a file with a multiline comment or sometimes multiple lines of comments, syntax highlighting breaks (at least for .ps1 files).

For example, starting a file with one of the following works fine:

# Foo
---
<# Bar #>
---
# Bleh
#

This, however, breaks it:

<# Foo
#>
---
## Bar
##

Sample files in repo

Screenshots

image

image

image

@oscarlofwenhamn
Copy link
Contributor Author

Another example can be found in the example posted in #6064.

@lunny lunny added the type/bug label Feb 17, 2019
@zeripath
Copy link
Contributor

Hi!

Is this still an issue on try.gitea.io?

The revision that fixed #6064 probably fixed this.

@oscarlofwenhamn
Copy link
Contributor Author

Hi,
Yes, this problem remains (see my repo for an example: https://try.gitea.io/foobarbleh/foo).

@silverwind
Copy link
Member

I suspect a highlight.js issue here.

@oscarlofwenhamn
Copy link
Contributor Author

Is Gitea running the latest version? Could this be something to report to the highlight.js team?

@silverwind
Copy link
Member

highlight.js is 9.11.0, latest is 9.14.0.

It would help if you can showcase the issue in a fiddle like this, using the latest version.

@oscarlofwenhamn
Copy link
Contributor Author

I'm new to using fiddle, but I've given it a shot here. (I'm guessing there's no way of embedding fiddles on GitHub?)

One of the variants that are broken on Gitea works in the fiddle, but the second example remains broken. Let me know if there are any modifications or corrections that should be made to the fiddle and I'll sort it out.


Fiddle output screenshot:
image

@oscarlofwenhamn
Copy link
Contributor Author

If this isn't a priority issue for the general group, could anyone point me towards where (if possible) I could define what version of Highlight.js my instance should run? It would be nice to have this in upstream, but I would gladly sort out a local fix if this is not a priority issue.

@jolheiser
Copy link
Member

A temporary local workaround could be to use a custom footer template that instead gets highlight.js from {{AppSubUrl}}/js/highlight.js and then add the new version of highlight.js to your custom/public/js folder.
Docs for customizing templates

@silverwind
Copy link
Member

Updated fiddle with 9.15.6: https://jsfiddle.net/vk3j2gLd/3/

All three cases look alright, correct?

@oscarlofwenhamn
Copy link
Contributor Author

oscarlofwenhamn commented Feb 28, 2019

Updated fiddle with 9.15.6: https://jsfiddle.net/vk3j2gLd/3/

All three cases look alright, correct?

Yes, this looks great!

I'm gonna test it out with a local workaround, thanks for the suggestion @jolheiser. I'll get it done right away and post an update here, and if it works I'll close this issue and open a new one for updating highligt.js, if no one objects.

@silverwind
Copy link
Member

silverwind commented Feb 28, 2019

Once you confirm it works with in Gitea with newer highlight.js, I can raise a PR to update it. You want to use these resources:

https://cdnjs.cloudflare.com/ajax/libs/highlight.js/9.15.6/styles/default.min.css
https://cdnjs.cloudflare.com/ajax/libs/highlight.js/9.15.6/highlight.min.js

@oscarlofwenhamn
Copy link
Contributor Author

I can confirm that the variants listed in the fiddle all work with newer highlight.js, using the resources you've provided.


I did, however, notice another breaking variant. If I write three or more lines with two or more #'s, it still breaks, looking like the third line of #'s gets interpreted as something very particular as it gets uniquely highlighted.

image

This still is an improvement, but it is unfortunate as the user who raised the issue has a habit of tagging his codes with exactly that: opening with several lines of several #'s ... I will just have to ask him not to.

@oscarlofwenhamn
Copy link
Contributor Author

oscarlofwenhamn commented Mar 5, 2019

@silverwind I just noticed in the highlight.min.js you provided that it seems to be only registering something like 25 languages (powershell not included), instead of 175 which it seems like the original highlight.pack.js is registering. It being a .min. file maybe it is really slimmed down, but I figured I'd point it out.

@silverwind
Copy link
Member

Yeah, highlight.js weirdly does not publish a proper packed file with all languagues anywhere. I assume if we want to update it, we should include all languages which results in a 650kb file, not that much of a problem.

@oscarlofwenhamn
Copy link
Contributor Author

Definately, especially if the alternative would result in previously supported languages losing support. that feels like a step in the wrong direction.

@oscarlofwenhamn
Copy link
Contributor Author

@silverwind Any update on the PR?

@silverwind
Copy link
Member

silverwind commented Apr 16, 2019

Update PR is up, but it does not fix this issue. The root of your issue is that our use of hljs relies on automatic language detection which is rather broken. You can see it on the CSS class on the code element which in one of your broken examples has hljs ruby so the file was actually detected as ruby code.

A proper fix would be to generate a filename to language map similar to this and then pre-set the class so hljs can apply syntax highlighting based on filename and/or code fence language (e.g. like CodeMirror.findModeByName).

@stale
Copy link

stale bot commented Jun 15, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 weeks. Thank you for your contributions.

@stale stale bot added the issue/stale label Jun 15, 2019
@lunny lunny added the issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented label Jun 16, 2019
@stale stale bot removed the issue/stale label Jun 16, 2019
@oscarlofwenhamn
Copy link
Contributor Author

Sorry, mistakenly closed the issue.

silverwind added a commit to silverwind/gitea that referenced this issue Aug 4, 2019
@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
issue/confirmed Issue has been reviewed and confirmed to be present or accepted to be implemented type/bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants