Skip to content
This repository has been archived by the owner on Sep 7, 2021. It is now read-only.

Add warning for h2[name] elements #516

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ddbeck
Copy link
Contributor

@ddbeck ddbeck commented Jul 17, 2020

If Kuma is going to do weird things with name attributes, I figured it wouldn't be a bad idea to warn about it.

@ddbeck ddbeck requested a review from wbamberg July 17, 2020 11:10
@ddbeck
Copy link
Contributor Author

ddbeck commented Jul 17, 2020

At least for the time being, you can see how this works on a real page: text-rendering.

@ddbeck ddbeck force-pushed the h2-name-attribute-warning branch from 849d14f to df0874d Compare July 23, 2020 15:40
@ddbeck
Copy link
Contributor Author

ddbeck commented Jul 23, 2020

In our most recent call, Will made a good comment about how we shouldn't have to fight with fussy source stuff. With that in mind, I changed this warn only when a name attribute doesn't map to the innerText of the <h2> element.

@wbamberg
Copy link

Thanks for this, Daniel, and sorry to be slow reviewing.

This looks and seems to work great. But I'm not sure how it would be used.

Only logging a warning for a mismatch is not enough, I think - the problem with name is that if I'm making changes to lots of H2 headings (say, "Example" -> "Examples") then my changes will be undermined by Kuma's behaviour here even if the headings match.

So my first inclination is to log for any h2[name] occurrences. When I change your PR to do this, the number of warnings for Web/CSS goes from 12 to 311.

But even then, what am I supposed to do about these? Are we planning to have a task to fix them all manually? But I'm fixing them manually, anyway, when they give me problems fixing other errors. So I'm not sure who having a separate message is really useful here.

What I'd really like would be to fix Kuma's unhelpful behaviour :/. I've filed mdn/yari#6969.

@ddbeck
Copy link
Contributor Author

ddbeck commented Aug 5, 2020

My thinking here was to help you from seemingly "wrong" linter results caused by name attributes, rather than guard against all name attributes. Some examples:

Linter Source Commentary
⚠️ <h2 id="Example" name="Example">Examples</h2> This one is a problem because name has clobbered the ID. You'll get a missing ingredient, even though you've written the right heading. The PR as it stands helps with this.
⚠️ <h2 id="Examples" name="Examples">Example</h2> This one is a problem because name/id is correct but the text is wrong. This allows the ingredient check to pass, even though the text is incorrect. The PR as it stands helps with this.
<h2 id="Examples" name="Examples">Examples</h2> This one is harmless because everything—text, name, and id—is in agreement. That name exists is risky, but only hypothetically. The PR as it stands does not help with this.
<h2 id="Examples">Examples</h2> This is one is the ideal case: no dangerous name attribute and no problem. The PR as it stand (rightly) does not help with this.

You're suggesting making the third row a warning as well. The benefit of this is that you'd be warned before editing that name attributes might cause you problems. But that struck me as potentially fussy: if the name and text match up, it's not necessarily a content problem (i.e., a missing ingredient). That is to say, under your suggested change, you can fail this linter rule without having failed the recipe.

I can happily revert my change to capture those, I just thought I'd call attention to what that implies. Another route might be to make the ingredient check more aggressive: instead of checking h2#Examples we could check for both h2#Examples and that the text literal inside h2#Examples is exactly "Examples".

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants