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

header tags are html-rendered with name="" instead of id="" #135

Closed
dascritch opened this issue Aug 21, 2014 · 5 comments · Fixed by #140
Closed

header tags are html-rendered with name="" instead of id="" #135

dascritch opened this issue Aug 21, 2014 · 5 comments · Fixed by #140

Comments

@dascritch
Copy link

This bug was previously reported in markup repo
A section header in markdown is rendered as h3 > a[name]

To anchor an element in URL, the id attribute must be used

The name attribute is reserved for usage in form elements. Its availability as a id is a inheritance from Netscape days and should not have been used here.

Alas, as I read in your README.md, « Note that the id attribute is not whitelisted. »
So how can I patch this ?

dascritch added a commit to dascritch/html-pipeline that referenced this issue Aug 21, 2014
@mojavelinux
Copy link
Contributor

This is a long standing issue in the GitHub markup pipeline, so much so that I think this should conclude with at least a statement in the README as to why GitHub does what they do.

Intuition tells me that GitHub does it this way to avoid conflicts with CSS in user content. The challenge is that a markup file can have absolutely any heading, which means any anchor name. If assigned as an id attribute, there's a high likelyhood that one of the headings is going to match an id selector in the GitHub stylesheet and get styled in unexpected ways.

Watching GitHub work over the last few years, I've seen several strategies applied here to avoid conflicts, from dropping anchors outright to prefixing them with something like "user-content-". Every change seems to break something different.

From what I can see, the name is a reasonable compromise (though I'm not saying ideal). At least in Chrome and Firefox, the name attribute allows the anchor point to work. I'm sure there are browsers that don't support it, but it would be incorrect to say it doesn't work at all.

More transparency around decision making would be very welcomed here. I do understand the challenge that GitHub faces trying to avoid conflicts with open-ended content. Unless we all enjoy seeing the same issue get filed once a month, perhaps it's time to document the reason for why it is the way it is.

In other words, answer the question, "Why do we define heading anchors using the name attribute instead of the id attribute?"

@bkeepers
Copy link
Contributor

bkeepers commented Sep 3, 2014

To give some context and address the concerns that @mojavelinux raised…

TL;DR: Ideally, browsers would implement a way to deep-link to elements that doesn't clobber the DOM and have a negative impact on CSS and JavaScript, but they don't, so we've had to come up with workarounds.

We originally started sanitizing id to guard against user content clobbering elements used by CSS or JavaScript. For example, if we allowed <a id="new_repository">…</a> through, it would break the "New Repository" link on that page for any JavaScript-enabled browser. Allowing name was a less-than-ideal-but-acceptable workaround for enabling deep-linking within user content…until we realized that the name attribute clobbers the DOM. For example, <a name="addEventListener">…</a> would break event handling in JavaScript.

So a few months ago, we rolled out a change that prefixed all name elements with user-content-, and then use JavaScript to perform the actual scrolling to user content. For example <a name="example"></a> gets transformed into <a name="user-content-example"></a>, and whenever a page is loaded or the hash changes (e.g. https://github.com/jch/html-pipeline/issues/135#example), JavaScript looks for prefixed user content that matches the hash and scrolls to it.

While this again is not a perfect solution, but it has worked really well. It allows users to deep link to content without DOM clobbering. Using a similar technique, I think we can go back to allow id in user content.

See #111 and github/markup#219 (comment) for related discussions.

/cc @josh

@josh
Copy link
Contributor

josh commented Sep 3, 2014

I think adding back id support with that prefix would be legit. Pretty much address any security concerns around breaking QSA and overriding application defined behaviors built on ids.

bkeepers added a commit to bkeepers/html-pipeline that referenced this issue Sep 4, 2014
As gjtorikian#135 points out, `name`
is intended for form elements and is obsolete on anchors.
@jch jch closed this as completed in #140 Sep 5, 2014
@rlidwka
Copy link

rlidwka commented Nov 30, 2014

For example, <a name="addEventListener">…</a> would break event handling in JavaScript.

@bkeepers, what browsers does this issue come up in?

I face the similar problem with anchors now, and <a name="foo"> doesn't add anything to DOM in recent Chrome and FF versions I tested, only <form name="foo"> does that.

@bkeepers
Copy link
Contributor

bkeepers commented Dec 1, 2014

@rlidwka I think I experienced it in both Firefox and Chrome a few months ago. Maybe that has since changed. Our security team was able to make a page completely unusable by adding content with name attributes (I don't remember if that was addEventListener or something else).

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

Successfully merging a pull request may close this issue.

5 participants