Update permalink/blame buttons with line number fragment hash19742-permalink-blame-button-line-number-hash-links

Fix https://gitlab.com/gitlab-org/gitlab-ce/issues/19742

8 files changed, 157 insertions, 28 deletions

diff --git a/app/assets/javascripts/blob/blob_line_permalink_updater.js b/app/assets/javascripts/blob/blob_line_permalink_updater.js
new file mode 100644
index 00000000000..c8f68860fbd
--- /dev/null
+++ b/app/assets/javascripts/blob/blob_line_permalink_updater.js
@@ -0,0 +1,35 @@
+const lineNumberRe = /^L[0-9]+/;
+
+const updateLineNumbersOnBlobPermalinks = (linksToUpdate) => {
+  const hash = gl.utils.getLocationHash();
+  if (hash && lineNumberRe.test(hash)) {
+    const hashUrlString = `#${hash}`;
+
+    [].concat(Array.prototype.slice.call(linksToUpdate)).forEach((permalinkButton) => {
+      const baseHref = permalinkButton.getAttribute('data-original-href') || (() => {
+        const href = permalinkButton.getAttribute('href');
+        permalinkButton.setAttribute('data-original-href', href);
+        return href;
+      })();
+      permalinkButton.setAttribute('href', `${baseHref}${hashUrlString}`);
+    });
+  }
+};
+
+function BlobLinePermalinkUpdater(blobContentHolder, lineNumberSelector, elementsToUpdate) {
+  const updateBlameAndBlobPermalinkCb = () => {
+    // Wait for the hash to update from the LineHighlighter callback
+    setTimeout(() => {
+      updateLineNumbersOnBlobPermalinks(elementsToUpdate);
+    }, 0);
+  };
+
+  blobContentHolder.addEventListener('click', (e) => {
+    if (e.target.matches(lineNumberSelector)) {
+      updateBlameAndBlobPermalinkCb();
+    }
+  });
+  updateBlameAndBlobPermalinkCb();
+}
+
+export default BlobLinePermalinkUpdater;
diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js
index 7b9b9123c31..7956d6afff2 100644
--- a/app/assets/javascripts/dispatcher.js
+++ b/app/assets/javascripts/dispatcher.js
@@ -40,6 +40,7 @@ import BindInOut from './behaviors/bind_in_out';
 import GroupsList from './groups_list';
 import ProjectsList from './projects_list';
 import MiniPipelineGraph from './mini_pipeline_graph_dropdown';
+import BlobLinePermalinkUpdater from './blob/blob_line_permalink_updater';
 
 const ShortcutsBlob = require('./shortcuts_blob');
 const UserCallout = require('./user_callout');
@@ -252,6 +253,13 @@ const UserCallout = require('./user_callout');
         case 'projects:blob:show':
         case 'projects:blame:show':
           new LineHighlighter();
+
+          new BlobLinePermalinkUpdater(
+            document.querySelector('#blob-content-holder'),
+            '.diff-line-num[data-line-number]',
+            document.querySelectorAll('.js-data-file-blob-permalink-url, .js-blob-blame-link'),
+          );
+
           shortcut_handler = new ShortcutsNavigation();
           const fileBlobPermalinkUrlElement = document.querySelector('.js-data-file-blob-permalink-url');
           const fileBlobPermalinkUrl = fileBlobPermalinkUrlElement && fileBlobPermalinkUrlElement.getAttribute('href');
