diff options
author | Fatih Acet <acetfatih@gmail.com> | 2016-12-01 20:58:08 +0000 |
---|---|---|
committer | Fatih Acet <acetfatih@gmail.com> | 2016-12-01 20:58:08 +0000 |
commit | 4d37be0fb874998325d2785e6934fd87a117c806 (patch) | |
tree | 8cc9f5b7f29c3e7d11ba710ef9fdb5168c3b09db | |
parent | 24ed24cf545ed3796336815d863c92a2c6fa8d5f (diff) | |
parent | 131a04d7962b01daa58b8e5efe3f1359a3e73fe1 (diff) | |
download | gitlab-ce-4d37be0fb874998325d2785e6934fd87a117c806.tar.gz |
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
-rw-r--r-- | app/assets/javascripts/application.js | 31 | ||||
-rw-r--r-- | app/assets/javascripts/lib/utils/common_utils.js | 33 | ||||
-rw-r--r-- | app/assets/stylesheets/framework/typography.scss | 1 | ||||
-rw-r--r-- | changelogs/unreleased/22781-user-generated-permalinks.yml | 4 | ||||
-rw-r--r-- | doc/development/gotchas.md | 48 | ||||
-rw-r--r-- | features/steps/shared/markdown.rb | 2 | ||||
-rw-r--r-- | lib/banzai/filter/table_of_contents_filter.rb | 8 | ||||
-rw-r--r-- | spec/lib/banzai/filter/table_of_contents_filter_spec.rb | 21 | ||||
-rw-r--r-- | spec/support/matchers/markdown_matchers.rb | 6 |
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 |