Update permalink/blame buttons with line number fragment hash

What does this MR do?

Updates permalink and blame buttons href to include the line number fragment hash.

Previous MR to add y keyboard shortcut that maintained line number fragment hash, https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/8799

Are there points in the code the reviewer needs to double check?

• Are we ok with the app/assets/javascripts/extensions/history.js that fires off an event whenever history.pushState is called?
  • When clicking a line number, it changes the URL to #LX with history.pushState which doesn't have a way to listen to natively
  • Clicking a line number is preventDefault'ed and stopPropagation'ed so we can't hook on that and adding it directly in line_highter.js doesn't seem right.

Why was this MR needed?

Previously when you clicked a line number and had the #LX fragment hash and then click the Permalink or Blame button, it would get rid of the hash.

Screenshots (if relevant)

Does this MR meet the acceptance criteria?

• Changelog entry added
• Tests
  • Added for this feature/bug
  • All builds are passing
• Conform by the merge request performance guides
• Conform by the style guides
• Branch has no merge conflicts with master (if it does - rebase it please)
• Squashed related commits together

EE MR: https://gitlab.com/gitlab-org/gitlab-ee/merge_requests/1381

What are the relevant issue numbers?

Closes #19742 (closed)

marked the task All builds are passing as completed

@alfredo1 Friendly ping for review

@eric I was looking why we can't listen to the click event on numbers.

In line_highlighter.js it says.

$('#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();
});

These comments makes me think the presentational issues can be handled with CSS with :focus or :active (if we have one. Personally I don't think it's a problem). I think we should fix that and use .on('click', fn) instead and only use e.preventDefault() so you can still get your own event listener to be called.

Something like this:

LineHighlighter.prototype.bindEvents = function() {
  $('#blob-content-holder').on('click', 'a[data-line-number]', this.clickHandler);
};

LineHighlighter.prototype.clickHandler = function(event) {
  var current, lineNumber, range;
  event.preventDefault();
  ...
};

Can we do that?

assigned to @MadLittleMods

@MadLittleMods Can you get the ruby part to be reviewed before assigning to me? Thanks

resolved all discussions

added 616 commits

• 223b3db1...afd56634 - 615 commits from branch master
• ee065f95 - Update permalink/blame buttons with line number fragment hash

Compare with previous version

@alfredo1 Callback order of operation problems with the LineHighlighter also updating on click

assigned to @dbalexandre

added 1 commit

• 77508826 - Update permalink/blame buttons with line number fragment hash

Compare with previous version

@dbalexandre rspec tests need some review

@MadLittleMods Thanks! I left some notes :)

assigned to @MadLittleMods

added 159 commits

• 77508826...b696cbc5 - 158 commits from branch master
• 81e79b6a - Update permalink/blame buttons with line number fragment hash

Compare with previous version

added 1 commit

• 55e4097a - Update permalink/blame buttons with line number fragment hash

Compare with previous version

assigned to @dbalexandre

assigned to @alfredo1

@MadLittleMods can you check failing tests? Seems legit.

assigned to @MadLittleMods

changed milestone to %9.1

added 327 commits

• 55e4097a...b5cb1115 - 326 commits from branch master
• 292bcffd - Update permalink/blame buttons with line number fragment hash

Compare with previous version

added 327 commits

• 55e4097a...b5cb1115 - 326 commits from branch master
• 292bcffd - Update permalink/blame buttons with line number fragment hash

Compare with previous version

assigned to @alfredo1

added 38 commits

• 292bcffd...935574ed - 37 commits from branch master
• 349d6899 - Update permalink/blame buttons with line number fragment hash

Compare with previous version

@alfredo1 Trying to push this past the yarn errors, https://gitlab.com/gitlab-org/gitlab-ce/issues/28341

Trying to push this past the yarn errors

@MadLittleMods Retrying failed jobs was usually sufficient for me.

@winniehell I retried multiple times for this one but rebasing on master again and pushing seems to have worked (on final step now)

@MadLittleMods I think the error you experienced is not about network problems but about dependency resolution strategy in yarn. I'll create a new issue about it.

@MadLittleMods Looks good so far! A few comments for you

added 37 commits

• 349d6899...69c1a9ae - 36 commits from branch master
• bc02e731 - Update permalink/blame buttons with line number fragment hash

Compare with previous version

@alfredo1 Updated

@MadLittleMods can we merge blob_line_permalink_updater.js and update_line_numbers_on_blob_permalinks.js into one class? or you think these should be separated?

assigned to @MadLittleMods

added 37 commits

• bc02e731...130fd255 - 36 commits from branch master
• c857e0ca - Update permalink/blame buttons with line number fragment hash

Compare with previous version

@alfredo1 I merged them together

assigned to @alfredo1

added 47 commits

• c857e0ca...daa4590c - 46 commits from branch master
• 2d0a6d73 - Update permalink/blame buttons with line number fragment hash

Compare with previous version

added 47 commits

• c857e0ca...daa4590c - 46 commits from branch master
• 2d0a6d73 - Update permalink/blame buttons with line number fragment hash

Compare with previous version

@MadLittleMods just a comment for something I just noticed

assigned to @MadLittleMods

added 77 commits

• 2d0a6d73...1d4b11f3 - 76 commits from branch master
• 08df762f - Update permalink/blame buttons with line number fragment hash

Compare with previous version

added 1 commit

• c0242485 - Update permalink/blame buttons with line number fragment hash

Compare with previous version

assigned to @alfredo1

LGTM! Thanks @MadLittleMods

Approved this merge request

enabled an automatic merge when the pipeline for c0242485 succeeds

merged

mentioned in commit 08f7e49d

@MadLittleMods This seems to have introduced legitimate failures on master: https://gitlab.com/gitlab-org/gitlab-ce/builds/12138972

Thanks @rspeicher, looking into it now. They pass locally

Actually, they pass in this branch but fail on master.

mentioned in commit e10810de

mentioned in merge request !9914 (merged)

@rspeicher @alfredo1 Fix is here https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/9914

mentioned in issue #30144 (closed)