Bug 1953394 - [devtools] Scroll to center new pause location with column being out of the viewport. r=devtools-reviewers,bomsy

Before this patch, it would only make the column visible on the edge of the viewport,
so that we could only see the breakpoint marker, and/or only the first character after the pause location.

With this patch, if the paused location is visible, nothing happens.
But if the paused location is outside of the viewport, the paused location is centered horizontally.

Differential Revision: https://phabricator.services.mozilla.com/D241173

devtools/client/debugger/test/mochitest/browser_aj.toml

@@ -185,6 +185,8 @@ fail-if = ["a11y_checks"] # Bug 1849028 clicked element may not be focusable and
 ["browser_dbg-editor-scroll.js"]
 skip-if = ["cm5"]
 
+["browser_dbg-editor-horizontal-scroll.js"]
+
 ["browser_dbg-editor-select.js"]
 
 ["browser_dbg-ember-original-variable-mapping-notifications.js"]

devtools/client/debugger/test/mochitest/browser_dbg-editor-horizontal-scroll.js

@@ -0,0 +1,180 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at <http://mozilla.org/MPL/2.0/>. */
+
+// Tests that the editor scrolls correctly when pausing on location that
+// requires horizontal scrolling.
+
+"use strict";
+
+add_task(async function testHorizontalScrolling() {
+  if (!isCm6Enabled) {
+    ok(true, "This test is disabled on CM5");
+    return;
+  }
+
+  // Ensure having the default fixed height, as it can impact the number of displayed lines
+  await pushPref("devtools.toolbox.footer.height", 250);
+
+  // Also set a precise size for side panels, as it can impact the number of displayed columns
+  await pushPref("devtools.debugger.start-panel-size", 300);
+  await pushPref("devtools.debugger.end-panel-size", 300);
+
+  // Strengthen the test by ensuring we always use the same Firefox window size.
+  // Note that the inner size is the important one as that's the final space available for DevTools.
+  // The outer size will be different based on OS/Environment.
+  const expectedWidth = 1280;
+  const expectedHeight = 1040;
+  if (
+    window.innerWidth != expectedWidth ||
+    window.innerHeight != expectedHeight
+  ) {
+    info("Resize the top level window to match the expected size");
+    const onResize = once(window, "resize");
+    const deltaW = window.outerWidth - window.innerWidth;
+    const deltaH = window.outerHeight - window.innerHeight;
+    const originalWidth = window.outerWidth;
+    const originalHeight = window.outerHeight;
+    window.resizeTo(expectedWidth + deltaW, expectedHeight + deltaH);
+    await onResize;
+    registerCleanupFunction(() => {
+      window.resizeTo(originalWidth, originalHeight);
+    });
+  }
+  is(window.innerWidth, expectedWidth);
+
+  const dbg = await initDebugger(
+    "doc-editor-scroll.html",
+    "scroll.js",
+    "long.js"
+  );
+
+  await selectSource(dbg, "horizontal-scroll.js");
+  const editor = getCMEditor(dbg);
+
+  const global = editor.codeMirror.contentDOM.ownerGlobal;
+  const font = new global.FontFace(
+    "Ahem",
+    "url(chrome://mochitests/content/browser/devtools/client/debugger/test/mochitest/examples/Ahem.ttf)"
+  );
+  const loadedFont = await font.load();
+  global.document.fonts.add(loadedFont);
+
+  is(global.devicePixelRatio, 1);
+  is(global.browsingContext.top.window.devicePixelRatio, 1);
+  global.browsingContext.top.overrideDPPX = 1;
+  is(global.browsingContext.fullZoom, 1);
+  is(global.browsingContext.textZoom, 1);
+
+  // /!\ Change the Codemirror font to use a fixed font across all OSes
+  // and always have the same number of characters displayed.
+  // Note that this devtools mono makes the "o" characters almost invisible.
+  editor.codeMirror.contentDOM.style.fontFamily = "Ahem";
+  editor.codeMirror.contentDOM.style.fontSize = "10px";
+  editor.codeMirror.contentDOM.style.lineHeight = "15px";
+  editor.codeMirror.contentDOM.style.fontWeight = "normal";
+  editor.codeMirror.contentDOM.style.fontStyle = "normal";
+  editor.codeMirror.contentDOM.style.fontStretch = "normal";
+  is(global.getComputedStyle(editor.codeMirror.contentDOM).fontFamily, "Ahem");
+
+  await wait(1000);
+
+  is(
+    Math.round(editor.codeMirror.dom.getBoundingClientRect().width),
+    679,
+    "Sanity check to ensure we have a fixed editor width, so that we have the expected displayed columns"
+  );
+
+  // All the following methods lookup for first/last visible position in the current viewport.
+  // Also note that the element at the returned position may only be partially visible.
+  function getFirstVisibleColumn() {
+    const { x, y } = editor.codeMirror.dom.getBoundingClientRect();
+    const gutterWidth =
+      editor.codeMirror.dom.querySelector(".cm-gutters").clientWidth;
+    // This is hardcoded to match the second line, which is around 20px from the top.
+    // Also append the gutter width as it would pick hidden columns displayed behind it
+    const pos = editor.codeMirror.posAtCoords({
+      x: x + gutterWidth + 2,
+      y: y + 20,
+    });
+    // /!\ the column is 0-based while lines are 1-based
+    return pos - editor.codeMirror.state.doc.lineAt(pos).from;
+  }
+  function getLastVisibleColumn() {
+    const { x, y, width } = editor.codeMirror.dom.getBoundingClientRect();
+    // This is hardcoded to match the second line, which is around 20px from the top
+    const pos = editor.codeMirror.posAtCoords({ x: x + width, y: y + 20 });
+    // /!\ the column is 0-based while lines are 1-based
+    return pos - editor.codeMirror.state.doc.lineAt(pos).from;
+  }
+
+  info("Pause in middle of the screen, we should not scroll on pause");
+  await addBreakpoint(dbg, "horizontal-scroll.js", 2, 25);
+  invokeInTab("horizontal");
+  await waitForPaused(dbg);
+
+  const lastColumn = getLastVisibleColumn();
+  is(lastColumn, 55);
+  ok(
+    isScrolledPositionVisible(dbg, 2, 1),
+    "The 2nd line, first column is visible"
+  );
+  ok(
+    !isScrolledPositionVisible(dbg, 2, lastColumn),
+    "The 2nd line, last column is partially visible and considered hidden"
+  );
+  ok(
+    isScrolledPositionVisible(dbg, 2, lastColumn - 1),
+    "The column before the last column is visible"
+  );
+
+  info("Step to the last visible column, the editor shouldn't scroll");
+  // This breakpoint location is on the last visible column and would not cause a scroll.
+  await addBreakpoint(dbg, "horizontal-scroll.js", 2, lastColumn);
+  await resume(dbg);
+  await waitForPaused(dbg);
+
+  is(getLastVisibleColumn(), lastColumn, "We did not scroll horizontaly");
+  ok(
+    !isScrolledPositionVisible(dbg, 2, lastColumn),
+    "The last column is still considered hidden"
+  );
+  ok(
+    isScrolledPositionVisible(dbg, 2, lastColumn - 1),
+    "The column before the last colunm is still visible"
+  );
+
+  info(
+    "Step to the next column, and the editor should scroll it into the center"
+  );
+
+  info("Step into the next breakable column, the editor should now scroll");
+  // Set a breakpoint to the next breakable position (there is one every two columns, and lastColumn was breakable)
+  await addBreakpoint(dbg, "horizontal-scroll.js", 2, lastColumn + 2);
+  await resume(dbg);
+  await waitForPaused(dbg);
+
+  const lastColumn2 = getLastVisibleColumn();
+
+  is(lastColumn2, 74);
+  ok(
+    isScrolledPositionVisible(dbg, 2, lastColumn2),
+    "The new last column is visible"
+  );
+  ok(
+    !isScrolledPositionVisible(dbg, 2, lastColumn2 + 1),
+    "The column after the last is hidden"
+  );
+  const firstColumn = getFirstVisibleColumn();
+  is(firstColumn, 30);
+  ok(
+    !isScrolledPositionVisible(dbg, 2, firstColumn),
+    "The new first column is partially visible and considered hidden"
+  );
+  ok(
+    isScrolledPositionVisible(dbg, 2, firstColumn + 1),
+    "The column after the first visible is visible"
+  );
+
+  await resume(dbg);
+});

