Merge branch '23696-fix-diff-view-highlighting' into 'master'

Resolve "Highlighting lines is broken"

Add line highlighting back to diff view.  This was working in the MR "changes" tab, but not on a commit page such as https://gitlab.com/winniehell/reproduction-area/commit/9101e66f5761929002956e0f2dd65d7f8643903d

~~This MR also fixes the `scrollToElement` method in `MergeRequestTabs` to account for the extra height of the tab links which are now fixed in place once they are scrolled to the top of the screen.~~ (removed in favor of !7051)

This MR also refactors much of the `Diff` and `MergeRequestTabs` classes to es6 syntax in an effort to increase readability.

Check out both MR "change" tabs and commit diff pages and ensure that line highlighting works and that loading a page with one of these permalink hashes correctly highlights and scrolls to the line.

Ensure I didn't break anything in the transition to es6 syntax.  Check the functionality of the tabs on MR pages, as well as diff page interactivity (unfolding hidden lines in diff files, adding comments to diffs, etc).  I have checked these myself, but another set of eyes would be a good idea.

- [x] [CHANGELOG](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/CHANGELOG.md) entry added
- Tests
  - [x] 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)

Closes #23696

See merge request !7090

app/assets/javascripts/diff.js

-/* eslint-disable func-names, space-before-function-paren, wrap-iife, no-var, max-len, one-var, camelcase, one-var-declaration-per-line, no-unused-vars, no-unused-expressions, no-sequences, object-shorthand, comma-dangle, prefer-arrow-callback, semi, radix, padded-blocks, max-len */
-(function() {
-  this.Diff = (function() {
-    var UNFOLD_COUNT;
-
-    UNFOLD_COUNT = 20;
-
-    function Diff() {
-      $('.files .diff-file').singleFileDiff();
-      this.filesCommentButton = $('.files .diff-file').filesCommentButton();
-      if (this.diffViewType() === 'parallel') {
-        $('.content-wrapper .container-fluid').removeClass('container-limited');
-      }
-      $(document).off('click', '.js-unfold');
-      $(document).on('click', '.js-unfold', (function(_this) {
-        return function(event) {
-          var line_number, link, file, offset, old_line, params, prev_new_line, prev_old_line, ref, ref1, since, target, to, unfold, unfoldBottom;
-          target = $(event.target);
-          unfoldBottom = target.hasClass('js-unfold-bottom');
-          unfold = true;
-          ref = _this.lineNumbers(target.parent()), old_line = ref[0], line_number = ref[1];
-          offset = line_number - old_line;
-          if (unfoldBottom) {
-            line_number += 1;
-            since = line_number;
-            to = line_number + UNFOLD_COUNT;
-          } else {
-            ref1 = _this.lineNumbers(target.parent().prev()), prev_old_line = ref1[0], prev_new_line = ref1[1];
-            line_number -= 1;
-            to = line_number;
-            if (line_number - UNFOLD_COUNT > prev_new_line + 1) {
-              since = line_number - UNFOLD_COUNT;
-            } else {
-              since = prev_new_line + 1;
-              unfold = false;
-            }
-          }
-          file = target.parents('.diff-file');
-          link = file.data('blob-diff-path');
-          params = {
-            since: since,
-            to: to,
-            bottom: unfoldBottom,
-            offset: offset,
-            unfold: unfold,
-            view: file.data('view')
-          };
-          return $.get(link, params, function(response) {
-            return target.parent().replaceWith(response);
-          });
-        };
-      })(this));
-    }
-
-    Diff.prototype.diffViewType = function() {
-      return $('.inline-parallel-buttons a.active').data('view-type');
-    }
-
-    Diff.prototype.lineNumbers = function(line) {
-      if (!line.children().length) {
-        return [0, 0];
-      }
-
-      return line.find('.diff-line-num').map(function() {
-        return parseInt($(this).data('linenumber'));
-      });
-    };
-
-    return Diff;
-
-  })();
-
-}).call(this);

app/assets/javascripts/diff.js.es6

