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

Highlighting multiple codes in the same pre is broken #34

Closed
4 tasks done
talatkuyuk opened this issue Feb 3, 2025 · 7 comments
Closed
4 tasks done

Highlighting multiple codes in the same pre is broken #34

talatkuyuk opened this issue Feb 3, 2025 · 7 comments
Labels
💪 phase/solved Post is done 🐛 type/bug This is a problem

Comments

@talatkuyuk
Copy link
Contributor

talatkuyuk commented Feb 3, 2025

Initial checklist

Affected package

rehype-highlight

Steps to reproduce

rehype-highlight highlights the pre content instead of code content.

Actually, <pre> tag defines preformatted text and preserves both spaces and line breaks.

A <pre> element can hold text, anchor tag, or contain multiple <code> elements. It is valid HTML usage.

It is not suitable or is a violation to have additional elements in pre other than code; but, rehype-highligt should actually preserve the xml structure of pre element and not interfere with other elements' contents inside pre other than code, and focus code element content for code highlighting. For some reason pre element may have additional elements like copy content button or footer etc, or may have more than one code element.

See the rehype-highlight behaviour:

import { rehype } from "rehype";
import rehypeHighlight from "rehype-highlight";

const html = `<pre><button>click</button><code class="language-javascript">const a = 1;</code><span>footer</span></pre>`;

// without highlighting
const file = await rehype()
  .data("settings", { fragment: true })
  .process(html);

console.log(String(file)); 

// result: for reference without highlighting
// "<pre><button>click</button><code class="language-javascript">const a = 1;</code><span>footer</span></pre>"

// with highlighting
const file = await rehype()
  .data("settings", { fragment: true })
  .use(rehypeHighlight)
  .process(html);

console.log(String(file)); 

// result: pay attention to texts "click" and "footer"
// "<pre><button>click</button><code class="hljs language-javascript">clickconst a = <span class="hljs-number">1</span>;footer</code><span>footer</span></pre>"

const html_with_two_codes = "<pre><code class="language-javascript">const a = 1;</code><code class="language-phyton">printf("x")</code></pre>";

// with highlighting
const file = await rehype()
  .data("settings", { fragment: true })
  .use(rehypeHighlight)
  .process(html_with_two_codes);

console.log(String(file)); 

// result: strange because the first code content includes the second one's content after highligting.
// "<pre><code class="hljs language-javascript"><span class="hljs-keyword">const</span> a = <span class="hljs-number">1</span>;<span class="hljs-title function_">printf</span>(<span class="hljs-string">"x"</span>)</code><code class="hljs language-phyton">printf("x")</code></pre>"

I realized this situation while testing rehype-highlight-code-lines as a result of having undesired additional line containers which contains undesired content/spaces.

Actual behavior

rehype-highlight highlights the pre content instead of code content.

Expected behavior

rehype-highlight focuses and highlights the code content instead of pre content.

Runtime

node@20

Package manager

npm@11

Operating system

macos@latest

Build and bundle tools

No response

@github-actions github-actions bot added 👋 phase/new Post is being triaged automatically 🤞 phase/open Post is being triaged manually and removed 👋 phase/new Post is being triaged automatically labels Feb 3, 2025
@wooorm
Copy link
Member

wooorm commented Feb 3, 2025

rehype-highlight highlights the pre content instead of code content.

Does it? The code seems to check node.tagName === 'code'? And work on node?


Aside: Looks like you might want a “copy” button?
Would that word “copy” be “preformatted” as well?
Why not wrap the pre and code in another container, and add buttons to that?
On the MDX website, for example, I wrap certain things in a code frame, and add copy buttons: https://github.com/mdx-js/mdx/blob/b9ec24467b718a5e698984a235654a9f311161af/website/mdx-loader.js#L379.

@wooorm
Copy link
Member

wooorm commented Feb 3, 2025

Bug seems to be toText(parent) instead of toText(node), as introduced here: 734349c; wild, been there for years

@talatkuyuk
Copy link
Contributor Author

talatkuyuk commented Feb 3, 2025

I would say this, exactly. 👍 toText(parent) ---> toText(node)

try {
  result = lang
    ? lowlight.highlight(lang, toText(parent), {prefix})
    : lowlight.highlightAuto(toText(parent), {prefix, subset})
} catch (error) {
  // ...
}

Aside: Looks like you might want a “copy” button?

Nope. This is not about that. The issue effects rehpe-highlight-code-lines.

@wooorm wooorm closed this as completed in 5c3b277 Feb 3, 2025

This comment has been minimized.

@wooorm
Copy link
Member

wooorm commented Feb 3, 2025

My guess is that, because toText didn’t support a whitespace: 'pre' option years ago, I assumed parent would be fine

@wooorm wooorm added 💪 phase/solved Post is done 🐛 type/bug This is a problem labels Feb 3, 2025
@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Feb 3, 2025
@wooorm wooorm changed the title highlighting code content is more suitable than highlighting pre content Highlighting multiple codes in the same pre is broken Feb 3, 2025
@wooorm
Copy link
Member

wooorm commented Feb 3, 2025

Released in 7.0.2! Thank you :)

@talatkuyuk
Copy link
Contributor Author

You are so speedy, thank you too :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
💪 phase/solved Post is done 🐛 type/bug This is a problem
Development

No branches or pull requests

2 participants