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

be able to mimic the new heading structure used by github #20

Closed
4 tasks done
chrisweb opened this issue Oct 27, 2023 · 5 comments
Closed
4 tasks done

be able to mimic the new heading structure used by github #20

chrisweb opened this issue Oct 27, 2023 · 5 comments
Labels
💪 phase/solved Post is done

Comments

@chrisweb
Copy link
Contributor

Initial checklist

Problem

I tried to reproduce the new headings that github is now using, with an anchor inside of the heading element and inside of the anchor the heading text and an icon:

<h2 tabindex="-1" id="example" dir="auto">
	<a class="heading-link" href="#example">
		Example<svg class="octicon octicon-link" viewBox="0 0 16 16" width="16" height="16" aria-hidden="true"></svg>
	</a>
</h2>

However if I set the behavior to "wrap" it creates a heading with an anchor element inside which solves the first part of the problem but the second part of the problem can't be solved as I can not use content to create the icon when the behavior is "wrap"

So as of today it seems like there is no way of recreating what github does using rehype-autolink-headings, or did I miss something?

Solution

From the 4 alternatives, I like the last suggestion best as it is not a breaking change and it gives the devs full control about what ever they want to put into the heading element and it only introduces one new behavior value

Oh actually there is one more thing I didn't mention yet, rehype-autolink-headings has a "properties" option that allows to change the attributes of the anchor element, when the content was an icon it made sense to have for example a tabIndex set to "-1", but I was wondering if it was still making sense if the text of the heading is inside the anchor too and not just the icon?

This is why a had another look at the github markup I posted above, and that's when I noticed that they added the tabIndex attribute to the heading element and not the anchor element (meaning the actual anchor in the heading is gets focused when pressing the tab key, as it has no tabIndex), I assume that all the changes github did, are to make headings more accessible, but the tabIndex on the heading seems like it is a bug, I mean heading elements have no tabIndex so why would you want to set it to "-1" to disable it? I checked out the MDN tabIndex page and it confirmed my assumption that browsers don't add a tanIndex to headings. The tabIndex would only make sense it the heading element itself got converted into an anchor element that uses the aria heading role to mimic the behavior of a heading element, so I tried out the new github accessibility feature that they call Underline Links in Text Blocks Public Beta to see if it has an impact on headings, if I turn the new feature on, headings are not underlined at all, however if I turn it off (old behavior) headings are underlined on hover, but at no point the heading element is replaced by an anchor element, so still no clue why there is a tabIndex attribute on the heading element.

Putting aside githubs behavior for now, I was wondering if the properties option should get used by the new behavior and be applied to the anchor? The current default value for the properties options is {ariaHidden: 'true', tabIndex: -1}. But if the behavior is set to the new "substitute", then applying those two values to the link would not be wrong. I think the aria-hidden should be an attribute of the icon and not the anchor (as only the icon is decorative, but not the text), the tabIndex="-1" should still get applied to the anchor, because the anchor in the heading should not be focusable (as opposed to what github currently does). My logic is that an anchor has a tabIndex by default because it brings the user to a new part of a page or a completely new page, however the anchor in a heading does not have that purpose (it just changes the URL in the address bar so that it can be shared), from an accessibility point of view the anchors in the toc (table of contents) should be the ones that get used to move to parts of the page. This is why I think that it makes sense to change the default value of the properties to {tabIndex="-1"} when the behavior is set to substitute.

Alternatives

Here is a list of solutions I could think of, maybe there is one you like best:

  • First I thought about how the current code could get changed, so that "wrap" accepts content, but I guess this would introduce a breaking change if a dev has set content to something and has set the behavior to "wrap" (which today ignores the content)
  • Then I thought, well how about creating 2 new behaviors, for example a behavior called "wrappedAppend" and "wrappedPrepend" (not sure about the naming) where the heading text + content get wrapped, the difference being that wrappedAppend puts the content after the heading text and a wrappedPrepend puts the content before the text and then the text and content get both wrapped inside of a single anchor
  • Another way of solving it, could be to use the behavior "wrap", but introduce a new option called "withText" (or maybe called "addText", the opposite "omitText"...), the option would have as value true by default but if the dev sets it to false the text of the heading is not inserted automatically, meaning that "wrap" would only contain the content, but then there is potentially a new problem because the behaviors "append" and "prepend" are ambiguous (as there is no text to append / prepend), which means that if the option withText = false then "append", "prepend" as well as "wrap" would yield the same result
  • Finally I wondered if a single new "behavior" could be a better solution, a behavior for example called "substitute" (or maybe called "onlyContent", "inside"...), where only the content gets inserted into the heading element but not the heading text is not automatically inserted too, basically it is substituting the headings current content with what you define in the content function, that way you could use the content function to create the anchor and add the node text as well as the icon inside of that anchor
