Merge branch '22781-user-generated-permalinks' into 'master'

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](https://github.com/gitlabhq/gitlabhq/blob/master/doc/user/markdown.md#header-ids-and-links) 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

![2016-11-21-13.00.28](/uploads/e3be2cd6a9142dfd6e64db5462a6aa76/2016-11-21-13.00.28.gif)

## After:

![2016-11-21-13.12.45](/uploads/f7ae3f3a30c91325eaa3665591b6a850/2016-11-21-13.12.45.gif)  
![2016-11-21-13.03.00](/uploads/3a6a782c081ecaa05b8781548d794909/2016-11-21-13.03.00.gif)  

## Does this MR meet the acceptance criteria?

- [x] [Changelog entry](https://docs.gitlab.com/ce/development/changelog.html) added
- Tests
  - [x] Added for this feature/bug
  - [ ] All builds are passing
- [x] Conform by the [merge request performance guides](http://docs.gitlab.com/ce/development/merge_request_performance_guidelines.html)
- [x] Conform by the [style guides](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CONTRIBUTING.md#style-guides)
- [x] Branch has no merge conflicts with `master` (if it does - rebase it please)
- [x] [Squashed related commits together](https://git-scm.com/book/en/Git-Tools-Rewriting-History#Squashing-Commits)

## What are the relevant issue numbers?

Closes #22781 

See also prior attempts to address this issue:
#3908, !2023, !2024


See merge request !7631

9 files changed, 61 insertions, 93 deletions

diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js
index 76f3c6506ed..cfab4721f4b 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 d83c41fae9d..c5846068b07 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 070e42d63d2..aa604b1cd19 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 00000000000..e46739e48e3
--- /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 7bfc9cb361f..0f78e8238af 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 56b36f7c46c..a036d9b884f 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 a4eda6fdf76..8e7084f2543 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 356dd01a03a..70b31f3a880 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 8c98b1f988c..97b8b342eb2 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