Resolve "User-generated permalink IDs collide with GitLab interface"

What does this MR do?

Prevents ID values automatically generated by headers in GitLab Flavored Markdown from colliding with IDs used elsewhere in the GitLab interface. This can cause confusion when, for instance, a selector looks for a merge request tab with id="pipelines" and there is a header with the same ID earlier in the DOM.

How this works:

• All header IDs generated with GitLab Flavored Markdown are namespaced with id="user-content_foo"
• All anchor links which point to these IDs continue to use the non-namespaced hash <a href="#foo">...</a>
• When a page is loaded or when the hashchange event is triggered, javascript will automatically search for #user-content_foo if #foo cannot be found, and scroll to that position instead.

Before

After:

Does this MR meet the acceptance criteria?

• Changelog entry added
• Tests
  • Added for this feature/bug
  • All builds are passing
• Conform by the merge request performance guides
• Conform by the style guides
• Branch has no merge conflicts with master (if it does - rebase it please)
• Squashed related commits together

What are the relevant issue numbers?

Closes #22781 (closed)

See also prior attempts to address this issue: #3908 (closed), !2023 (merged), !2024 (merged)

Mentioned in commit f71a1bd1

Added 1 commit:

• f71a1bd1 - add CHANGELOG entry for !7631 (merged)

Compare with previous version

Marked the task Changelog entry added as completed

@mikegreiling can we add tests to this?

Added 2 commits:

• 1bff97a3 - remove id collision caveat from documentation
• a26cdd5c - update gitlab flavored markdown tests to reflect namespaced ids

Compare with previous version

Marked the task Added for this feature/bug as completed

Marked the task All builds are passing as completed

@ClemMakesApps done.

Also noticed this was mentioned in the documentation as a caveat for selecting IDs in frontend code. I removed this since it's no longer necessary...

Added 1 commit:

• 3900986b - update gitlab flavored markdown tests to reflect namespaced ids

Compare with previous version

Marked the task All builds are passing as incomplete

Mentioned in commit 2f9f539a

Added 4 commits:

• 23fc5c80 - add transparent namespace to all user-generated anchors in GitLab flavored markdown
• 2f9f539a - add CHANGELOG entry for !7631 (merged)
• afad3d97 - remove id collision caveat from documentation
• 4ed3e07b - update gitlab flavored markdown tests to reflect namespaced ids

Compare with previous version

@mikegreiling when I was testing this, I went to http://localhost:3000/gitlab-org/gitlab-ce#user-content_open-source-software-to-collaborate-on-code and it focused the anchor tag. Can we make it not outline the hidden anchor tag?

Reassigned to @mikegreiling

@ClemMakesApps yeah, I can do this. However, I don't think many people are going to ever see or use permalinks formatted with the namespace included like you did there, so I'm not sure how common this will be :)

I agree, probably very uncommon but wanted to make sure everything is

Added 1 commit:

• b9530852 - prevent anchor tag outline on :focus or :target pseudo-class

Compare with previous version

Added 1 commit:

• bca2893c - remove setTimeout wrapper for location hash correction

Compare with previous version

@ClemMakesApps I've addressed each of your suggestions except for the prefer-template ESLint rule.

I can rename the file to *.es6 and use template syntax, or I can leave it as-is. What do you think?

I really wish our ESLint rules could apply contextually to es6 and non-es6 files... rules like prefer-arrow-functions, no-var, comma-dangle, and prefer-templates have no solution outside of es6 scripts so they are somewhat useless here...

Mentioned in merge request !7051 (merged)

Mentioned in issue #24876 (closed)

Reassigned to @ClemMakesApps

@ClemMakesApps let me know if you have any more feedback, or you can pass it along to an endboss

Actually, I see @ClemMakesApps is on vacation from Wed-Fri this week... @fatihacet can you take this from here?

Reassigned to @fatihacet

Awesome! Thanks @mikegreiling!

@smcgivern can you take a look at the ruby code here?

Reassigned to @smcgivern

I don't like that we have this fallback case that works some of the time and not other times. I can probably live with it, but here's an alternative idea:

Keep the IDs for UGC as they are. Add gl_ to the start of IDs we are using internally. This is ugly, but users can't generate IDs containing an underscore, and the ugliness is restricted to us.

@fatihacet @mikegreiling et al, what do you think about this?

Reassigned to @mikegreiling

@smcgivern I think prefixing our own ids would be a difficult restriction to live with. I agree, we should try to prefix common words like pipeline, changes, discussion, etc as a best practice, so we can avoid conflicts with this fallback procedure, but ensuring we prefix absolutely everything would be a large undertaking. Every id we use in scss and our templates would have to change, and we'd have to modify any helper functions which auto-generate id values in forms, or any external libraries we use that rely on particular ids. It would be a large audit.

To me, prefixing the user-definable ids seems the cleaner option.

This is also exactly what GitHub is doing (not that we need to take cues from them, but it does seem to be working):

What do you think?

I look at it like sanitizing user content in the same way that we'd scrub user input for HTML or JavaScript injections...

@mikegreiling aren't we already supposed to do this with js- as a prefix? I guess that's just as much in favour of your position as mine, though - because that wouldn't work either (as the doc you've removed mentions)!

Basically, I don't think we necessarily need to do what GH does, and if we feel like we could do that in a way that let the ugliness only be visible to us, I'd prefer that. But you (and the other frontenders) know best here, so I'll just shut up and review the Ruby

Ruby LGTM, minus my quibbling!

Reassigned to @fatihacet

Thanks, @smcgivern

@fatihacet what do you think? Should we open this up to more discussion amongst the frontend team or should we merge it?

LGTM

Reassigned to @mikegreiling

Added 1 commit:

• 9924a388 - remove underscore from user-content id namespace

Compare with previous version

Mentioned in commit d4931a49

Added 555 commits:

• bca2893c...ef6a91af - 548 commits from branch master
• 3d7ae742 - remove duplicate functionality (bad merge conflict resolution?)
• 75288b61 - add transparent namespace to all user-generated anchors in GitLab flavored markdown
• d4931a49 - add CHANGELOG entry for !7631 (merged)
• 5da8a650 - remove id collision caveat from documentation
• 17323bfa - update gitlab flavored markdown tests to reflect namespaced ids
• 4e10e175 - prevent anchor tag outline on :focus or :target pseudo-class
• f35e9ede - remove setTimeout wrapper for location hash correction

Compare with previous version

Added 555 commits:

• bca2893c...ef6a91af - 548 commits from branch master
• 3d7ae742 - remove duplicate functionality (bad merge conflict resolution?)
• 75288b61 - add transparent namespace to all user-generated anchors in GitLab flavored markdown
• d4931a49 - add CHANGELOG entry for !7631 (merged)
• 5da8a650 - remove id collision caveat from documentation
• 17323bfa - update gitlab flavored markdown tests to reflect namespaced ids
• 4e10e175 - prevent anchor tag outline on :focus or :target pseudo-class
• f35e9ede - remove setTimeout wrapper for location hash correction

Compare with previous version

Mentioned in issue #25179 (closed)

Mentioned in merge request !7318 (merged)

Mentioned in commit 85422ea4

Added 101 commits:

• 9924a388...24e5a1e8 - 93 commits from branch master
• ffd28232 - remove duplicate functionality (bad merge conflict resolution?)
• ff2026f4 - add transparent namespace to all user-generated anchors in GitLab flavored markdown
• 85422ea4 - add CHANGELOG entry for !7631 (merged)
• 94ae12f6 - remove id collision caveat from documentation
• 567b8a99 - update gitlab flavored markdown tests to reflect namespaced ids
• 118ab549 - prevent anchor tag outline on :focus or :target pseudo-class
• 19f174bc - remove setTimeout wrapper for location hash correction
• 131a04d7 - remove underscore from user-content id namespace

Compare with previous version

Added 101 commits:

• 9924a388...24e5a1e8 - 93 commits from branch master
• ffd28232 - remove duplicate functionality (bad merge conflict resolution?)
• ff2026f4 - add transparent namespace to all user-generated anchors in GitLab flavored markdown
• 85422ea4 - add CHANGELOG entry for !7631 (merged)
• 94ae12f6 - remove id collision caveat from documentation
• 567b8a99 - update gitlab flavored markdown tests to reflect namespaced ids
• 118ab549 - prevent anchor tag outline on :focus or :target pseudo-class
• 19f174bc - remove setTimeout wrapper for location hash correction
• 131a04d7 - remove underscore from user-content id namespace

Compare with previous version

Build passing

Reassigned to @fatihacet

Resolved all discussions

Tested locally Thanks @mikegreiling

Resolved all discussions

Mentioned in commit 4d37be0f

Status changed to merged

This has ~"Pick into Stable" but no milestone assigned. Where should it go?

removed ~149423 label

changed milestone to %8.15

@mikegreiling should this be %8.15 or %8.16?

This was merged into master prior to %8.15's release so I presume it made it in 8.15.