doc: properly mark up deprecation anchors

[aduh95]

Using empty <a> tags here have several downsides:

• It doesn't meet WCAG standards, so that may impact screenreader users.
• Visual side-effect on hover:

This PR uses the <i> tag instead, because it's the first one-letter tag I could think of.

Checklist

• make -j4 test (UNIX), or vcbuild test (Windows) passes
• documentation is changed or added
• commit message follows commit guidelines

[DerekNonGeneric]

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

[aduh95]

@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]

Although I'm opposed to using <i> tags in this situation, I also believe empty <a> tags are wrong too.

We've discussed an alternative that is more screen-reader friendly instead of perhaps what might've been a hack to begin with. All of this can be handled in the same PR, it's not that difficult.

@aduh95, please let me know if it you would prefer if I addressed my own concerns.

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

[aduh95]

@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.

[DerekNonGeneric]

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?

[aduh95]

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

node/tools/doc/html.js

Lines 350 to 368 in c2996ce

	const headingText = file.contents.slice(
	node.children[0].position.start.offset,
	node.position.end.offset).trim();
	const id = getId(`${realFilename}_${headingText}`, idCounters);
	const hasStability = node.stability !== undefined;
	toc += ' '.repeat((depth - 1) * 2) +
	(hasStability ? `* <span class="stability_${node.stability}">` : '* ') +
	`<a href="#${id}">${headingText}</a>${hasStability ? '</span>' : ''}\n`;
	let anchor =
	`<span><a class="mark" href="#${id}" id="${id}">#</a></span>`;
	if (realFilename === 'errors' && headingText.startsWith('ERR_')) {
	anchor += `<span><a class="mark" href="#${headingText}" ` +
	`id="${headingText}">#</a></span>`;
	}
	const api = headingText.replace(/^.*:\s+/, '').replace(/\(.*/, '');

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?

[DerekNonGeneric]

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]

@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.

[Trott]

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).

[DerekNonGeneric]

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. 🤔

[Trott]

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.

[Trott]

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?

[Trott]

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>

[DerekNonGeneric]

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.

[DerekNonGeneric]

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.

[Trott]

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?).

[Trott]

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

[Trott]

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.

[DerekNonGeneric]

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.

[DerekNonGeneric]

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

Regarding…

node/tools/doc/html.js

Lines 350 to 353 in c2996ce

	const headingText = file.contents.slice(
	node.children[0].position.start.offset,
	node.position.end.offset).trim();
	const id = getId(`${realFilename}_${headingText}`, idCounters);

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. 👍

[Trott]

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) 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).

I'd prefer not to introduce HTML markup directly into the headers as it adds unnecessary complexity.

@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.

[Trott]

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.

[DerekNonGeneric]

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.

[Trott]

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.

node/doc/api_assets/style.css

Line 67 in 6f0b87d

a,

[DerekNonGeneric]

Nice, thanks for fixing this. Sweet diff too.

[nodejs-github-bot]

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

[nodejs-github-bot]

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

[Trott]

Landed in fa105eb

Thanks for sticking with this one!