devtools/client/debugger/test/mochitest/browser_dbg-editor-scroll.js

@@ -89,6 +89,33 @@ add_task(async function testIsPositionVisible() {
   // Ensure having the default fixed height, as it can impact the number of displayed lines
   await pushPref("devtools.toolbox.footer.height", 250);
 
+  // Also set a precise size for side panels, as it can impact the number of displayed columns
+  await pushPref("devtools.debugger.start-panel-size", 300);
+  await pushPref("devtools.debugger.end-panel-size", 300);
+
+  // Strengthen the test by ensuring we always use the same Firefox window size.
+  // Note that the inner size is the important one as that's the final space available for DevTools.
+  // The outer size will be different based on OS/Environment.
+  const expectedWidth = 1280;
+  const expectedHeight = 1040;
+  if (
+    window.innerWidth != expectedWidth ||
+    window.innerHeight != expectedHeight
+  ) {
+    info("Resize the top level window to match the expected size");
+    const onResize = once(window, "resize");
+    const deltaW = window.outerWidth - window.innerWidth;
+    const deltaH = window.outerHeight - window.innerHeight;
+    const originalWidth = window.outerWidth;
+    const originalHeight = window.outerHeight;
+    window.resizeTo(expectedWidth + deltaW, expectedHeight + deltaH);
+    await onResize;
+    registerCleanupFunction(() => {
+      window.resizeTo(originalWidth, originalHeight);
+    });
+  }
+  is(window.innerWidth, expectedWidth);
+
   const dbg = await initDebugger(
     "doc-editor-scroll.html",
     "scroll.js",
@@ -98,18 +125,20 @@ add_task(async function testIsPositionVisible() {
   await selectSource(dbg, "scroll.js");
   const editor = getCMEditor(dbg);
 
-  function getFirstLine() {
+  // All the following methods lookup for first/last visible position in the current viewport.
+  // Also note that the element at the returned position may only be partially visible.
+  function getFirstVisibleLine() {
     const { x, y } = editor.codeMirror.dom.getBoundingClientRect();
     // Add a pixel as we may be on the edge of the previous line which is hidden
     const pos = editor.codeMirror.posAtCoords({ x, y: y + 1 });
     return editor.codeMirror.state.doc.lineAt(pos).number;
   }
-  function getLastLine() {
+  function getLastVisibleLine() {
     const { x, y, height } = editor.codeMirror.dom.getBoundingClientRect();
     const pos = editor.codeMirror.posAtCoords({ x, y: y + height });
     return editor.codeMirror.state.doc.lineAt(pos).number;
   }
-  const lastLine = getLastLine();
+  const lastLine = getLastVisibleLine();
 
   is(
     lastLine,
@@ -145,13 +174,13 @@ add_task(async function testIsPositionVisible() {
   await resume(dbg);
 
   info(
-    "Set a breakpoint on the last partially visibible line, it should scroll that line in the middle of the viewport"
+    "Set a breakpoint on the last partially visible line, it should scroll that line in the middle of the viewport"
   );
   await addBreakpoint(dbg, "scroll.js", lastLine);
   invokeInTab("line" + lastLine);
   await waitForPaused(dbg);
 
-  const newLastLine = getLastLine();
+  const newLastLine = getLastVisibleLine();
   is(newLastLine, 16, "The new last line is the 16th");
   ok(
     !isScrolledPositionVisible(dbg, newLastLine),
@@ -161,7 +190,7 @@ add_task(async function testIsPositionVisible() {
     isScrolledPositionVisible(dbg, newLastLine - 1),
     "The line before is reported as visible"
   );
-  const firstLine = getFirstLine();
+  const firstLine = getFirstVisibleLine();
   is(firstLine, 6);
   ok(
     isScrolledPositionVisible(dbg, firstLine),
@@ -181,7 +210,7 @@ add_task(async function testIsPositionVisible() {
   invokeInTab("line50");
   await waitForPaused(dbg);
 
-  const newLastLine2 = getLastLine();
+  const newLastLine2 = getLastVisibleLine();
   is(newLastLine2, 55);
   ok(
     !isScrolledPositionVisible(dbg, newLastLine2),
@@ -191,7 +220,7 @@ add_task(async function testIsPositionVisible() {
     isScrolledPositionVisible(dbg, newLastLine2 - 1),
     "The line before is visible"
   );
-  const firstLine2 = getFirstLine();
+  const firstLine2 = getFirstVisibleLine();
   is(firstLine2, 45);
   ok(
     isScrolledPositionVisible(dbg, firstLine2),

devtools/client/debugger/test/mochitest/examples/doc-editor-scroll.html

@@ -10,6 +10,7 @@
 
   <body>
     <script src="scroll.js"></script>
+    <script src="horizontal-scroll.js"></script>
     <script src="long.js"></script>
     <script src="frames.js"></script>
   </body>

devtools/client/debugger/test/mochitest/examples/horizontal-scroll.js

@@ -0,0 +1,2 @@
+let a="a",b="b",c="c",d="d",e="e",f="f",g="g",h="h",i="i",j="j",k="k",l="l",m="m",n="n",o="o",p="p",q="q",r="r",s="s",t="t",u="u",v="v",x="x",y="y",z="z";
+function horizontal() { console.log("horizontal");a;b;c;d;e;f;g;h;i;j;k;l;m;n;o;p;q;r;s;t;u;v;x;y;z;a;b;c;d;e;f;g;h;i;j;k;l;m;n;o;p;q;r;s;t;u;v;x;y;z;a;b;c;d;e;f;g;h;i;j;k;l;m;n;o;p;q;r;s;t;u;v;x;y;z;a;b;c;d;e;f;g;h;i;j;k;l;m;n;o;p;q;r;s;t;u;v;x;y;z }

devtools/client/shared/sourceeditor/editor.js

@@ -3458,13 +3458,18 @@ class Editor extends EventEmitter {
       // `coordsAtPos` returns the absolute position of the line/column location
       // so that we have to ensure comparing with same absolute position for
       // CodeMirror DOM Element.
+      //
+      // Note that it may return the coordinates for a column breakpoint marker
+      // so it may still report as visible, if the marker is on the edge of the viewport
+      // and the displayed character at line/column is actually hidden after the scrollable area.
       const coords = cm.coordsAtPos(pos);
       if (!coords) {
         return false;
       }
       const { x, y, width, height } = cm.dom.getBoundingClientRect();
+      const gutterWidth = cm.dom.querySelector(".cm-gutters").clientWidth;
 
-      inXView = withinBounds(coords.left - x, 0, width);
+      inXView = coords.left > x + gutterWidth && coords.right < x + width;
       inYView = coords.top > y && coords.bottom < y + height;
     } else {
       const { top, left } = cm.charCoords({ line, ch: column }, "local");
@@ -3548,6 +3553,7 @@ class Editor extends EventEmitter {
    * @param {Number} line - The line in the source
    * @param {Number} column - The column in the source
    * @param {String|null} yAlign - Optional value for position of the line after the line is scrolled.
+   *                               (Used by `scrollEditorIntoView` test helper)
    */
   async scrollTo(line, column, yAlign) {
     if (this.isDestroyed()) {
@@ -3566,7 +3572,7 @@ class Editor extends EventEmitter {
         }
         return cm.dispatch({
           effects: EditorView.scrollIntoView(offset, {
-            x: "nearest",
+            x: "center",
             y: yAlign || "center",
           }),
         });