+/* eslint-disable class-methods-use-this */
+
+(() => {
+  const UNFOLD_COUNT = 20;
+
+  class Diff {
+    constructor() {
+      $('.files .diff-file').singleFileDiff();
+      $('.files .diff-file').filesCommentButton();
+
+      if (this.diffViewType() === 'parallel') {
+        $('.content-wrapper .container-fluid').removeClass('container-limited');
+      }
+
+      $(document)
+        .off('click', '.js-unfold, .diff-line-num a')
+        .on('click', '.js-unfold', this.handleClickUnfold.bind(this))
+        .on('click', '.diff-line-num a', this.handleClickLineNum.bind(this));
+
+      this.highlighSelectedLine();
+    }
+
+    handleClickUnfold(e) {
+      const $target = $(e.target);
+      // current babel config relies on iterators implementation, so we cannot simply do:
+      // const [oldLineNumber, newLineNumber] = this.lineNumbers($target.parent());
+      const ref = this.lineNumbers($target.parent());
+      const oldLineNumber = ref[0];
+      const newLineNumber = ref[1];
+      const offset = newLineNumber - oldLineNumber;
+      const bottom = $target.hasClass('js-unfold-bottom');
+      let since;
+      let to;
+      let unfold = true;
+
+      if (bottom) {
+        const lineNumber = newLineNumber + 1;
+        since = lineNumber;
+        to = lineNumber + UNFOLD_COUNT;
+      } else {
+        const lineNumber = newLineNumber - 1;
+        since = lineNumber - UNFOLD_COUNT;
+        to = lineNumber;
+
+        // make sure we aren't loading more than we need
+        const prevNewLine = this.lineNumbers($target.parent().prev())[1];
+        if (since <= prevNewLine + 1) {
+          since = prevNewLine + 1;
+          unfold = false;
+        }
+      }
+
+      const file = $target.parents('.diff-file');
+      const link = file.data('blob-diff-path');
+      const view = file.data('view');
+
+      const params = { since, to, bottom, offset, unfold, view };
+      $.get(link, params, response => $target.parent().replaceWith(response));
+    }
+
+    openAnchoredDiff(anchoredDiff, cb) {
+      const diffTitle = $(`#file-path-${anchoredDiff}`);
+      const diffFile = diffTitle.closest('.diff-file');
+      const nothingHereBlock = $('.nothing-here-block:visible', diffFile);
+      if (nothingHereBlock.length) {
+        diffFile.singleFileDiff(true, cb);
+      } else {
+        cb();
+      }
+    }
+
+    handleClickLineNum(e) {
+      const hash = $(e.currentTarget).attr('href');
+      e.preventDefault();
+      if (window.history.pushState) {
+        window.history.pushState(null, null, hash);
+      } else {
+        window.location.hash = hash;
+      }
+      this.highlighSelectedLine();
+    }
+
+    diffViewType() {
+      return $('.inline-parallel-buttons a.active').data('view-type');
+    }
+
+    lineNumbers(line) {
+      if (!line.children().length) {
+        return [0, 0];
+      }
+      return line.find('.diff-line-num').map((i, elm) => parseInt($(elm).data('linenumber'), 10));
+    }
+
+    highlighSelectedLine() {
+      const $diffFiles = $('.diff-file');
+      $diffFiles.find('.hll').removeClass('hll');
+
+      if (window.location.hash !== '') {
+        const hash = window.location.hash.replace('#', '');
+        $diffFiles
+          .find(`tr#${hash}:not(.match) td, td#${hash}, td[data-line-code="${hash}"]`)
+          .addClass('hll');
+      }
+    }
+  }
+
+  window.gl = window.gl || {};
+  window.gl.Diff = Diff;
+})();

app/assets/javascripts/dispatcher.js.es6

@@ -61,7 +61,7 @@
           new ZenMode();
           break;
         case 'projects:compare:show':
-          new Diff();
+          new gl.Diff();
           break;
         case 'projects:issues:new':
         case 'projects:issues:edit':
@@ -74,7 +74,7 @@
           break;
         case 'projects:merge_requests:new':
         case 'projects:merge_requests:edit':
-          new Diff();
+          new gl.Diff();
           shortcut_handler = new ShortcutsNavigation();
           new GLForm($('.merge-request-form'));
           new IssuableForm($('.merge-request-form'));
@@ -91,7 +91,7 @@
           new GLForm($('.release-form'));
           break;
         case 'projects:merge_requests:show':
-          new Diff();
+          new gl.Diff();
           shortcut_handler = new ShortcutsIssuable(true);
           new ZenMode();
           new MergedButtons();