diff --git a/app/assets/javascripts/line_highlighter.js b/app/assets/javascripts/line_highlighter.js
index 966fcd8ec47..1821ca18053 100644
--- a/app/assets/javascripts/line_highlighter.js
+++ b/app/assets/javascripts/line_highlighter.js
@@ -67,17 +67,7 @@ require('vendor/jquery.scrollTo');
     }
 
     LineHighlighter.prototype.bindEvents = function() {
-      $('#blob-content-holder').on('mousedown', 'a[data-line-number]', this.clickHandler);
-      // While it may seem odd to bind to the mousedown event and then throw away
-      // the click event, there is a method to our madness.
-      //
-      // If not done this way, the line number anchor will sometimes keep its
-      // active state even when the event is cancelled, resulting in an ugly border
-      // around the link and/or a persisted underline text decoration.
-      $('#blob-content-holder').on('click', 'a[data-line-number]', function(event) {
-        event.preventDefault();
-        event.stopPropagation();
-      });
+      $('#blob-content-holder').on('click', 'a[data-line-number]', this.clickHandler);
     };
 
     LineHighlighter.prototype.clickHandler = function(event) {
diff --git a/app/assets/stylesheets/framework/highlight.scss b/app/assets/stylesheets/framework/highlight.scss
index 909a0f4afda..6d27d7568cf 100644
--- a/app/assets/stylesheets/framework/highlight.scss
+++ b/app/assets/stylesheets/framework/highlight.scss
@@ -57,8 +57,13 @@
         visibility: hidden;
       }
 
-      &:hover i {
-        visibility: visible;
+      &:hover,
+      &:focus {
+        outline: none;
+
+        & i {
+          visibility: visible;
+        }
       }
     }
   }
diff --git a/app/views/projects/blob/_actions.html.haml b/app/views/projects/blob/_actions.html.haml
index c44d8fcd430..14d42f7d9ec 100644
--- a/app/views/projects/blob/_actions.html.haml
+++ b/app/views/projects/blob/_actions.html.haml
@@ -12,7 +12,7 @@
           class: 'btn btn-sm'
     - else
       = link_to 'Blame', namespace_project_blame_path(@project.namespace, @project, @id),
