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

highlight: Add tabindex when code is not highlighted #8917

Merged
merged 1 commit into from
Aug 23, 2021

Conversation

helfper
Copy link
Contributor

@helfper helfper commented Aug 22, 2021

PR #8568 added tabindex="0" to most <pre> tags, but it's still missing when using code fences and goldmark-highlighting doesn't highlight that code block. This PR fixes it by making this scenario reuse the same code that was changed in the aforementioned PR.

@bep
Copy link
Member

bep commented Aug 22, 2021

We have, I think, at least one other issue discussing using the default Chroma lexer when no lexer is given. Would that not fix this issue?

@helfper
Copy link
Contributor Author

helfper commented Aug 22, 2021

I opened this upstream: yuin/goldmark-highlighting#22

If that's accepted in a way that the code will always be highlighted (in the limit with the fallback lexer) and not as a flag, then yes, ctx.Highlighted() will always be true and the outer else -- which is where this change has an effect -- will never be executed.

@bep
Copy link
Member

bep commented Aug 22, 2021

OK, so if we enable GuessLanguage and pass a dummy lexer value that does not exist, that would work, or?

@helfper
Copy link
Contributor Author

helfper commented Aug 22, 2021

The situations in which the code is not highlighted (ctx.Highlighted() == false) by goldmark-highlighting are:

  • Attribute nohl is present, or
  • GuessLanguage is false and the language is not set or Chroma doesn't have a lexer for it

If GuessLanguage is true and the attribute nohl is not set, it will always be highlighted. If Chroma can't guess the language, goldmark-highlighting just uses the fallback lexer.

@bep
Copy link
Member

bep commented Aug 22, 2021

If I'm reading the above correctly, we should get a better situation by setting the GuessLanguage=true.

@helfper
Copy link
Contributor Author

helfper commented Aug 22, 2021

That's correct, but currently that's linked to Hugo's markup.highlight.guessSyntax, so it's controlled by the user. You may decide to turn it on by default though.

I think Chroma doesn't do much guessing still, but otherwise I'm usually not a big fan of guessing because of my experience with highlight.js, which does some weird highlightings when it tries to guess languages:
https://learn.netlify.app/en/basics/installation/#create-your-first-chapter-page
https://learn.netlify.app/en/cont/pages/#folders

@bep
Copy link
Member

bep commented Aug 22, 2021

I think Chroma doesn't do much guessing still

It doesn't, I had a look some time ago.

@bep
Copy link
Member

bep commented Aug 23, 2021

OK, thinking about it -- I'll merge this, and then follow up this upstream.

@bep bep merged commit 7a15eda into gohugoio:master Aug 23, 2021
@helfper
Copy link
Contributor Author

helfper commented Aug 23, 2021

TBH the main reason I raised this PR was because I was just scrolling through the code trying to figure out where the recently raised issues about highlighting come from, and liked how nice and symmetric this part of the code turned out 😃

if ctx.Highlighted() {
if entering {
writeDivStart(w, ctx)
} else {
writeDivEnd(w)
}
} else {
if entering {
highlight.WritePreStart(w, language, "")
} else {
highlight.WritePreEnd(w)
}
}

@helfper helfper deleted the no-highlighted-tabindex branch August 23, 2021 11:12
@bep
Copy link
Member

bep commented Aug 23, 2021

Yes, that's pretty code.

@helfper
Copy link
Contributor Author

helfper commented Aug 23, 2021

Looking further into it, even the <div> part could possibly be moved to highlight.go, because there's some duplication with code already there. At this point, one may also start wondering if goldmark-highlighting actually adds much value to Hugo, since most of its logic already exists in highlight.go. I think the only missing part in highlight.go is probably the attributes parsing in code fences, but Hugo already does something similar for other types of blocks, so it may not be too difficult to plug that into highlight.go.

The problem with the current overlap is that issues with a similar symptom like #8824 and #8913 need to be fixed in two different places -- the former in Hugo, the latter in goldmark-highlighting.

McShelby added a commit to McShelby/hugo-theme-relearn that referenced this pull request Aug 23, 2021
The new highlighter does not work to well if no syntax is specified
for the code block / code fence / indention.
See gohugoio/hugo#8917
@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants