naturalcrit / homebrewery

Create authentic looking D&D homebrews using only markdown
https://homebrewery.naturalcrit.com
MIT License
1.09k stars 326 forks source link

heading IDs are not unique vs manual html or curly-blocks #2832

Open ericscheid opened 1 year ago

ericscheid commented 1 year ago

Renderer

v3

Browser

Chrome

Operating System

MacOS

What happened?

Heading markdown (# A Heading) gets rendered as html Hn elements with an id attribute derived from the heading text (<h1 id="a-heading">A heading</h1>). Marked will ensure the value of the id attribute is unique among all heading elements it renders .. but does not consider any other elements with the same id.

Other elements with the same id could come from manually inserted html (<table id="myTable">...) and even from curly-blocks and curly-spans ({{#myID ...}}) and also curly-injectors (*BOLD*{#myID}).

The heading IDs should be unique to the full render, not just other headings.

Additional: any <a name="value"></a> should also be considered and avoided, since links to those fragment targets compete for the same namespace.

[Marking as bug because heading ids should be unique. Rare, esoteric, and obscure, and can be worked around.]

Code

{{#bq,note
this is a note with the id `bq`
}}

## BQ

Above is a `H2` which will get rendered as `<h2 id="bq">BQ</h2>`. This is not unique, given the curly-block with the id `bq` 

## BQ

Another `BQ` heading, but this time `marked()` will assign `id="bq-1"` because it has already seen the id `bq` in it's processing [of headings].
ericscheid commented 1 year ago

No rush on this bug.

Right now, even the TOC generator does not use the heading IDs for linking, it just uses page numbers. So, this bug will only appear for people knowingly and manually creating links to anchors. Hopefully they'll figure out the workaround (i.e. assign unique IDs to manually inserted html et al).

ericscheid commented 1 year ago

This issue will become (slightly) more important if these get solved: #1923 #2830

5e-Cleric commented 1 year ago

Important to consider that repeated IDs or names will not break anything really, it will just prevent links and scripts to get the element, but should not break a brew.

G-Ambatte commented 1 year ago

It's possible that we could inject a step right before the rendered Markdown gets pushed into the BrewRenderer display element which builds a list of all element IDs in the HTML document and then runs a deduplication process.

5e-Cleric commented 1 year ago

Another note, headings IDs are not unique unless they are the same tag, if you have a h1 and a h5 with the same name, they will have the same ID.

ericscheid commented 1 year ago

Another note, headings IDs are not unique unless they are the same tag, if you have a h1 and a h5 with the same name, they will have the same ID.

I'm seeing unique IDs in v3

image

This only seems to be a problem with the Legacy renderer

image
5e-Cleric commented 1 year ago

A correction, there are not unique if they are not in the same page.

image
ericscheid commented 1 year ago

OK, that is a problem then. The adding of heading id is an us-problem, not a marked-problem, right?

A problem that will also be complicated with partial-rendering mode.

G-Ambatte commented 1 year ago

This is an us problem, we render the Markdown one page at a time; I did suspect it would be unique if we rendered the whole thing at once.

G-Ambatte commented 1 year ago

PR #3089 renders the whole document at once, so if/when it goes live, heading IDs will be unique throughout the entire brew and persist in the HTML, even when on a non-rendered page.

A manually specified ID can still clash with an automatically generated one, though.

5e-Cleric commented 10 months ago

1430 also linked to this issue.

5e-Cleric commented 10 months ago

Isn't this fixed by #3156?

calculuschild commented 10 months ago

Not quite. We removed a blocker but still need to actually implement a global headerID list.

dbolack-ab commented 9 months ago

Not quite. We removed a blocker but still need to implement a global headerID list.

If we need a global header list for things other than this, then we will need to move GFMheader code into markdown.js and extend it there. As I ( very little ) understand markdown extensions, even if we forked and/or extended the GFM header module, we have no way of passing in a current list of IDs ( if maintained in markdown.js ) or resetting the list ( if maintained in the extension.

If we don't need the global header list for other things then I think we can close this, pending #3262

dbolack-ab commented 5 months ago

I have put together a PR that lets the GFM-Header-ID code behave the way we would like.

5e-Cleric commented 5 months ago

We want to use such a list for #2840, for example.