@@ -101,7 +101,7 @@
           new MergedButtons();
           break;
         case "projects:merge_requests:diffs":
-          new Diff();
+          new gl.Diff();
           new ZenMode();
           new MergedButtons();
           break;
@@ -117,7 +117,7 @@
           break;
         case 'projects:commit:show':
           new Commit();
-          new Diff();
+          new gl.Diff();
           new ZenMode();
           shortcut_handler = new ShortcutsNavigation();
           break;

app/assets/javascripts/merge_request.js

@@ -40,7 +40,7 @@
       if (window.mrTabs) {
         window.mrTabs.unbindEvents();
       }
-      window.mrTabs = new MergeRequestTabs(this.opts);
+      window.mrTabs = new gl.MergeRequestTabs(this.opts);
     };
 
     MergeRequest.prototype.showAllCommits = function() {

app/assets/javascripts/single_file_diff.js

@@ -14,6 +14,7 @@
     COLLAPSED_HTML = '<div class="nothing-here-block diff-collapsed">This diff is collapsed. <a class="click-to-expand">Click to expand it.</a></div>';
 
     function SingleFileDiff(file, forceLoad, cb) {
+      var clickTarget;
       this.file = file;
       this.toggleDiff = bind(this.toggleDiff, this);
       this.content = $('.diff-content', this.file);
@@ -31,9 +32,9 @@
         this.content.after(this.collapsedContent);
         this.$toggleIcon.addClass('fa-caret-down');
       }
-      $('.file-title, .click-to-expand', this.file).on('click', this.toggleDiff);
+      clickTarget = $('.file-title, .click-to-expand', this.file).on('click', this.toggleDiff);
       if (forceLoad) {
-        this.toggleDiff(null, cb);
+        this.toggleDiff({ target: clickTarget }, cb);
       }
     }
 

changelogs/unreleased/23696-fix-diff-view-highlighting.yml

+---
+title: Fix diff view permalink highlighting
+merge_request: 7090
+author:

spec/javascripts/merge_request_tabs_spec.js

-/* eslint-disable space-before-function-paren, no-var, comma-dangle, dot-notation, quotes, no-undef, no-return-assign, no-underscore-dangle, camelcase, padded-blocks, max-len */
+/* eslint-disable no-var, comma-dangle, object-shorthand */
 
 /*= require merge_request_tabs */
 //= require breakpoints
 
