Hmm, I was under the impression that <i> was deprecated in favor of <em> in HTML5.

Would you mind double-checking that?

I'd be interested in knowing what both WHATWG and W3C have to say on the matter to ensure that this is technically correct.

Have you read this following as well?

https://www.w3.org/International/questions/qa-b-and-i-tags

@DerekNonGeneric https://www.w3.org/TR/html52/textlevel-semantics.html#the-i-element

The <i> element represents a span of text in an alternate voice or mood, or otherwise offset from the normal prose in a manner indicating a different quality of text, such as a taxonomic designation, a technical term, an idiomatic phrase from another language, transliteration, a thought, or a ship name in Western texts.

And <em>: https://www.w3.org/TR/html52/textlevel-semantics.html#elementdef-em

The <em> element represents stress emphasis of its contents.

None is deprecated, they are just two different elements. We definitely don't want to stress emphasis of those empty tags, so <em> is ruled out. <i> is not ideal either though, if someone wants to suggest something else please do.

Have you read this following as well?

That seems to be related to the use of those tags as presentational fix, but that's not what we're doing here.

@DerekNonGeneric let me rephrase your stand to check if we're on the same ground: the current state (using empty <a> tags) is not OK, but this PR currently doesn't really improve that (using empty <i> tags).

Would empty <span> tags be acceptable to you?

I feel that this type of hack is unnecessary and would be detrimental to start allowing

I'm not sure to know what you mean by that exactly, can elaborate on that? I guess that's referencing the discussion earlier on this thread, but I'm not sure which part.

What you proposed in #34955 (comment) is the de jure correct way of doing this. We don't need empty tags at all. As you mentioned, the only issue is the <code> <span> tags appearing in the table of contents hash fragment URLs. That's no biggie and can easily be addressed in this PR, no?

OK, so the building of the TOC items is done here if I’m not mistaken:

As you can see, it is not HTML aware, just manipulation of strings in plain JS. How would you do what you are proposing in a non-hacky way? I'm not familiar with this part of the code base, so maybe I'm missing something obvious?

Right, so the problem is the same problem we had been experiencing with code appearing in the URL fragments before. Take a look at what I wrote in #34240 (comment). @Trott knows exactly how that was handled, so please wait for him to reply here. I can't find the PR URL where that was taken care of. @aduh95, you may be able to find it in one of the previous Node release blog posts since I think it happened relatively recently. I think we should trace that PR and do the same for this situation.

@Trott knows exactly how that was handled, so please wait for him to reply here.

I don't recall, I'm afraid. I won't have time to look and try to refresh my memory tonight. I'll try to dig in tomorrow, but no guarantees. If someone else wants to do it, please don't wait for me.

Although if all you want to know is why <code> used to be used in the headers: It's because backticks didn't used to render anything in the headers. Now they do though, so please use them, unless there's a good reason to use <code> instead (which I can't think of one).

Well, we want to use <span> tags in the headers, which don't have a replacement Markdown character, so the PR I'm thinking of might not exactly address the same concern we have here. 🤔

Well, we want to use <span> tags in the headers, which don't have a replacement Markdown character, so the PR I'm thinking of might not exactly address the same concern we have here. 🤔

I think using <span> to assign an id is fine. It's suboptimal to have it just above the header perhaps, but I think we can live with that. And even if we don't want to live with it, maybe let's consider "moving away from <a>" and "getting the id into the header itself" separate issues and just do the former in this PR and tackle the latter in a subsequent PR?

Does that work?

I can think of multiple possibilities for moving the id attribute into the header. That might be a protracted discussion with some experimentation required. I don't think we should hold up "get rid of <a name="foo"></a>" for that.

I'm trying to find something that says that using <a> tags the way they are currently used is an accessibility issue, but I'm not finding it. Yes, an empty <a> tag that has an href attribute would present an accessibility challenge. But these are not links. They do not have href attributes. Am I wrong to think that they are likely fine from an accessibility perspective?

I suppose one option is to get rid of these things entirely and rely on the longer auto-generated ids. So instead of #DEP0004, we'd use #deprecations_dep0004_cryptostream_prototype_readystate.

<p><a id="DEP0049"></a></p>
--
  | <h3>DEP0049: <code>util.isFunction()</code><span><a class="mark" href="#deprecations_dep0049_util_isfunction" id="deprecations_dep0049_util_isfunction">#</a></span></h3>

Am I wrong to think that they are likely fine from an accessibility perspective?

Empty tags are a concern for https://validator.w3.org and from an accessibility perspective, I have no idea what a screen reader would w/ something like this.

I suppose one option is to get rid of these things entirely and rely on the longer auto-generated ids.

I imagine they were being used as permalinks at some point or elsewhere in the documentation. If I am mistaken, we could just remove them entirely, I have no problem w/ that.

Another alternative would be to just use the <h3> tag and add an ID since Markdown accepts HTML like that. That would probably be easiest and what I would probably prefer if it is safest that these IDs remain here at all.

Empty tags are a concern for https://validator.w3.org

That appears to be incorrect. https://validator.w3.org does not have a problem with our deprecations.html page other than a single name attribute that was fixed in 4332f73.

and from an accessibility perspective

Well, that's what I'm asking for: A citation for that. I can't find any indication that's a correct statement. I mean, empty <a> tags that are links? Yes, that's a problem. Empty <a> tags with no href attribute that will never be links and that are only intended to be placeholders so browsers can link to them in the doc? As far as I can tell, that's totally valid and most/all accessibility tools handle it perfectly OK.

