search

syntax-tree / mdast-util-to-hast

utility to transform mdast to hast
https://unifiedjs.com
MIT License
102 stars 42 forks source link

allow empty footnoteLabel #70

Closed cocoliliace closed 1 year ago

cocoliliace commented 1 year ago

Initial checklist

Description of changes

The default value of footnoteLabel, 'Footnotes', should only be applied if the user did not specify its value (undefined or null). If the user specifies an empty string, footnoteLabel should have the value of an empty string.

The implementation copies the exact implementation for clobberPrefix, written ~5 lines above.

A user may want to use an empty string when they use footnoteTagName: "hr"

wooorm commented 1 year ago

Hmm, that’s not what footnoteLabelTagName is for. It’s for changing to h1 or h3 or whatever to suit your heading structure.

Do you want to remove a label? I don’t see why you don’t want a label?

And why choose a hr? I don’t understand the reasoning, semantically.

While the empty string is okay in that no text shows up, an empty text node is still injected into the hr, which is not correct: https://github.com/syntax-tree/mdast-util-to-hast/blob/c87cd606731c88a27dbce4bfeaab913a9589bf83/lib/footer.js#L116.

cocoliliace commented 1 year ago

Do you want to remove a label? I don’t see why you don’t want a label?

It's for styling purposes. For a blog post, at least, I feel the text "Footnote" is unnecessary since the positioning of the text already conveys that they are footnotes.

And why choose a hr? I don’t understand the reasoning, semantically.

The link you sent says "The hr element represents a paragraph-level thematic break, e.g., a scene change in a story, or a transition to another topic within a section of a reference book". Does that not apply to footnotes?

While the empty string is okay in that no text shows up, an empty text node is still injected into the hr, which is not correct

When I use a space character " ", for footnoteLabel, I get the following html for my compiled page:

<section data-footnotes="" class="footnotes"><hr class="sr-only" id="footnote-label"> 
<ol>
<li id="fn-1">
...

Is this not correct? I don't see an extra text node. The output of using " " is actually perfect for what I'm looking for. I just thought it might make my code more sensible if I could use an empty string.

cocoliliace commented 1 year ago

Do you want to remove a label? I don’t see why you don’t want a label?

Actually, I didn't read carefully. I just realized you were talking about footnoteLabel being a label, as documented. In that case maybe there's a different bug because it's showing up as the text of the node rather than the label:

<section data-footnotes="" class="footnotes"><h2 class="sr-only" id="footnote-label">Footnotes</h2>
<ol>
<li id="user-content-fn-1">

The examples on README.md also show it as visible text rather than aria-label.

wooorm commented 1 year ago

For a blog post, at least, I feel the text "Footnote" is unnecessary since the positioning of the text already conveys that they are footnotes.

This is why a sr-only is added. Which is what GH does. It provides, with CSS as described in the readme, and accessible label to screen readers.

"The hr element represents a paragraph-level thematic break, e.g., a scene change in a story, or a transition to another topic within a section of a reference book". Does that not apply to footnotes?

I wonder: the examples it shows is only between “paragraphs” (a concept broader than just p). Between means there’s something before and after. And a scene change / transition also needs something before and after. In this case, we already have a <section> element making a new section. I don’t think a hr at the start of a section makes sense semantically, because the <section> is also such a break!

When I use a space character " ", for footnoteLabel, I get the following html for my compiled page:

What you are seeing is a fail-safe: when we generate HTML, and we see a “void” element (that cannot have children or closing tag), but there are children, we emit the children. This is because there is one case where W3C and WhatWG used to differ, and to show bugs in code instead of silently drop them:

https://github.com/syntax-tree/hast-util-to-html/blob/3c9469abbb1ddd576e93387e37434c0f4d1db6ef/lib/handle/element.js#L88-L92

You are correct that setting '' does not output anything there! However, the AST still has a child node of {type: 'text', value: ''}. And that child node doesn’t make sense there either. So, if this feature request was going to land (depends on the other outstanding Qs), I’d want to remove that child text too!

I think your main thing is that you don’t want to visually show giant “Footnotes” text. That’s fine, GH doesn’t either. Do you think it’s okay to apply the CSS recommended? (https://github.com/syntax-tree/mdast-util-to-hast#css)

cocoliliace commented 1 year ago

Thank you for the explanation. I'm convinced now and will be using border-top instead of an <hr>, and hiding .sr-only with css. Though do you think it'd be better to place the footnoteLabel as an aria-label on the <section> tag instead of making a heading tag and hiding it?

https://stackoverflow.com/a/26032207/10254049

wooorm commented 1 year ago

Glad that works for you!

Some more references:

Though do you think it'd be better to place the footnoteLabel as an aria-label on the <section> tag instead of making a heading tag and hiding it?

I don’t think that works! aria-label replaces the element, for ARIA users, with the value from that attribute. So it would remove the entire footnotes for screenreader users.

GH does use it for the backlinks: <a … aria-label="Back to content">↩</a>, where sighted users see ↩ but screenreaders see Back to content

I think I can close this now, right? Feel free to keep on discussing below if you’d like!

github-actions[bot] commented 1 year ago

Hi! This was closed. Team: If this was merged, please describe when this is likely to be released. Otherwise, please add one of the no/* labels.

cocoliliace commented 1 year ago

I don’t think that works! aria-label replaces the element, for ARIA users, with the value from that attribute. So it would remove the entire footnotes for screenreader users.

Ah that makes sense. Thank you for the explanation and all the resources. Very helpful!

I think I can close this now, right?

Yep no further questions thank you :)