-(function() {
-  describe('MergeRequestTabs', function() {
-    var stubLocation;
-    stubLocation = function(stubs) {
-      var defaults;
-      defaults = {
+(function () {
+  describe('MergeRequestTabs', function () {
+    var stubLocation = {};
+    var setLocation = function (stubs) {
+      var defaults = {
         pathname: '',
         search: '',
         hash: ''
       };
-      return $.extend(defaults, stubs);
+      $.extend(stubLocation, defaults, stubs || {});
     };
     fixture.preload('merge_request_tabs.html');
-    beforeEach(function() {
-      this["class"] = new MergeRequestTabs();
-      return this.spies = {
-        ajax: spyOn($, 'ajax').and.callFake(function() {}),
-        history: spyOn(history, 'replaceState').and.callFake(function() {})
+
+    beforeEach(function () {
+      this.class = new gl.MergeRequestTabs({ stubLocation: stubLocation });
+      setLocation();
+
+      this.spies = {
+        ajax: spyOn($, 'ajax').and.callFake(function () {}),
+        history: spyOn(window.history, 'replaceState').and.callFake(function () {})
       };
     });
-    describe('#activateTab', function() {
-      beforeEach(function() {
+
+    describe('#activateTab', function () {
+      beforeEach(function () {
         fixture.load('merge_request_tabs.html');
-        return this.subject = this["class"].activateTab;
+        this.subject = this.class.activateTab;
       });
-      it('shows the first tab when action is show', function() {
+      it('shows the first tab when action is show', function () {
         this.subject('show');
-        return expect($('#notes')).toHaveClass('active');
+        expect($('#notes')).toHaveClass('active');
       });
-      it('shows the notes tab when action is notes', function() {
+      it('shows the notes tab when action is notes', function () {
         this.subject('notes');
-        return expect($('#notes')).toHaveClass('active');
+        expect($('#notes')).toHaveClass('active');
       });
-      it('shows the commits tab when action is commits', function() {
+      it('shows the commits tab when action is commits', function () {
         this.subject('commits');
-        return expect($('#commits')).toHaveClass('active');
+        expect($('#commits')).toHaveClass('active');
       });
-      return it('shows the diffs tab when action is diffs', function() {
+      it('shows the diffs tab when action is diffs', function () {
         this.subject('diffs');
-        return expect($('#diffs')).toHaveClass('active');
+        expect($('#diffs')).toHaveClass('active');
       });
     });
-    return describe('#setCurrentAction', function() {
-      beforeEach(function() {
-        return this.subject = this["class"].setCurrentAction;
+
+    describe('#setCurrentAction', function () {
+      beforeEach(function () {
+        this.subject = this.class.setCurrentAction;
       });
-      it('changes from commits', function() {
-        this["class"]._location = stubLocation({
+      it('changes from commits', function () {
+        setLocation({
           pathname: '/foo/bar/merge_requests/1/commits'
         });
         expect(this.subject('notes')).toBe('/foo/bar/merge_requests/1');
-        return expect(this.subject('diffs')).toBe('/foo/bar/merge_requests/1/diffs');
+        expect(this.subject('diffs')).toBe('/foo/bar/merge_requests/1/diffs');
       });
-      it('changes from diffs', function() {
-        this["class"]._location = stubLocation({
+      it('changes from diffs', function () {
+        setLocation({
           pathname: '/foo/bar/merge_requests/1/diffs'
         });
         expect(this.subject('notes')).toBe('/foo/bar/merge_requests/1');
-        return expect(this.subject('commits')).toBe('/foo/bar/merge_requests/1/commits');
+        expect(this.subject('commits')).toBe('/foo/bar/merge_requests/1/commits');
       });
-      it('changes from diffs.html', function() {
-        this["class"]._location = stubLocation({
+      it('changes from diffs.html', function () {
+        setLocation({
           pathname: '/foo/bar/merge_requests/1/diffs.html'
         });
         expect(this.subject('notes')).toBe('/foo/bar/merge_requests/1');
-        return expect(this.subject('commits')).toBe('/foo/bar/merge_requests/1/commits');
+        expect(this.subject('commits')).toBe('/foo/bar/merge_requests/1/commits');
       });
-      it('changes from notes', function() {
-        this["class"]._location = stubLocation({
+      it('changes from notes', function () {
+        setLocation({
           pathname: '/foo/bar/merge_requests/1'
         });
         expect(this.subject('diffs')).toBe('/foo/bar/merge_requests/1/diffs');
-        return expect(this.subject('commits')).toBe('/foo/bar/merge_requests/1/commits');
+        expect(this.subject('commits')).toBe('/foo/bar/merge_requests/1/commits');
       });
-      it('includes search parameters and hash string', function() {
-        this["class"]._location = stubLocation({
+      it('includes search parameters and hash string', function () {
+        setLocation({
           pathname: '/foo/bar/merge_requests/1/diffs',
           search: '?view=parallel',
           hash: '#L15-35'
         });
-        return expect(this.subject('show')).toBe('/foo/bar/merge_requests/1?view=parallel#L15-35');
+        expect(this.subject('show')).toBe('/foo/bar/merge_requests/1?view=parallel#L15-35');
       });
-      it('replaces the current history state', function() {
-        var new_state;
-        this["class"]._location = stubLocation({
+      it('replaces the current history state', function () {
+        var newState;
+        setLocation({
           pathname: '/foo/bar/merge_requests/1'
         });
-        new_state = this.subject('commits');
-        return expect(this.spies.history).toHaveBeenCalledWith({
+        newState = this.subject('commits');
+        expect(this.spies.history).toHaveBeenCalledWith({
           turbolinks: true,
-          url: new_state
-        }, document.title, new_state);
+          url: newState
+        }, document.title, newState);
       });
-      return it('treats "show" like "notes"', function() {
-        this["class"]._location = stubLocation({
+      it('treats "show" like "notes"', function () {
+        setLocation({
           pathname: '/foo/bar/merge_requests/1/commits'
         });
-        return expect(this.subject('show')).toBe('/foo/bar/merge_requests/1');
+        expect(this.subject('show')).toBe('/foo/bar/merge_requests/1');
       });
     });
   });
-
 }).call(this);