Commit 4d37be0f authored by Fatih Acet's avatar Fatih Acet

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
parents 24ed24cf 131a04d7
...@@ -57,32 +57,11 @@ ...@@ -57,32 +57,11 @@
(function () { (function () {
document.addEventListener('page:fetch', gl.utils.cleanupBeforeFetch); document.addEventListener('page:fetch', gl.utils.cleanupBeforeFetch);
window.addEventListener('hashchange', gl.utils.shiftWindow); window.addEventListener('hashchange', gl.utils.handleLocationHash);
window.addEventListener('load', function onLoad() {
// automatically adjust scroll position for hash urls taking the height of the navbar into account window.removeEventListener('load', onLoad, false);
// https://github.com/twitter/bootstrap/issues/1768 gl.utils.handleLocationHash();
window.adjustScroll = function() { }, false);
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);
}
};
$(function () { $(function () {
var $body = $('body'); var $body = $('body');
......
/* 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() {
(function(w) { (function(w) {
var base; var base;
...@@ -94,10 +94,35 @@ ...@@ -94,10 +94,35 @@
return $(document).off('scroll'); return $(document).off('scroll');
}; };
w.gl.utils.shiftWindow = function() { // automatically adjust scroll position for hash urls taking the height of the navbar into account
return w.scrollBy(0, -100); // 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) { gl.utils.updateTooltipTitle = function($tooltipEl, newTitle) {
return $tooltipEl.tooltip('destroy').attr('title', newTitle).tooltip('fixTitle'); return $tooltipEl.tooltip('destroy').attr('title', newTitle).tooltip('fixTitle');
......
...@@ -182,6 +182,7 @@ ...@@ -182,6 +182,7 @@
left: -16px; left: -16px;
position: absolute; position: absolute;
text-decoration: none; text-decoration: none;
outline: none;
&::after { &::after {
content: image-url('icon_anchor.svg'); content: image-url('icon_anchor.svg');
......
---
title: Prevent DOM ID collisions resulting from user-generated content anchors
merge_request: 7631
author:
...@@ -141,51 +141,3 @@ in an initializer._ ...@@ -141,51 +141,3 @@ in an initializer._
### Further reading ### Further reading
- Stack Overflow: [Why you should not write inline JavaScript](http://programmers.stackexchange.com/questions/86589/why-should-i-avoid-inline-scripting) - 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)
...@@ -2,7 +2,7 @@ module SharedMarkdown ...@@ -2,7 +2,7 @@ module SharedMarkdown
include Spinach::DSL include Spinach::DSL
def header_should_have_correct_id_and_link(level, text, id, parent = ".wiki") 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}" expect(node[:href]).to eq "##{id}"
# Work around a weird Capybara behavior where calling `parent` on a node # Work around a weird Capybara behavior where calling `parent` on a node
......
...@@ -35,9 +35,11 @@ module Banzai ...@@ -35,9 +35,11 @@ module Banzai
headers[id] += 1 headers[id] += 1
if header_content = node.children.first if header_content = node.children.first
# namespace detection will be automatically handled via javascript (see issue #22781)
namespace = "user-content-"
href = "#{id}#{uniq}" href = "#{id}#{uniq}"
push_toc(href, text) push_toc(href, text)
header_content.add_previous_sibling(anchor_tag(href)) header_content.add_previous_sibling(anchor_tag("#{namespace}#{href}", href))
end end
end end
...@@ -48,8 +50,8 @@ module Banzai ...@@ -48,8 +50,8 @@ module Banzai
private private
def anchor_tag(href) def anchor_tag(id, href)
%Q{<a id="#{href}" class="anchor" href="##{href}" aria-hidden="true"></a>} %Q{<a id="#{id}" class="anchor" href="##{href}" aria-hidden="true"></a>}
end end
def push_toc(href, text) def push_toc(href, text)
......
...@@ -22,7 +22,7 @@ describe Banzai::Filter::TableOfContentsFilter, lib: true do ...@@ -22,7 +22,7 @@ describe Banzai::Filter::TableOfContentsFilter, lib: true do
html = header(i, "Header #{i}") html = header(i, "Header #{i}")
doc = filter(html) 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
end end
...@@ -32,7 +32,12 @@ describe Banzai::Filter::TableOfContentsFilter, lib: true do ...@@ -32,7 +32,12 @@ describe Banzai::Filter::TableOfContentsFilter, lib: true do
expect(doc.css('h1 a').first.attr('class')).to eq 'anchor' expect(doc.css('h1 a').first.attr('class')).to eq 'anchor'
end 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')) doc = filter(header(1, 'Header'))
expect(doc.css('h1 a').first.attr('href')).to eq '#header' expect(doc.css('h1 a').first.attr('href')).to eq '#header'
end end
...@@ -40,29 +45,29 @@ describe Banzai::Filter::TableOfContentsFilter, lib: true do ...@@ -40,29 +45,29 @@ describe Banzai::Filter::TableOfContentsFilter, lib: true do
describe 'generated IDs' do describe 'generated IDs' do
it 'translates spaces to dashes' do it 'translates spaces to dashes' do
doc = filter(header(1, 'This header has spaces in it')) 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 end
it 'squeezes multiple spaces and dashes' do it 'squeezes multiple spaces and dashes' do
doc = filter(header(1, 'This---header is poorly-formatted')) 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 end
it 'removes punctuation' do it 'removes punctuation' do
doc = filter(header(1, "This, header! is, filled. with @ punctuation?")) 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 end
it 'appends a unique number to duplicates' do it 'appends a unique number to duplicates' do
doc = filter(header(1, 'One') + header(2, 'One')) doc = filter(header(1, 'One') + header(2, 'One'))
expect(doc.css('h1 a').first.attr('id')).to eq 'one' expect(doc.css('h1 a').first.attr('href')).to eq '#one'
expect(doc.css('h2 a').first.attr('id')).to eq 'one-1' expect(doc.css('h2 a').first.attr('href')).to eq '#one-1'
end end
it 'supports Unicode' do it 'supports Unicode' do
doc = filter(header(1, '한글')) 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 '#한글' expect(doc.css('h1 a').first.attr('href')).to eq '#한글'
end end
end end
......
...@@ -38,9 +38,9 @@ module MarkdownMatchers ...@@ -38,9 +38,9 @@ module MarkdownMatchers
set_default_markdown_messages set_default_markdown_messages
match do |actual| match do |actual|
expect(actual).to have_selector('h1 a#gitlab-markdown') expect(actual).to have_selector('h1 a#user-content-gitlab-markdown')
expect(actual).to have_selector('h2 a#markdown') expect(actual).to have_selector('h2 a#user-content-markdown')
expect(actual).to have_selector('h3 a#autolinkfilter') expect(actual).to have_selector('h3 a#user-content-autolinkfilter')
end end
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment