Fix link togglers jumping to top29414-fix-toggle-discussion-link-jump

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

15 files changed, 62 insertions, 13 deletions

diff --git a/app/assets/javascripts/behaviors/toggler_behavior.js b/app/assets/javascripts/behaviors/toggler_behavior.js
index 0726c6c9636..2ea4c7384a5 100644
--- a/app/assets/javascripts/behaviors/toggler_behavior.js
+++ b/app/assets/javascripts/behaviors/toggler_behavior.js
@@ -18,11 +18,16 @@
     // Button does not change visibility. If button has icon - it changes chevron style.
     //
     // %div.js-toggle-container
-    //   %a.js-toggle-button
+    //   %button.js-toggle-button
     //   %div.js-toggle-content
     //
-    $('body').on('click', '.js-toggle-button', function() {
+    $('body').on('click', '.js-toggle-button', function(e) {
       toggleContainer($(this).closest('.js-toggle-container'));
+
+      const targetTag = e.target.tagName.toLowerCase();
+      if (targetTag === 'a' || targetTag === 'button') {
+        e.preventDefault();
+      }
     });
 
     // If we're accessing a permalink, ensure it is not inside a
diff --git a/app/assets/stylesheets/framework/variables.scss b/app/assets/stylesheets/framework/variables.scss
index 6841adb637e..94fee722408 100644
--- a/app/assets/stylesheets/framework/variables.scss
+++ b/app/assets/stylesheets/framework/variables.scss
@@ -101,6 +101,7 @@ $gl-text-green: #4a2;
 $gl-text-red: #d12f19;
 $gl-text-orange: #d90;
 $gl-link-color: #3777b0;
+$gl-link-hover-color: darken($gl-link-color, 15%);
 $gl-grayish-blue: #7f8fa4;
 $gl-gray: $gl-text-color;
 $gl-gray-dark: #313236;
diff --git a/app/assets/stylesheets/pages/diff.scss b/app/assets/stylesheets/pages/diff.scss
index eab79c2a481..1aa1079903c 100644
--- a/app/assets/stylesheets/pages/diff.scss
+++ b/app/assets/stylesheets/pages/diff.scss
@@ -431,6 +431,21 @@
   border-bottom: none;
 }
 
+.diff-stats-summary-toggler {
+  padding: 0;
+  background-color: transparent;
+  border: 0;
+  color: $gl-link-color;
+  transition: color 0.1s linear;
+
+  &:hover,
+  &:focus {
+    outline: none;
+    text-decoration: underline;
+    color: $gl-link-hover-color;
+  }
+}
+
 // Mobile
 @media (max-width: 480px) {
   .diff-title {
diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss
index 7c3172421c1..256deddb713 100644
--- a/app/assets/stylesheets/pages/merge_requests.scss
+++ b/app/assets/stylesheets/pages/merge_requests.scss
@@ -60,7 +60,17 @@
       }
 
       .modify-merge-commit-link {
+        padding: 0;
+
+        background-color: transparent;
+        border: 0;
+
         color: $gl-text-color;
+
+        &:hover,
+        &:focus {
+          text-decoration: underline;
+        }
       }
 
       .merge-param-checkbox {
diff --git a/app/assets/stylesheets/pages/notes.scss b/app/assets/stylesheets/pages/notes.scss
index e238f0865f6..540ef8e6993 100644
--- a/app/assets/stylesheets/pages/notes.scss
+++ b/app/assets/stylesheets/pages/notes.scss
@@ -426,8 +426,22 @@ ul.notes {
 }
 
 .discussion-toggle-button {
+  padding: 0;
+  background-color: transparent;
+  border: 0;
   line-height: 20px;
   font-size: 13px;
+  transition: color 0.1s linear;
+
+  &:hover {
+    color: $gl-link-color;
+  }
+
+  &:focus {
+    text-decoration: underline;
+    outline: none;
+    color: $gl-link-color;
+  }
 
   .fa {
     margin-right: 3px;
diff --git a/app/views/discussions/_discussion.html.haml b/app/views/discussions/_discussion.html.haml
index 6f5d4bf2a2f..2d78c55211e 100644
--- a/app/views/discussions/_discussion.html.haml
+++ b/app/views/discussions/_discussion.html.haml
@@ -8,7 +8,7 @@
       .discussion.js-toggle-container{ class: discussion.id, data: { discussion_id: discussion.id } }
         .discussion-header
           .discussion-actions
-            = link_to "#", class: "note-action-button discussion-toggle-button js-toggle-button" do
+            %button.note-action-button.discussion-toggle-button.js-toggle-button{ type: "button" }
               - if expanded
                 = icon("chevron-up")
               - else
diff --git a/app/views/projects/commits/_commit.html.haml b/app/views/projects/commits/_commit.html.haml
index 6ab9a80e083..4b1ff75541a 100644
--- a/app/views/projects/commits/_commit.html.haml
+++ b/app/views/projects/commits/_commit.html.haml
@@ -24,7 +24,7 @@
           .visible-xs-inline
             = render_commit_status(commit, ref: ref)
         - if commit.description?
-          %a.text-expander.hidden-xs.js-toggle-button ...
+          %button.text-expander.hidden-xs.js-toggle-button{ type: "button" } ...
 
         - if commit.description?
           %pre.commit-row-description.js-toggle-content
diff --git a/app/views/projects/diffs/_stats.html.haml b/app/views/projects/diffs/_stats.html.haml
index 8e24e28765f..fd4f3c8d3cc 100644
--- a/app/views/projects/diffs/_stats.html.haml
+++ b/app/views/projects/diffs/_stats.html.haml
@@ -1,7 +1,7 @@
 .js-toggle-container
   .commit-stat-summary
     Showing
-    = link_to '#', class: 'js-toggle-button' do
+    %button.diff-stats-summary-toggler.js-toggle-button{ type: "button" }
       %strong= pluralize(diff_files.size, "changed file")
     with
     %strong.cgreen #{diff_files.sum(&:added_lines)} additions
diff --git a/app/views/projects/merge_requests/widget/open/_accept.html.haml b/app/views/projects/merge_requests/widget/open/_accept.html.haml
index c94c7944c0b..e5ec151a61d 100644
--- a/app/views/projects/merge_requests/widget/open/_accept.html.haml
+++ b/app/views/projects/merge_requests/widget/open/_accept.html.haml
@@ -37,7 +37,7 @@
             = check_box_tag :should_remove_source_branch
             Remove source branch
       .accept-control
-        = link_to "#", class: "modify-merge-commit-link js-toggle-button" do
+        %button.modify-merge-commit-link.js-toggle-button{ type: "button" }
           = icon('edit')
           Modify commit message
     .js-toggle-content.hide.prepend-top-default
diff --git a/app/views/projects/new.html.haml b/app/views/projects/new.html.haml
index 2a98bba05ee..be02525e088 100644
--- a/app/views/projects/new.html.haml
+++ b/app/views/projects/new.html.haml
@@ -75,7 +75,7 @@
                       Gitea
                 %div
                   - if git_import_enabled?
-                    = link_to "#", class: 'btn js-toggle-button import_git' do
+                    %button.btn.js-toggle-button.import_git{ type: "button" }
                       = icon('git', text: 'Repo by URL')
                 .import_gitlab_project
                   - if gitlab_project_import_enabled?
diff --git a/changelogs/unreleased/29414-fix-toggle-discussion-link-jump.yml b/changelogs/unreleased/29414-fix-toggle-discussion-link-jump.yml
new file mode 100644
index 00000000000..04342f5359d
--- /dev/null
+++ b/changelogs/unreleased/29414-fix-toggle-discussion-link-jump.yml
@@ -0,0 +1,4 @@
+---
+title: Update toggle buttons to be <button>
+merge_request:
+author:
diff --git a/features/steps/dashboard/new_project.rb b/features/steps/dashboard/new_project.rb
index cb36d6ae1a9..d4a04f693b8 100644
--- a/features/steps/dashboard/new_project.rb
+++ b/features/steps/dashboard/new_project.rb
@@ -19,7 +19,7 @@ class Spinach::Features::NewProject < Spinach::FeatureSteps
     expect(page).to have_link('Bitbucket')
     expect(page).to have_link('GitLab.com')
     expect(page).to have_link('Google Code')
-    expect(page).to have_link('Repo by URL')
+    expect(page).to have_button('Repo by URL')
     expect(page).to have_link('GitLab export')
   end
 
diff --git a/features/steps/project/merge_requests.rb b/features/steps/project/merge_requests.rb
index 9f0057cace7..c9c4f537fad 100644
--- a/features/steps/project/merge_requests.rb
+++ b/features/steps/project/merge_requests.rb
@@ -382,7 +382,7 @@ class Spinach::Features::ProjectMergeRequests < Spinach::FeatureSteps
   end
 
   step 'I modify merge commit message' do
-    find('.modify-merge-commit-link').click
+    click_button "Modify commit message"
     fill_in 'commit_message', with: 'wow such merge'
   end
 
diff --git a/spec/features/merge_requests/diff_notes_resolve_spec.rb b/spec/features/merge_requests/diff_notes_resolve_spec.rb
index d5e3d8e7eff..69164aabdb2 100644
--- a/spec/features/merge_requests/diff_notes_resolve_spec.rb
+++ b/spec/features/merge_requests/diff_notes_resolve_spec.rb
@@ -296,7 +296,7 @@ feature 'Diff notes resolve', feature: true, js: true do
       it 'displays next discussion even if hidden' do
         page.all('.note-discussion').each do |discussion|
           page.within discussion do
-            click_link 'Toggle discussion'
+            click_button 'Toggle discussion'
           end
         end
 
@@ -477,13 +477,13 @@ feature 'Diff notes resolve', feature: true, js: true do
       it 'shows resolved icon' do
         expect(page).to have_content '1/1 discussion resolved'
 
-        click_link 'Toggle discussion'
+        click_button 'Toggle discussion'
         expect(page).to have_selector('.line-resolve-btn.is-active')
       end
 
       it 'does not allow user to click resolve button' do
         expect(page).to have_selector('.line-resolve-btn.is-disabled')
-        click_link 'Toggle discussion'
+        click_button 'Toggle discussion'
 
         expect(page).to have_selector('.line-resolve-btn.is-disabled')
       end
diff --git a/spec/features/merge_requests/merge_commit_message_toggle_spec.rb b/spec/features/merge_requests/merge_commit_message_toggle_spec.rb
index 3dbe26cddb0..1bc2a5548dd 100644
--- a/spec/features/merge_requests/merge_commit_message_toggle_spec.rb
+++ b/spec/features/merge_requests/merge_commit_message_toggle_spec.rb
@@ -41,7 +41,7 @@ feature 'Clicking toggle commit message link', feature: true, js: true do
     visit namespace_project_merge_request_path(project.namespace, project, merge_request)
 
     expect(textbox).not_to be_visible
-    click_link "Modify commit message"
+    click_button "Modify commit message"
     expect(textbox).to be_visible
   end