From ffd28232610b87b3c738357655b95feedb3479d0 Mon Sep 17 00:00:00 2001
From: Mike Greiling <mike@pixelcog.com>
Date: Mon, 21 Nov 2016 12:18:49 -0600
Subject: [PATCH 1/8] remove duplicate functionality (bad merge conflict
resolution?)
---
app/assets/javascripts/application.js | 31 ++++---------------
.../javascripts/lib/utils/common_utils.js | 16 ++++++++--
2 files changed, 19 insertions(+), 28 deletions(-)
diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js
index 76f3c6506ed..ec6d7cad3ab 100644
--- a/app/assets/javascripts/application.js
+++ b/app/assets/javascripts/application.js
@@ -57,32 +57,13 @@
(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);
+ if (window.location.hash) {
+ setTimeout(gl.utils.handleLocationHash, 100);
}
- };
+ }, 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..b6b3bd18877 100644
--- a/app/assets/javascripts/lib/utils/common_utils.js
+++ b/app/assets/javascripts/lib/utils/common_utils.js
@@ -94,10 +94,20 @@
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 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;
+ if (fixedTabs) adjustment -= fixedTabs.offsetHeight;
+ window.scrollBy(0, adjustment);
+ };
gl.utils.updateTooltipTitle = function($tooltipEl, newTitle) {
return $tooltipEl.tooltip('destroy').attr('title', newTitle).tooltip('fixTitle');
--
GitLab
From ff2026f40ec0bc162dc4281b067ed4716b2ad248 Mon Sep 17 00:00:00 2001
From: Mike Greiling <mike@pixelcog.com>
Date: Mon, 21 Nov 2016 12:02:19 -0600
Subject: [PATCH 2/8] add transparent namespace to all user-generated anchors
in GitLab flavored markdown
---
.../javascripts/lib/utils/common_utils.js | 21 ++++++++++++++++---
lib/banzai/filter/table_of_contents_filter.rb | 8 ++++---
2 files changed, 23 insertions(+), 6 deletions(-)
diff --git a/app/assets/javascripts/lib/utils/common_utils.js b/app/assets/javascripts/lib/utils/common_utils.js
index b6b3bd18877..09b0a5eb9a5 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;
@@ -97,6 +97,9 @@
// 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');
@@ -104,9 +107,21 @@
var adjustment = 0;
if (navbar) adjustment -= navbar.offsetHeight;
if (subnav) adjustment -= subnav.offsetHeight;
- if (fixedTabs) adjustment -= fixedTabs.offsetHeight;
- window.scrollBy(0, adjustment);
+ // 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) {
diff --git a/lib/banzai/filter/table_of_contents_filter.rb b/lib/banzai/filter/table_of_contents_filter.rb
index a4eda6fdf76..80669953723 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)
--
GitLab
From 85422ea412ce1c3ea75562dfb008b6cdfaec8858 Mon Sep 17 00:00:00 2001
From: Mike Greiling <mike@pixelcog.com>
Date: Mon, 21 Nov 2016 13:27:11 -0600
Subject: [PATCH 3/8] add CHANGELOG entry for !7631
---
changelogs/unreleased/22781-user-generated-permalinks.yml | 4 ++++
1 file changed, 4 insertions(+)
create mode 100644 changelogs/unreleased/22781-user-generated-permalinks.yml
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:
--
GitLab
From 94ae12f6979db948687464e39f76a39fa5f43bae Mon Sep 17 00:00:00 2001
From: Mike Greiling <mike@pixelcog.com>
Date: Mon, 21 Nov 2016 13:41:15 -0600
Subject: [PATCH 4/8] remove id collision caveat from documentation
---
doc/development/gotchas.md | 48 --------------------------------------
1 file changed, 48 deletions(-)
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)
--
GitLab
From 567b8a99fe0e454a6d2c0b79cde86ee2dea806f3 Mon Sep 17 00:00:00 2001
From: Mike Greiling <mike@pixelcog.com>
Date: Mon, 21 Nov 2016 13:48:29 -0600
Subject: [PATCH 5/8] update gitlab flavored markdown tests to reflect
namespaced ids
---
features/steps/shared/markdown.rb | 2 +-
.../filter/table_of_contents_filter_spec.rb | 21 ++++++++++++-------
spec/support/matchers/markdown_matchers.rb | 6 +++---
3 files changed, 17 insertions(+), 12 deletions(-)
diff --git a/features/steps/shared/markdown.rb b/features/steps/shared/markdown.rb
index 56b36f7c46c..d355913a7ce 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/spec/lib/banzai/filter/table_of_contents_filter_spec.rb b/spec/lib/banzai/filter/table_of_contents_filter_spec.rb
index 356dd01a03a..e551a7d0ca5 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..a44bd2601c1 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
--
GitLab
From 118ab549147684d86af39fd9229789e6b15d09a7 Mon Sep 17 00:00:00 2001
From: Mike Greiling <mike@pixelcog.com>
Date: Tue, 22 Nov 2016 16:11:12 -0600
Subject: [PATCH 6/8] prevent anchor tag outline on :focus or :target
pseudo-class
---
app/assets/stylesheets/framework/typography.scss | 1 +
1 file changed, 1 insertion(+)
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');
--
GitLab
From 19f174bc68e0dc17e0298d8903bc855868334776 Mon Sep 17 00:00:00 2001
From: Mike Greiling <mike@pixelcog.com>
Date: Tue, 22 Nov 2016 17:23:34 -0600
Subject: [PATCH 7/8] remove setTimeout wrapper for location hash correction
---
app/assets/javascripts/application.js | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/app/assets/javascripts/application.js b/app/assets/javascripts/application.js
index ec6d7cad3ab..cfab4721f4b 100644
--- a/app/assets/javascripts/application.js
+++ b/app/assets/javascripts/application.js
@@ -60,9 +60,7 @@
window.addEventListener('hashchange', gl.utils.handleLocationHash);
window.addEventListener('load', function onLoad() {
window.removeEventListener('load', onLoad, false);
- if (window.location.hash) {
- setTimeout(gl.utils.handleLocationHash, 100);
- }
+ gl.utils.handleLocationHash();
}, false);
$(function () {
--
GitLab
From 131a04d7962b01daa58b8e5efe3f1359a3e73fe1 Mon Sep 17 00:00:00 2001
From: Mike Greiling <mike@pixelcog.com>
Date: Tue, 29 Nov 2016 13:07:38 -0600
Subject: [PATCH 8/8] remove underscore from user-content id namespace
---
app/assets/javascripts/lib/utils/common_utils.js | 2 +-
features/steps/shared/markdown.rb | 2 +-
lib/banzai/filter/table_of_contents_filter.rb | 2 +-
spec/lib/banzai/filter/table_of_contents_filter_spec.rb | 6 +++---
spec/support/matchers/markdown_matchers.rb | 6 +++---
5 files changed, 9 insertions(+), 9 deletions(-)
diff --git a/app/assets/javascripts/lib/utils/common_utils.js b/app/assets/javascripts/lib/utils/common_utils.js
index 09b0a5eb9a5..c5846068b07 100644
--- a/app/assets/javascripts/lib/utils/common_utils.js
+++ b/app/assets/javascripts/lib/utils/common_utils.js
@@ -110,7 +110,7 @@
// scroll to user-generated markdown anchor if we cannot find a match
if (document.getElementById(hash) === null) {
- var target = document.getElementById('user-content_' + hash);
+ var target = document.getElementById('user-content-' + hash);
if (target && target.scrollIntoView) {
target.scrollIntoView(true);
window.scrollBy(0, adjustment);
diff --git a/features/steps/shared/markdown.rb b/features/steps/shared/markdown.rb
index d355913a7ce..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#user-content_#{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 80669953723..8e7084f2543 100644
--- a/lib/banzai/filter/table_of_contents_filter.rb
+++ b/lib/banzai/filter/table_of_contents_filter.rb
@@ -36,7 +36,7 @@ module Banzai
if header_content = node.children.first
# namespace detection will be automatically handled via javascript (see issue #22781)
- namespace = "user-content_"
+ namespace = "user-content-"
href = "#{id}#{uniq}"
push_toc(href, text)
header_content.add_previous_sibling(anchor_tag("#{namespace}#{href}", href))
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 e551a7d0ca5..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 "user-content_header-#{i}"
+ expect(doc.css("h#{i} a").first.attr('id')).to eq "user-content-header-#{i}"
end
end
@@ -34,7 +34,7 @@ describe Banzai::Filter::TableOfContentsFilter, lib: true 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'
+ expect(doc.css('h1 a').first.attr('id')).to eq 'user-content-header'
end
it 'links to the non-namespaced id' do
@@ -67,7 +67,7 @@ describe Banzai::Filter::TableOfContentsFilter, lib: true do
it 'supports Unicode' do
doc = filter(header(1, '한글'))
- expect(doc.css('h1 a').first.attr('id')).to eq 'user-content_한글'
+ 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 a44bd2601c1..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#user-content_gitlab-markdown')
- expect(actual).to have_selector('h2 a#user-content_markdown')
- expect(actual).to have_selector('h3 a#user-content_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
--
GitLab