Bug 1427800 - Wrong anchor scrolling with old UI

diff --git a/extensions/BugModal/web/bug_modal.js b/extensions/BugModal/web/bug_modal.js
index c1080cde8fa97af9a4e97c994bb93b97a294a6d4..c6abb5d7a41da8ddf527886b127b135bc372e09f 100644 (file)
--- a/extensions/BugModal/web/bug_modal.js
+++ b/extensions/BugModal/web/bug_modal.js
@@ -700,7 +700,7 @@ $(function() {
     $('#needinfo-scroll')
         .click(function(event) {
             event.preventDefault();
-            $.scrollTo($('#needinfo_role'), function() { $('#needinfo_role').focus(); });
+            $.scrollTo($('#needinfo_container'), function() { $('#needinfo_role').focus(); });
         });
 
     // knob
@@ -830,13 +830,15 @@ $(function() {
             // remove embedded links to attachment details
             reply_text = reply_text.replace(/(attachment\s+\d+)(\s+\[[^\[\n]+\])+/gi, '$1');
 
-            if ($('#comment').val() != reply_text) {
-                $('#comment').val($('#comment').val() + reply_text);
-            }
-            if (BUGZILLA.user.settings.autosize_comments) {
-                autosize.update($('#comment'));
-            }
             $.scrollTo($('#comment'), function() {
+                if ($('#comment').val() != reply_text) {
+                    $('#comment').val($('#comment').val() + reply_text);
+                }
+
+                if (BUGZILLA.user.settings.autosize_comments) {
+                    autosize.update($('#comment'));
+                }
+
                 $('#comment').focus();
             });
         });
@@ -942,7 +944,7 @@ $(function() {
             $('#' + id + '-view').hide();
             $('#' + id).show().focus().select();
         });
-        
+
     // timetracking
     $('#work_time').change(function() {
         // subtracts time spent from remaining time
@@ -1429,18 +1431,15 @@ $(function() {
             let $target;
 
             if (typeof target === 'string') {
-                $target = $('#' + target);
+                $target = document.getElementById(target);
                 window.location.hash = target;
             } else {
-                $target = target;
-            }
-
-            if ($target.length) {
-                $('main').scrollTop(Math.round($target.position().top) - 20);
+                // Use raw DOM node instead of jQuery
+                $target = target.get(0);
             }
 
-            if (complete) {
-                complete();
+            if ($target) {
+                scroll_element_into_view($target, complete);
             }
         }
 

diff --git a/extensions/BugModal/web/common_bug_modal.js b/extensions/BugModal/web/common_bug_modal.js
index 160f922ccac38d9f6c2070f5541b4a6c765031c3..dc91824f6ad20c9646d29084bca7af24d8d2d3fe 100644 (file)
--- a/extensions/BugModal/web/common_bug_modal.js
+++ b/extensions/BugModal/web/common_bug_modal.js
@@ -424,7 +424,7 @@ $(function() {
     var rbs = $("#readable-bug-status");
     var rbs_text = bugzillaReadableStatus.readable(rbs.data('readable-bug-status'));
     rbs.text(rbs_text);
-    
+
     if (BUGZILLA.user.id === 0) return;
 
     //
@@ -700,7 +700,7 @@ $(function() {
     $('#needinfo-scroll')
         .click(function(event) {
             event.preventDefault();
-            $.scrollTo($('#needinfo_role'), function() { $('#needinfo_role').focus(); });
+            $.scrollTo($('#needinfo_container'), function() { $('#needinfo_role').focus(); });
         });
 
     // knob
@@ -831,10 +831,13 @@ $(function() {
             // remove embedded links to attachment details
             reply_text = reply_text.replace(/(attachment\s+\d+)(\s+\[[^\[\n]+\])+/gi, '$1');
 
-            if ($('#comment').val() != reply_text) {
-                $('#comment').val($('#comment').val() + reply_text);
-            }
-            $.scrollTo($('#comment'), function() { $('#comment').focus(); });
+            $.scrollTo($('#comment'), function() {
+                if ($('#comment').val() != reply_text) {
+                    $('#comment').val($('#comment').val() + reply_text);
+                }
+
+                $('#comment').focus();
+            });
         });
 
     // add comment --> private
@@ -1356,18 +1359,15 @@ $(function() {
             let $target;
 
             if (typeof target === 'string') {
-                $target = $('#' + target);
+                $target = document.getElementById(target);
                 window.location.hash = target;
             } else {
-                $target = target;
-            }
-
-            if ($target.length) {
-                $('main').scrollTop(Math.round($target.position().top) - 20);
+                // Use raw DOM node instead of jQuery
+                $target = target.get(0);
             }
 
-            if (complete) {
-                complete();
+            if ($target) {
+                scroll_element_into_view($target, complete);
             }
         }
 

diff --git a/js/global.js b/js/global.js
index 93f364c9e7343cb35a44c0adcde7f83b2d799344..68aceb03faf8927c5cc30e687b13a3845dde4f5c 100644 (file)
--- a/js/global.js
+++ b/js/global.js
@@ -214,22 +214,64 @@ const detect_blocked_gravatars = () => {
 }
 
 /**
- * If the URL contains a hash like #c10, scroll down the page to show the
- * element below the fixed global header. This workaround is required for
- * comments on show_bug.cgi, components on describecomponents.cgi, etc.
+ * If the current URL contains a hash like `#c10`, adjust the scroll position to
+ * make some room above the focused element.
  */
-const scroll_element_into_view = () => {
+const adjust_scroll_onload = () => {
     if (location.hash) {
-        const $main = document.querySelector('main');
         const $target = document.querySelector(location.hash);
 
         if ($target) {
-            window.setTimeout(() => $main.scrollTop = $target.offsetTop - 20, 50);
+            window.setTimeout(() => scroll_element_into_view($target), 50);
         }
     }
 }
 
+/**
+ * Bring an element into the visible area of the browser window. Unlike the
+ * native `Element.scrollIntoView()` function, this adds some extra room above
+ * the target element. Smooth scroll can be done using CSS.
+ * @param {Element} $target - An element to be brought.
+ * @param {Function} [complete] - An optional callback function to be executed
+ *  once the scroll is complete.
+ */
+const scroll_element_into_view = ($target, complete) => {
+    let top = 0;
+    let $element = $target;
+
+    // Traverse up in the DOM tree to the scroll container of the
+    // focused element, either `<main>` or `<div role="feed">`.
+    do {
+        top += ($element.offsetTop || 0);
+        $element = $element.offsetParent;
+    } while ($element && !$element.matches('main, [role="feed"]'))
+
+    if (!$element) {
+        return;
+    }
+
+    if (typeof complete === 'function') {
+        const callback = () => {
+            $element.removeEventListener('scroll', listener);
+            complete();
+        };
+
+        // Emulate the `scrollend` event
+        const listener = () => {
+            window.clearTimeout(timer);
+            timer = window.setTimeout(callback, 100);
+        };
+
+        // Make sure the callback is always fired even if no scroll happened
+        let timer = window.setTimeout(callback, 100);
+
+        $element.addEventListener('scroll', listener);
+    }
+
+    $element.scrollTop = top - 20;
+}
+
 window.addEventListener('DOMContentLoaded', focus_main_content, { once: true });
 window.addEventListener('load', detect_blocked_gravatars, { once: true });
-window.addEventListener('load', scroll_element_into_view, { once: true });
-window.addEventListener('hashchange', scroll_element_into_view);
+window.addEventListener('load', adjust_scroll_onload, { once: true });
+window.addEventListener('hashchange', adjust_scroll_onload);

diff --git a/template/en/default/bug/navigate.html.tmpl b/template/en/default/bug/navigate.html.tmpl
index 027e3fcf963727dd516705368baf5afce0f010db..862ec7acea90b50f09ec9d24e642b747d67c267d 100644 (file)
--- a/template/en/default/bug/navigate.html.tmpl
+++ b/template/en/default/bug/navigate.html.tmpl
@@ -39,7 +39,7 @@
     </li>
     [%# Links to more things users can do with this bug. %]
     [% Hook.process("links") %]
-    <li>&nbsp;-&nbsp;<a href="#">Top of page </a></li>
+    <li>&nbsp;-&nbsp;<a href="#main-inner">Top of page </a></li>
   </ul>
   [% PROCESS "bug/tagging.html.tmpl" %]
   <hr>