@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 Oct 27, 2023
@chrisweb
Copy link
Contributor Author

I did a small PR #21 of what I'm currently using for my project, please let me know what you think of it and if you would be interested of merging it, I would also be willing to make modifications if needed

@ZachSaucier
Copy link

As an alternative I did this:

behavior: 'wrap',
properties: {
  className: ['section_heading'],
},

Then added the icon with CSS:

.section_heading::after {
  content: url("data:image/svg+xml,%3Csvg xmlns='http://www.w3.org/2000/svg' viewBox='0 0 16 16' width='18' aria-hidden='true'%3E%3Cpath fill='%23000' d='m7.775 3.275 1.25-1.25a3.5 3.5 0 1 1 4.95 4.95l-2.5 2.5a3.5 3.5 0 0 1-4.95 0 .751.751 0 0 1 .018-1.042.751.751 0 0 1 1.042-.018 1.998 1.998 0 0 0 2.83 0l2.5-2.5a2.002 2.002 0 0 0-2.83-2.83l-1.25 1.25a.751.751 0 0 1-1.042-.018.751.751 0 0 1-.018-1.042Zm-4.69 9.64a1.998 1.998 0 0 0 2.83 0l1.25-1.25a.751.751 0 0 1 1.042.018.751.751 0 0 1 .018 1.042l-1.25 1.25a3.5 3.5 0 1 1-4.95-4.95l2.5-2.5a3.5 3.5 0 0 1 4.95 0 .751.751 0 0 1-.018 1.042.751.751 0 0 1-1.042.018 1.998 1.998 0 0 0-2.83 0l-2.5 2.5a1.998 1.998 0 0 0 0 2.83Z'%3E%3C/path%3E%3C/svg%3E");
  width: 18px;
  display: inline-block;
  margin-left: 8px;
}

@chrisweb
Copy link
Contributor Author

chrisweb commented Nov 7, 2023

Yes this works great 👍 , I mean if you are ok with having the svg in your css than this solution is valid too, it is just that if it is possible to extend the wrap functionality or add a new "behavior" type, then your solution will still work but you will also be able to do it without css, which would be great to have that choice

if you look at the comments in the PR, you will find a suggestion by wooorm which works really well, it does not use a new behavior but instead add the possibility to use content with the "wrap" behavior, which solves this case and even might solve other uses cases someone else might come up with

wooorm added a commit that referenced this issue Nov 8, 2023
Related-to: GH-20.
Related-to: GH-21.

Co-authored-by: Chris Weber <chris963@gmail.com>
@wooorm wooorm closed this as completed in 01133a3 Nov 8, 2023

This comment has been minimized.

@wooorm wooorm added the 💪 phase/solved Post is done label Nov 8, 2023
@github-actions github-actions bot removed the 🤞 phase/open Post is being triaged manually label Nov 8, 2023
@wooorm
Copy link
Member

wooorm commented Nov 8, 2023

This should now do the trick:

import {s} from 'hastscript'
import {rehype} from 'rehype'
import rehypeAutolinkHeadings from 'rehype-autolink-headings'
import rehypeSlug from 'rehype-slug'
import {read} from 'to-vfile'

const file = await rehype()
  .data('settings', {fragment: true})
  .use(rehypeSlug)
  .use(rehypeAutolinkHeadings, {
    behavior: 'wrap',
    content: s(
      'svg.octicon.octicon-link',
      {
        viewBox: '0 0 16 16',
        version: '1.1',
        width: 16,
        height: 16,
        ariaHidden: 'true'
      },
      s('path', {
        d: 'm7.775 3.275 1.25-1.25a3.5 3.5 0 1 1 4.95 4.95l-2.5 2.5a3.5 3.5 0 0 1-4.95 0 .751.751 0 0 1 .018-1.042.751.751 0 0 1 1.042-.018 1.998 1.998 0 0 0 2.83 0l2.5-2.5a2.002 2.002 0 0 0-2.83-2.83l-1.25 1.25a.751.751 0 0 1-1.042-.018.751.751 0 0 1-.018-1.042Zm-4.69 9.64a1.998 1.998 0 0 0 2.83 0l1.25-1.25a.751.751 0 0 1 1.042.018.751.751 0 0 1 .018 1.042l-1.25 1.25a3.5 3.5 0 1 1-4.95-4.95l2.5-2.5a3.5 3.5 0 0 1 4.95 0 .751.751 0 0 1-.018 1.042.751.751 0 0 1-1.042.018 1.998 1.998 0 0 0-2.83 0l-2.5 2.5a1.998 1.998 0 0 0 0 2.83Z'
      })
    ),
    headingProperties: {tabIndex: '-1', dir: 'auto'},
    properties: {className: ['heading-link']}
  })
  .process(await read('example.html'))

console.log(String(file))

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

No branches or pull requests

3 participants