-          class: 'btn btn-sm' unless @blob.empty?
+          class: 'btn btn-sm js-blob-blame-link' unless @blob.empty?
   = link_to 'History', namespace_project_commits_path(@project.namespace, @project, @id),
       class: 'btn btn-sm'
   = link_to 'Permalink', namespace_project_blob_path(@project.namespace, @project,
diff --git a/changelogs/unreleased/19742-permalink-blame-button-line-number-hash-links.yml b/changelogs/unreleased/19742-permalink-blame-button-line-number-hash-links.yml
new file mode 100644
index 00000000000..199f1edec8b
--- /dev/null
+++ b/changelogs/unreleased/19742-permalink-blame-button-line-number-hash-links.yml
@@ -0,0 +1,4 @@
+---
+title: Update permalink/blame buttons with line number fragment hash
+merge_request:
+author:
diff --git a/spec/features/projects/blobs/blob_line_permalink_updater_spec.rb b/spec/features/projects/blobs/blob_line_permalink_updater_spec.rb
new file mode 100644
index 00000000000..d94204230f6
--- /dev/null
+++ b/spec/features/projects/blobs/blob_line_permalink_updater_spec.rb
@@ -0,0 +1,97 @@
+require 'spec_helper'
+
+feature 'Blob button line permalinks (BlobLinePermalinkUpdater)', feature: true, js: true do
+  include TreeHelper
+
+  let(:project) { create(:project, :public, :repository) }
+  let(:path) { 'CHANGELOG' }
+  let(:sha) { project.repository.commit.sha }
+
+  describe 'On a file(blob)' do
+    def get_absolute_url(path = "")
+      "http://#{page.server.host}:#{page.server.port}#{path}"
+    end
+
+    def visit_blob(fragment = nil)
+      visit namespace_project_blob_path(project.namespace, project, tree_join('master', path), anchor: fragment)
+    end
+
+    describe 'Click "Permalink" button' do
+      it 'works with no initial line number fragment hash' do
+        visit_blob
+
+        expect(find('.js-data-file-blob-permalink-url')['href']).to eq(get_absolute_url(namespace_project_blob_path(project.namespace, project, tree_join(sha, path))))
+      end
+
+      it 'maintains intitial fragment hash' do
+        fragment = "L3"
+
+        visit_blob(fragment)
+
+        expect(find('.js-data-file-blob-permalink-url')['href']).to eq(get_absolute_url(namespace_project_blob_path(project.namespace, project, tree_join(sha, path), anchor: fragment)))
+      end
+
+      it 'changes fragment hash if line number clicked' do
+        ending_fragment = "L5"
+
+        visit_blob
+
+        find('#L3').click
+        find("##{ending_fragment}").click
+
+        expect(find('.js-data-file-blob-permalink-url')['href']).to eq(get_absolute_url(namespace_project_blob_path(project.namespace, project, tree_join(sha, path), anchor: ending_fragment)))
+      end
+
+      it 'with initial fragment hash, changes fragment hash if line number clicked' do
+        fragment = "L1"
+        ending_fragment = "L5"
+
+        visit_blob(fragment)
+
+        find('#L3').click
+        find("##{ending_fragment}").click
+
+        expect(find('.js-data-file-blob-permalink-url')['href']).to eq(get_absolute_url(namespace_project_blob_path(project.namespace, project, tree_join(sha, path), anchor: ending_fragment)))
+      end
+    end
+
+    describe 'Click "Blame" button' do
+      it 'works with no initial line number fragment hash' do
+        visit_blob
+
+        expect(find('.js-blob-blame-link')['href']).to eq(get_absolute_url(namespace_project_blame_path(project.namespace, project, tree_join('master', path))))
+      end
+
+      it 'maintains intitial fragment hash' do
+        fragment = "L3"
+
+        visit_blob(fragment)
+
+        expect(find('.js-blob-blame-link')['href']).to eq(get_absolute_url(namespace_project_blame_path(project.namespace, project, tree_join('master', path), anchor: fragment)))
+      end
+
+      it 'changes fragment hash if line number clicked' do
+        ending_fragment = "L5"
+
+        visit_blob
+
+        find('#L3').click
+        find("##{ending_fragment}").click
+
+        expect(find('.js-blob-blame-link')['href']).to eq(get_absolute_url(namespace_project_blame_path(project.namespace, project, tree_join('master', path), anchor: ending_fragment)))
+      end
+
+      it 'with initial fragment hash, changes fragment hash if line number clicked' do
+        fragment = "L1"
+        ending_fragment = "L5"
+
+        visit_blob(fragment)
+
+        find('#L3').click
+        find("##{ending_fragment}").click
+
+        expect(find('.js-blob-blame-link')['href']).to eq(get_absolute_url(namespace_project_blame_path(project.namespace, project, tree_join('master', path), anchor: ending_fragment)))
+      end
+    end
+  end
+end
diff --git a/spec/javascripts/line_highlighter_spec.js b/spec/javascripts/line_highlighter_spec.js
index a0b2ebc221b..a1fd2d38968 100644
--- a/spec/javascripts/line_highlighter_spec.js
+++ b/spec/javascripts/line_highlighter_spec.js
@@ -7,16 +7,12 @@ require('~/line_highlighter');
   describe('LineHighlighter', function() {
     var clickLine;
     preloadFixtures('static/line_highlighter.html.raw');
-    clickLine = function(number, eventData) {
-      var e;
-      if (eventData == null) {
-        eventData = {};
-      }
+    clickLine = function(number, eventData = {}) {
       if ($.isEmptyObject(eventData)) {
-        return $("#L" + number).mousedown().click();
+        return $("#L" + number).click();
       } else {
-        e = $.Event('mousedown', eventData);
-        return $("#L" + number).trigger(e).click();
+        const e = $.Event('click', eventData);
+        return $("#L" + number).trigger(e);
       }
     };
     beforeEach(function() {
@@ -63,12 +59,6 @@ require('~/line_highlighter');
       });
     });
     describe('#clickHandler', function() {
-      it('discards the mousedown event', function() {
-        var spy;
-        spy = spyOnEvent('a[data-line-number]', 'mousedown');
-        clickLine(13);
-        return expect(spy).toHaveBeenPrevented();
-      });
       it('handles clicking on a child icon element', function() {
         var spy;
         spy = spyOn(this["class"], 'setHash').and.callThrough();