diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js
index 76f3c6506eddc2f166a9fb6ede7199f1bfac5faf..cfab4721f4b09689209105aa043407cafc2d05ae 100644
--- a/app/assets/javascripts/application.js
+++ b/app/assets/javascripts/application.js
@@ -57,32 +57,11 @@
(function () {
document.addEventListener('page:fetch', gl.utils.cleanupBeforeFetch);
- window.addEventListener('hashchange', gl.utils.shiftWindow);
-
- // automatically adjust scroll position for hash urls taking the height of the navbar into account
- // https://github.com/twitter/bootstrap/issues/1768
- window.adjustScroll = function() {
- var navbar = document.querySelector('.navbar-gitlab');
- var subnav = document.querySelector('.layout-nav');
- var fixedTabs = document.querySelector('.js-tabs-affix');
-
- adjustment = 0;
- if (navbar) adjustment -= navbar.offsetHeight;
- if (subnav) adjustment -= subnav.offsetHeight;
- if (fixedTabs) adjustment -= fixedTabs.offsetHeight;
-
- return scrollBy(0, adjustment);
- };
-
- window.addEventListener("hashchange", adjustScroll);
-
- window.onload = function () {
- // Scroll the window to avoid the topnav bar
- // https://github.com/twitter/bootstrap/issues/1768
- if (location.hash) {
- return setTimeout(adjustScroll, 100);
- }
- };
+ window.addEventListener('hashchange', gl.utils.handleLocationHash);
+ window.addEventListener('load', function onLoad() {
+ window.removeEventListener('load', onLoad, false);
+ gl.utils.handleLocationHash();
+ }, false);
$(function () {
var $body = $('body');
diff --git a/app/assets/javascripts/lib/utils/common_utils.js b/app/assets/javascripts/lib/utils/common_utils.js
index d83c41fae9d7a664a2cf956012793b5f829216ef..c5846068b0737334b46549db28529c5570238ac0 100644
--- a/app/assets/javascripts/lib/utils/common_utils.js
+++ b/app/assets/javascripts/lib/utils/common_utils.js
@@ -1,4 +1,4 @@
-/* eslint-disable func-names, space-before-function-paren, wrap-iife, no-var, no-unused-expressions, no-param-reassign, no-else-return, quotes, object-shorthand, comma-dangle, camelcase, one-var, vars-on-top, one-var-declaration-per-line, no-return-assign, consistent-return, padded-blocks, max-len */
+/* eslint-disable func-names, space-before-function-paren, wrap-iife, no-var, no-unused-expressions, no-param-reassign, no-else-return, quotes, object-shorthand, comma-dangle, camelcase, one-var, vars-on-top, one-var-declaration-per-line, no-return-assign, consistent-return, padded-blocks, max-len, prefer-template */
(function() {
(function(w) {
var base;
@@ -94,10 +94,35 @@
return $(document).off('scroll');
};
- w.gl.utils.shiftWindow = function() {
- return w.scrollBy(0, -100);
- };
+ // automatically adjust scroll position for hash urls taking the height of the navbar into account
+ // https://github.com/twitter/bootstrap/issues/1768
+ w.gl.utils.handleLocationHash = function() {
+ var hash = w.gl.utils.getLocationHash();
+ if (!hash) return;
+
+ var navbar = document.querySelector('.navbar-gitlab');
+ var subnav = document.querySelector('.layout-nav');
+ var fixedTabs = document.querySelector('.js-tabs-affix');
+ var adjustment = 0;
+ if (navbar) adjustment -= navbar.offsetHeight;
+ if (subnav) adjustment -= subnav.offsetHeight;
+
+ // scroll to user-generated markdown anchor if we cannot find a match
+ if (document.getElementById(hash) === null) {
+ var target = document.getElementById('user-content-' + hash);
+ if (target && target.scrollIntoView) {
+ target.scrollIntoView(true);
+ window.scrollBy(0, adjustment);
+ }
+ } else {
+ // only adjust for fixedTabs when not targeting user-generated content
+ if (fixedTabs) {
+ adjustment -= fixedTabs.offsetHeight;
+ }
+ window.scrollBy(0, adjustment);
+ }
+ };
gl.utils.updateTooltipTitle = function($tooltipEl, newTitle) {
return $tooltipEl.tooltip('destroy').attr('title', newTitle).tooltip('fixTitle');
diff --git a/app/assets/stylesheets/framework/typography.scss b/app/assets/stylesheets/framework/typography.scss
index 070e42d63d2e7b33b47b397a2320300aa0a87ed7..aa604b1cd19420868cbc82275e311db6090af496 100644
--- a/app/assets/stylesheets/framework/typography.scss
+++ b/app/assets/stylesheets/framework/typography.scss
@@ -182,6 +182,7 @@
left: -16px;
position: absolute;
text-decoration: none;
+ outline: none;
&::after {
content: image-url('icon_anchor.svg');
diff --git a/changelogs/unreleased/22781-user-generated-permalinks.yml b/changelogs/unreleased/22781-user-generated-permalinks.yml
new file mode 100644
index 0000000000000000000000000000000000000000..e46739e48e371c8d6bfa78d549cc849a52898eeb
--- /dev/null
+++ b/changelogs/unreleased/22781-user-generated-permalinks.yml
@@ -0,0 +1,4 @@
+---
+title: Prevent DOM ID collisions resulting from user-generated content anchors
+merge_request: 7631
+author:
diff --git a/doc/development/gotchas.md b/doc/development/gotchas.md
index 7bfc9cb361f3373c9164a6e4513a960778dd5b09..0f78e8238af6bf8ce70ff8a3fbf4bdd54bb73b58 100644
--- a/doc/development/gotchas.md
+++ b/doc/development/gotchas.md
@@ -141,51 +141,3 @@ in an initializer._
### Further reading
- Stack Overflow: [Why you should not write inline JavaScript](http://programmers.stackexchange.com/questions/86589/why-should-i-avoid-inline-scripting)
-
-## ID-based CSS selectors need to be a bit more specific
-
-Normally, because HTML `id` attributes need to be unique to the page, it's
-perfectly fine to write some JavaScript like the following:
-
-```javascript
-$('#js-my-selector').hide();
-```
-
-However, there's a feature of GitLab's Markdown processing that [automatically
-adds anchors to header elements][ToC Processing], with the `id` attribute being
-automatically generated based on the content of the header.
-
-Unfortunately, this feature makes it possible for user-generated content to
-create a header element with the same `id` attribute we're using in our
-selector, potentially breaking the JavaScript behavior. A user could break the
-above example with the following Markdown:
-
-```markdown
-## JS My Selector
-```
-
-Which gets converted to the following HTML:
-
-```html
-<h2>
- <a id="js-my-selector" class="anchor" href="#js-my-selector" aria-hidden="true"></a>
- JS My Selector
-</h2>
-```
-
-[ToC Processing]: https://gitlab.com/gitlab-org/gitlab-ce/blob/8-4-stable/lib/banzai/filter/table_of_contents_filter.rb#L31-37
-
-### Solution
-
-The current recommended fix for this is to make our selectors slightly more
-specific:
-
-```javascript
-$('div#js-my-selector').hide();
-```
-
-### Further reading
-
-- Issue: [Merge request ToC anchor conflicts with tabs](https://gitlab.com/gitlab-org/gitlab-ce/issues/3908)
-- Merge Request: [Make tab target selectors less naive](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/2023)
-- Merge Request: [Make cross-project reference's clipboard target less naive](https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/2024)
diff --git a/features/steps/shared/markdown.rb b/features/steps/shared/markdown.rb
index 56b36f7c46c4773242ef54bb7448b1ad3d2f0762..a036d9b884fbe685cf13ebac10860c04c4c30539 100644
--- a/features/steps/shared/markdown.rb
+++ b/features/steps/shared/markdown.rb
@@ -2,7 +2,7 @@ module SharedMarkdown
include Spinach::DSL
def header_should_have_correct_id_and_link(level, text, id, parent = ".wiki")
- node = find("#{parent} h#{level} a##{id}")
+ node = find("#{parent} h#{level} a#user-content-#{id}")
expect(node[:href]).to eq "##{id}"
# Work around a weird Capybara behavior where calling `parent` on a node
diff --git a/lib/banzai/filter/table_of_contents_filter.rb b/lib/banzai/filter/table_of_contents_filter.rb
index a4eda6fdf7685e6092189fbd3a8d928c9ad4173f..8e7084f2543649e9bf7a6e737f068afbeeb23510 100644
--- a/lib/banzai/filter/table_of_contents_filter.rb
+++ b/lib/banzai/filter/table_of_contents_filter.rb
@@ -35,9 +35,11 @@ module Banzai
headers[id] += 1
if header_content = node.children.first
+ # namespace detection will be automatically handled via javascript (see issue #22781)
+ namespace = "user-content-"
href = "#{id}#{uniq}"
push_toc(href, text)
- header_content.add_previous_sibling(anchor_tag(href))
+ header_content.add_previous_sibling(anchor_tag("#{namespace}#{href}", href))
end
end
@@ -48,8 +50,8 @@ module Banzai
private
- def anchor_tag(href)
- %Q{<a id="#{href}" class="anchor" href="##{href}" aria-hidden="true"></a>}
+ def anchor_tag(id, href)
+ %Q{<a id="#{id}" class="anchor" href="##{href}" aria-hidden="true"></a>}
end
def push_toc(href, text)
diff --git a/spec/lib/banzai/filter/table_of_contents_filter_spec.rb b/spec/lib/banzai/filter/table_of_contents_filter_spec.rb
index 356dd01a03a81014ecaac3a92792f215c58bddfe..70b31f3a880496b17378009f280f84a907998b3b 100644
--- a/spec/lib/banzai/filter/table_of_contents_filter_spec.rb
+++ b/spec/lib/banzai/filter/table_of_contents_filter_spec.rb
@@ -22,7 +22,7 @@ describe Banzai::Filter::TableOfContentsFilter, lib: true do
html = header(i, "Header #{i}")
doc = filter(html)
- expect(doc.css("h#{i} a").first.attr('id')).to eq "header-#{i}"
+ expect(doc.css("h#{i} a").first.attr('id')).to eq "user-content-header-#{i}"
end
end
@@ -32,7 +32,12 @@ describe Banzai::Filter::TableOfContentsFilter, lib: true do
expect(doc.css('h1 a').first.attr('class')).to eq 'anchor'
end
- it 'links to the id' do
+ it 'has a namespaced id' do
+ doc = filter(header(1, 'Header'))
+ expect(doc.css('h1 a').first.attr('id')).to eq 'user-content-header'
+ end
+
+ it 'links to the non-namespaced id' do
doc = filter(header(1, 'Header'))
expect(doc.css('h1 a').first.attr('href')).to eq '#header'
end
@@ -40,29 +45,29 @@ describe Banzai::Filter::TableOfContentsFilter, lib: true do
describe 'generated IDs' do
it 'translates spaces to dashes' do
doc = filter(header(1, 'This header has spaces in it'))
- expect(doc.css('h1 a').first.attr('id')).to eq 'this-header-has-spaces-in-it'
+ expect(doc.css('h1 a').first.attr('href')).to eq '#this-header-has-spaces-in-it'
end
it 'squeezes multiple spaces and dashes' do
doc = filter(header(1, 'This---header is poorly-formatted'))
- expect(doc.css('h1 a').first.attr('id')).to eq 'this-header-is-poorly-formatted'
+ expect(doc.css('h1 a').first.attr('href')).to eq '#this-header-is-poorly-formatted'
end
it 'removes punctuation' do
doc = filter(header(1, "This, header! is, filled. with @ punctuation?"))
- expect(doc.css('h1 a').first.attr('id')).to eq 'this-header-is-filled-with-punctuation'
+ expect(doc.css('h1 a').first.attr('href')).to eq '#this-header-is-filled-with-punctuation'
end
it 'appends a unique number to duplicates' do
doc = filter(header(1, 'One') + header(2, 'One'))
- expect(doc.css('h1 a').first.attr('id')).to eq 'one'
- expect(doc.css('h2 a').first.attr('id')).to eq 'one-1'
+ expect(doc.css('h1 a').first.attr('href')).to eq '#one'
+ expect(doc.css('h2 a').first.attr('href')).to eq '#one-1'
end
it 'supports Unicode' do
doc = filter(header(1, '한글'))
- expect(doc.css('h1 a').first.attr('id')).to eq '한글'
+ expect(doc.css('h1 a').first.attr('id')).to eq 'user-content-한글'
expect(doc.css('h1 a').first.attr('href')).to eq '#한글'
end
end
diff --git a/spec/support/matchers/markdown_matchers.rb b/spec/support/matchers/markdown_matchers.rb
index 8c98b1f988cb46df98ed1c1d351f899375bd1d83..97b8b342eb22a46bed8383a62213a8880c6269f1 100644
--- a/spec/support/matchers/markdown_matchers.rb
+++ b/spec/support/matchers/markdown_matchers.rb
@@ -38,9 +38,9 @@ module MarkdownMatchers
set_default_markdown_messages
match do |actual|
- expect(actual).to have_selector('h1 a#gitlab-markdown')
- expect(actual).to have_selector('h2 a#markdown')
- expect(actual).to have_selector('h3 a#autolinkfilter')
+ expect(actual).to have_selector('h1 a#user-content-gitlab-markdown')
+ expect(actual).to have_selector('h2 a#user-content-markdown')
+ expect(actual).to have_selector('h3 a#user-content-autolinkfilter')
end
end