I could be wrong, but can someone please provide a citation?

I have no idea what a screen reader would w/ something like this.

Pretty sure it would (correctly) ignore it (or maybe indicate it can be tabbed to?).

More evidence that we may be solving a non-problem: WebAIM WAVE does not flag this as an accessibility issue. Would seem to be pretty low-hanging fruit if it were a problem. https://wave.webaim.org/report#/https://nodejs.org/dist/latest-v14.x/docs/api/deprecations.html

More evidence that we may be solving a non-problem: WebAIM WAVE does not flag this as an accessibility issue. Would seem to be pretty low-hanging fruit if it were a problem. https://wave.webaim.org/report#/https://nodejs.org/dist/latest-v14.x/docs/api/deprecations.html

By the way, I've opened PRs for the 3 broken links reported by WAVE. If someone wants to handle the less trivial issue of resolving the lack of contrast for the stuff that ends up as red text in the syntax-highlighting, that would be most welcome.

Nice, good investigation. Up to @aduh95 as to whether or not to pursue this fix any further. Thanks for looking into this!

For the record: the table of contents generator is to blame here. It's broken for sure.

@aduh95, to address my remaining concerns in this PR, please see the diff I've left below.

Regarding…

Patch it this way…

- const headingText = file.contents.slice( 
+ let headingText = file.contents.slice( 
   node.children[0].position.start.offset, 
   node.position.end.offset).trim();
+ // Sanitize all HTML tags from heading text
+ headingText = headingText.replace(/<\/?[^>]+(>|$)/g, '');
 const id = getId(`${realFilename}_${headingText}`, idCounters); 

Then all that's left to do is what you proposed in #34955 (comment). You will have a pretty nice negative diff!

Do these remaining things and I will approve this PR. 👍

The visual side effect on hover can be handled via CSS. However, I'm not seeing the visual side effect. What browser/OS are you using and what element are you hovering over?

Seeing the hover effect now. Nice catch.

I personally would prefer to either leave the <a id="DEP0XXX"></a> tags (as I don't see any evidence they are an accessibility issue) […]

It may not be an accessibility issue, but that's just a fundamentally flawed way of using HTML in general. You're not supposed to open a tag and then close it w/ nothing inside of it. For instances where that is necessary, one should be using self-closing tags. Take the <br/> tag for example. It is self-closing, but don't take my word for it; see below.

All elements are identified by their tag name and are marked up using either start tags and end tags or self-closing tags. A start tag marks the beginning of an element, while an end tag marks the end. Start tags are delimited using angle brackets with the tag name and any attributes in between. End tags are delimited by angle brackets with a slash before the tag name.

A Self-closing tag is a special form of start tag with a slash immediately before the closing right angle bracket. These indicate that the element is to be closed immediately, and has no content.

Refs: https://dev.w3.org/html5/html-author/#tags

[…] or else remove them entirely (as I'm not sure they get used anywhere anyway and there's already an id attribute generated for the header elements automatically).

The id attribute automatically generated for the header elements doesn't support our use-case of including HTML tags. My opinion is that it is broken as this should certainly be supported.

@DerekNonGeneric I'd prefer we try not to parse HTML with regular expressions because it's not possible to do it in a completely reliable way. You need to use a proper parser.

We don't really need a completely reliable way, but I have no problem adding another dependency to the doctool, we do already use a few foreign dependencies for that tool anyways.

The visual side effect on hover can be handled via CSS. However, I'm not seeing the visual side effect. What browser/OS are you using and what element are you hovering over?

Seeing the hover effect now. Nice catch.

I think the correct solution here is to remove this line as a:link on the next line will style all the links but leave these placeholder hash link thingies alone.

The visual side effect on hover can be handled via CSS. However, I'm not seeing the visual side effect.

Sorry, I don't follow. What are you referring to here? (I don't think this is a stylistic concern is it?)

@aduh95, we want them to have both of these IDs, not one or the other. So we need to keep the slug one too.

@DerekNonGeneric What do you mean by that? An HTML element cannot have several id attributes, I'm a bit confused here. Can you give an HTML code example of the solution you have in mind?

I haven't tested it yet, but can you just ensure that the heading output (the built docs) end up looking like this?

<h3 id="DEP0001">DEP0001: <code>http.OutgoingMessage.prototype.flush</code><span><a class="mark" href="#deprecations_dep0001_http_outgoingmessage_prototype_flush" id="deprecations_dep0001_http_outgoingmessage_prototype_flush">#</a></span></h3>

It's possible that your code is already working as expected due to the ID in the mark at the end of that line.

<a class="mark" href="#deprecations_dep0001_http_outgoingmessage_prototype_flush" id="deprecations_dep0001_http_outgoingmessage_prototype_flush">#</a>

Thanks, much clearer now! PTAL at the last commit:

• id attribute is added to the <h3> elements with the shorter DEPXXXX value.
• the # link anchor still has the classic longer id value (depxxxx_…) ensuring all links currently working continue to do so.
• the TOC links now use the shorter #DEPXXXX hash.

CI: https://ci.nodejs.org/job/node-test-pull-request/33029/

CI: https://ci.nodejs.org/job/node-test-pull-request/33049/

Landed in fa105eb

Thanks for sticking with this one!