mdx-js / mdx-analyzer

MDX extension for Visual Studio Code
https://marketplace.visualstudio.com/items?itemName=unifiedjs.vscode-mdx
MIT License
349 stars 21 forks source link

Headings H3 - H6 are assigned incorrect textmate scopes #483

Open icorbrey opened 2 weeks ago

icorbrey commented 2 weeks ago

Initial checklist

Affected packages and versions

v1.8.11

Link to runnable example

No response

Steps to reproduce

  1. Paste the following text into a VSCode window:
    
    # Heading 1

Heading 2

Heading 3

Heading 4

Heading 5
Heading 6

2. Execute "Developer: Inspect Editor Tokens and Scopes" through the `Ctrl`+`Shift`+`P` menu
3. Inspect the scopes of each heading

### Expected behavior

Since H1 is assigned `markup.heading.atx.1` and H2 is assigned `markup.heading.atx.2`, H3 - H6 should be assigned `markup.heading.atx.3`, `markup.heading.atx.4`, and so on.

### Actual behavior

H3 - H6 are all assigned `markup.heading.atx.2`.

### Affected runtime and version

Not sure, it's in the most recent version of VSCode

### Affected package manager and version

_No response_

### Affected OS and version

Ubuntu 22.04.5 LTS

### Build and bundle tools

_No response_
icorbrey commented 2 weeks ago

A visual example of what's occurring, I'm writing a theme that differentiates each heading level with color and this is currently broken on MDX. Expected: image

Actual: image

icorbrey commented 2 weeks ago

Looks like the issue is upstream in https://github.com/wooorm/markdown-tm-language, should be an easy fix? H3 - H6 are already targeted in the TMLanguage file, just not assigned the right IDs

icorbrey commented 2 weeks ago

PR is in to hopefully fix this at https://github.com/wooorm/markdown-tm-language/pull/12

wooorm commented 2 weeks ago

Hi!

Have you tested that your changes work with GitHub and some common/default VS Code themes?

Often, themes don’t support all scopes.

wooorm commented 2 weeks ago

I think for GH this will be fine. That’s just looking at markup.heading.

remcohaszing commented 2 weeks ago

It might be interesting to know of this breaks any things. But even if this breaks a theme, this change makes sense IMO. Themes can adapt.

icorbrey commented 2 weeks ago

Is there a reason H3 - H6 were assigned H2 scopes?

wooorm commented 2 weeks ago

I don’t remember. Hence the question.

It might be interesting to know of this breaks any things. But even if this breaks a theme, this change makes sense IMO. Themes can adapt.

GH doesn’t. There are several workarounds for GH. That’s the most important.

As for VS Code defaults: that’s chicken and egg. At this point this grammar is not too often used. So it is less likely that VS Code changes things. But theoretically I’d agree with you.

Also: this can be used in many places. TextMate. Sublime. Anywhere. Some compatibility between existing things is nice. We can’t expect every software to change.

icorbrey commented 2 weeks ago

Okay so after looking a fair amount I can't really see anything that is explicitly relying on H3 - H6 all having H2 scope, but some that already rely on headings having the correct scope that get broken in MDX. For example, Catppuccin Frappe from Catppuccin for VSCode

image

Searching for files not using deeper headings shows that pretty much repo that references H2 scopes and not any deeper ones is just bundling https://github.com/wooorm/markdown-tm-language instead of doing something meaningful with it.

Now I can't speak on GitHub, as I don't know where I'd even begin to look for their implementations let alone if they're source available, but from my surface level lookover I can't see anywhere that they'd be relying on this behavior. They seem to not care what level a heading is wherever I look.

wooorm commented 2 weeks ago

For GH, see my earlier comment https://github.com/mdx-js/mdx-analyzer/issues/483#issuecomment-2467876802. As for source, see e.g. https://github.com/primer/github-syntax-dark/blob/master/lib/github-dark.css.

Thanks for checking!

icorbrey commented 2 weeks ago

Ah, I think I missed that comment. Thanks!