Bug 1440229 - [devtools] Scroll to first search result matching Range in Markup view. r=devtools-reviewers,jdescottes.

When navigating through search results, we were selecting nodes and scrolling
them into view, centered vertically.
This is not working well in some cases:
- if the node itself is taller than the markup view, we're scrolling in the middle
  of the node and might miss the actual search results
- if the node is wider than the markup view, since we're not scrolling horizontally,
  we might miss a search result at the very right of the node

To fix those issue, we're taking advantage of `nsISelectionController.scrollSelectionIntoView`,
putting the first search result Range in the selection so we scroll to the right location.
It's okay to put the Range in the selection since the user can't have any text
selected at this point.

A couple test cases are added to cover this.

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

Author: nchevobbe

devtools/client/inspector/markup/markup.js

@@ -1077,7 +1077,18 @@ MarkupView.prototype = {
 
     const slotted = selection.isSlotted();
     const smoothScroll = reason === "reveal-from-slot";
-    const onShow = this.showNode(selection.nodeFront, { slotted, smoothScroll })
+    const selectionSearchQuery = selection.getSearchQuery();
+
+    const onShow = this.showNode(selection.nodeFront, {
+      slotted,
+      smoothScroll,
+      // Don't scroll if we selected the node from the search, we'll scroll to the first
+      // matching Range done in _updateSearchResultsHighlightingInSelectedNode.
+      // This need to be done there because the matching Range might be out of screen,
+      // for example if the node is very tall, or if the markup view overflows horizontally
+      // and the Range is located near the right end of the node container.
+      scroll: !selectionSearchQuery,
+    })
       .then(() => {
         // We could be destroyed by now.
         if (this._destroyed) {
@@ -1088,7 +1099,7 @@ MarkupView.prototype = {
         const container = this.getContainer(selection.nodeFront, slotted);
         this._markContainerAsSelected(container);
         this._updateSearchResultsHighlightingInSelectedNode(
-          selection.getSearchQuery()
+          selectionSearchQuery
         );
 
         // Make sure the new selection is navigated to.
@@ -1111,6 +1122,20 @@ MarkupView.prototype = {
     return highlights.get(highlightName);
   },
 
+  /**
+   * @returns {nsISelectionController}
+   */
+  _getSelectionController() {
+    if (!this._selectionController) {
+      // QueryInterface can be expensive, so cache the controller.
+      this._selectionController = this.win.docShell
+        .QueryInterface(Ci.nsIInterfaceRequestor)
+        .getInterface(Ci.nsISelectionDisplay)
+        .QueryInterface(Ci.nsISelectionController);
+    }
+    return this._selectionController;
+  },
+
   /**
    * Highlight search results in the markup view.
    *
@@ -1136,6 +1161,8 @@ MarkupView.prototype = {
     searchQuery = searchQuery.toLowerCase();
     const searchQueryLength = searchQuery.length;
     let currentNode = treeWalker.nextNode();
+    let scrolled = false;
+
     while (currentNode) {
       const text = currentNode.textContent.toLowerCase();
       let startPos = 0;
@@ -1152,10 +1179,47 @@ MarkupView.prototype = {
         searchHighlight.add(range);
 
         startPos = index + searchQuery.length;
+
+        // We want to scroll the first matching range into view
+        if (!scrolled) {
+          // We want to take advantage of nsISelectionController.scrollSelectionIntoView,
+          // so we need to put the range in the selection. That's fine to do here because
+          // in this situation the user shouldn't have any text selected
+          const selection = this.win.getSelection();
+          selection.removeAllRanges();
+          selection.addRange(range);
+
+          const selectionController = this._getSelectionController();
+          selectionController.scrollSelectionIntoView(
+            selectionController.SELECTION_NORMAL,
+            selectionController.SELECTION_ON,
+            selectionController.SCROLL_SYNCHRONOUS |
+              selectionController.SCROLL_VERTICAL_CENTER
+          );
+          selection.removeAllRanges();
+          scrolled = true;
+        }
       }
 
       currentNode = treeWalker.nextNode();
     }
+
+    // It can happen that we didn't find a Range for a search result (e.g. if the matching
+    // string is in a cropped attribute). In such case, go back to scroll the container
+    // into view.
+    if (!scrolled) {
+      const container = this.getContainer(
+        this.inspector.selection.nodeFront,
+        this.inspector.selection.isSlotted()
+      );
+      scrollIntoViewIfNeeded(
+        container.editor.elt,
+        // centered
+        true,
+        // smoothScroll
+        false
+      );
+    }
   },
 
   /**
@@ -1770,23 +1834,38 @@ MarkupView.prototype = {
   /**
    * Make sure the given node's parents are expanded and the
    * node is scrolled on to screen.
-   */
-  showNode(node, { centered = true, slotted, smoothScroll = false } = {}) {
-    if (slotted && !this.hasContainer(node, slotted)) {
+   *
+   * @param {NodeFront} nodeFront
+   * @param {Object} options
+   * @param {Boolean} options.centered
+   * @param {Boolean} options.scroll
+   * @param {Boolean} options.slotted
+   * @param {Boolean} options.smoothScroll
+   * @returns
+   */
+  showNode(
+    nodeFront,
+    { centered = true, scroll = true, slotted, smoothScroll = false } = {}
+  ) {
+    if (slotted && !this.hasContainer(nodeFront, slotted)) {
       throw new Error("Tried to show a slotted node not previously imported");
     } else {
-      this._ensureNodeImported(node);
+      this._ensureNodeImported(nodeFront);
     }
 
     return this._waitForChildren()
       .then(() => {
         if (this._destroyed) {
           return Promise.reject("markupview destroyed");
         }
-        return this._ensureVisible(node);
+        return this._ensureVisible(nodeFront);
       })
       .then(() => {
-        const container = this.getContainer(node, slotted);
+        if (!scroll) {
+          return;
+        }
+
+        const container = this.getContainer(nodeFront, slotted);
         scrollIntoViewIfNeeded(container.editor.elt, centered, smoothScroll);
       }, this._handleRejectionIfNotDestroyed);
   },
@@ -2642,6 +2721,7 @@ MarkupView.prototype = {
     this._elt.innerHTML = "";
     this._elt = null;
 
+    this._selectionController = null;
     this.controllerWindow = null;
     this.doc = null;
     this.highlighters = null;

devtools/client/inspector/markup/test/browser_markup_search_01.js

@@ -121,6 +121,99 @@ add_task(async function () {
   await onSearchCleared;
 
   checkHighlightedSearchResults(inspector, []);
+
+  await searchFor("TALLTOPMATCH", inspector);
+  const talltopNodeFront = await getNodeFront("section.talltop", inspector);
+  const talltopNodeFrontChildren =
+    await inspector.walker.children(talltopNodeFront);
+  is(
+    inspector.selection.nodeFront,
+    talltopNodeFrontChildren.nodes[0],
+    `The section.talltop text node is selected`
+  );
+  checkHighlightedSearchResults(inspector, ["TALLTOPMATCH"]);
+
+  await searchFor("TALLBOTTOMMATCH", inspector);
+  const tallbottomNodeFront = await getNodeFront(
+    "section.tallbottom",
+    inspector
+  );
+  const tallbottomNodeFrontChildren =
+    await inspector.walker.children(tallbottomNodeFront);
+  is(
+    inspector.selection.nodeFront,
+    tallbottomNodeFrontChildren.nodes[0],
+    `The section.tallbottom text node is selected`
+  );
+  checkHighlightedSearchResults(inspector, ["TALLBOTTOMMATCH"]);
+
+  await searchFor("OVERFLOWSMATCH", inspector);
+  const overflowsNodeFront = await getNodeFront("section.overflows", inspector);
+  const overflowsNodeFrontChildren =
+    await inspector.walker.children(overflowsNodeFront);
+  is(
+    inspector.selection.nodeFront,
+    overflowsNodeFrontChildren.nodes[0],
+    "The section.overflows text node is selected"
+  );
+  checkHighlightedSearchResults(inspector, ["OVERFLOWSMATCH"]);
+
+  info(
+    "Check that matching node with non-visible search result are still being scrolled to"
+  );
+  // Scroll to top to make sure the node isn't in view at first
+  const markupViewContainer = inspector.markup.win.document.documentElement;
+  markupViewContainer.scrollTop = 0;
+  markupViewContainer.scrollLeft = 0;
+
+  const croppedAttributeContainer = await getContainerForSelector(
+    "section#cropped-attribute",
+    inspector
+  );
+  let croppedAttributeContainerRect =
+    croppedAttributeContainer.elt.getBoundingClientRect();
+
+  ok(
+    croppedAttributeContainerRect.y < 0 ||
+      croppedAttributeContainerRect.y > markupViewContainer.clientHeight,
+    "section#cropped-attribute container is not into view before searching for a match in its attributes"
+  );
+
+  await searchFor("croppedvalue", inspector);
+  is(
+    inspector.selection.nodeFront,
+    await getNodeFront("section#cropped-attribute", inspector),
+    "The section#cropped-attribute element is selected"
+  );
+  checkHighlightedSearchResults(inspector, []);
+  // Check that node visible after it was selected
+  croppedAttributeContainerRect =
+    croppedAttributeContainer.elt.getBoundingClientRect();
+
+  Assert.greaterOrEqual(
+    croppedAttributeContainerRect.y,
+    0,
+    `Node with cropped attributes is not above visible viewport`
+  );
+  Assert.less(
+    croppedAttributeContainerRect.y,
+    markupViewContainer.clientHeight,
+    `Node with cropped attributes is not below visible viewport`
+  );
+
+  // Sanity check to make sure the markup view does overflow in both axes. We need to
+  // wait after the search is done as their text node is only revealed when cycling through
+  // search results.
+  Assert.greater(
+    markupViewContainer.scrollHeight,
+    markupViewContainer.clientHeight,
+    "Markup view overflows vertically"
+  );
+  Assert.greater(
+    markupViewContainer.scrollWidth,
+    markupViewContainer.clientWidth,
+    "Markup view overflows horizontally"
+  );
 });
 
 async function searchFor(selector, inspector) {
@@ -133,12 +226,47 @@ async function searchFor(selector, inspector) {
 }
 
 function checkHighlightedSearchResults(inspector, expectedHighlights) {
+  const searchInputValue = getSelectorSearchBox(inspector).value;
+
+  info(`Checking highlights for "${searchInputValue}" search`);
+  const devtoolsHighlights = [
+    ...inspector.markup.win.CSS.highlights
+      .get(DEVTOOLS_SEARCH_HIGHLIGHT_NAME)
+      .values(),
+  ];
   Assert.deepEqual(
-    [
-      ...inspector.markup.win.CSS.highlights
-        .get(DEVTOOLS_SEARCH_HIGHLIGHT_NAME)
-        .values(),
-    ].map(range => range.toString()),
-    expectedHighlights
+    devtoolsHighlights.map(range => range.toString()),
+    expectedHighlights,
+    `Got expected highlights for "${searchInputValue}"`
   );
+
+  if (expectedHighlights.length) {
+    const markupViewContainer = inspector.markup.win.document.documentElement;
+    info(
+      `Check that we scrolled so the first highlighted range for "${searchInputValue}" is visible`
+    );
+    const [rect] = devtoolsHighlights[0].getClientRects();
+    const { x, y } = rect;
+
+    Assert.greaterOrEqual(
+      y,
+      0,
+      `First "${searchInputValue}" match not above visible viewport`
+    );
+    Assert.less(
+      y,
+      markupViewContainer.clientHeight,
+      `First "${searchInputValue}" match not below visible viewport`
+    );
+    Assert.greaterOrEqual(
+      x,
+      0,
+      `First "${searchInputValue}" match not before the "left border" of the visible viewport`
+    );
+    Assert.less(
+      x,
+      markupViewContainer.clientWidth,
+      `First "${searchInputValue}" match not after the "right border" of the visible viewport`
+    );
+  }
 }

devtools/client/inspector/markup/test/doc_markup_search.html

@@ -11,5 +11,30 @@
     <button type="button" class="Button">OK</button>
     <p>Click the button</p>
   </section>
+  <section class="talltop"></section>
+  <section class="tallbottom"></section>
+  <section class="overflows">
+    thisisaverylongtextnodesowegetthemarkupviewtooverflowhorizontallythisisaverylongtextnodesowegetthemarkupviewtooverflowhorizontallythisisaverylongtextnodesowegetthemarkupviewtooverflowhorizontallyOVERFLOWSMATCH
+  </section>
+  <section id="cropped-attribute"></section>
+  <script>
+    "use strict";
+
+    const sentence = "this is a very tall text node so we can check that search highlights are properly scrolled into view.";
+    const textNodeContent = sentence.repeat(100);
+
+    // Split the search term in 2 here so we only have a single result when searching for it
+    document.querySelector(".talltop").append(document.createTextNode("TALLTOP" + "MATCH" + textNodeContent));
+    document.querySelector(".tallbottom").append(document.createTextNode(textNodeContent + "TALLBOTTOM" + "MATCH"));
+
+    // Keep in sync with devtools.markup.collapseAttributeLength
+    const croppedAttributeEl = document.querySelector("#cropped-attribute")
+    const collapseAttributeLength = 120;
+    // Splitting in 2 so this won't be seen as a match
+    const strToMatch = "cropped" + "value";
+    const middle = Math.floor(collapseAttributeLength / 2);
+    const cls = "-".repeat(middle) + strToMatch + "-".repeat(middle);
+    croppedAttributeEl.classList.add(cls);
+  </script>
 </body>
 </html>