philc / philc/vimium

Find Mode: Fix scrolling to matches in scrollable containers (e.g., GitLab)

Open
#4,835 1 comment 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
JavaScript
Stars
27k
Forks
2.6k
PR merge metrics
No merged PRs in 30d

Description

Problem

I was trying to fix issue #4827, which is about finding mode fails to scroll to search results on websites that use scrollable container elements instead of window scrolling (e.g., GitLab). The matched text is found and highlighted correctly, but the viewport doesn't scroll to reveal it.

Root cause: The original implementation used globalThis.scrollTo() which only scrolls the main window. On sites like GitLab, content is inside a nested scrollable <div>, so window.scrollY stays at 0.

Proposed Solutions
Option A: Use scrollIntoView() (Simpler)

const highlight = (textNode, startIndex, length) => {
  // ... selection setup ...
  
  // Use scrollIntoView to properly handle scrollable containers.
  const span = document.createElement("span");
  range.insertNode(span);
  span.scrollIntoView({
    behavior: "instant",
    block: "center",
  });
  span.remove();
  // Restore selection after DOM manipulation
  selection.removeAllRanges();
  selection.addRange(range);
  return true;
};

Pros:

  • Simple, minimal code
  • Browser handles all scrollable ancestor detection automatically
  • Works for deeply nested scrollable containers

Cons:

  • Requires temporary DOM insertion/removal
  • Always scrolls (even if element is already visible) — though scrollIntoView is optimized
  • Less explicit control over scroll behavior

Option B: Manual scrollable parent detection (More explicit)

const getScrollableParent = (element) => {
  let node = element;
  while (
    node && (node instanceof Element) && (node !== document.body) &&
    (node !== document.documentElement)
  ) {
    const style = globalThis.getComputedStyle(node);
    const overflowY = style.overflowY;
    if (
      ((overflowY === "auto") || (overflowY === "scroll")) &&
      (node.scrollHeight > node.clientHeight)
    ) {
      return node;
    }
    node = node.parentElement;
  }
  return null;
};
const highlight = (textNode, startIndex, length) => {
  // ... selection setup ...
  let rect = range.getBoundingClientRect();
  const scrollParent = getScrollableParent(textNode.parentElement);
  if (scrollParent) {
    const parentRect = scrollParent.getBoundingClientRect();
    if ((rect.top < parentRect.top) || (rect.bottom > parentRect.bottom)) {
      const offset = (rect.top - parentRect.top) + (rect.height / 2) -
        (scrollParent.clientHeight / 2);
      scrollParent.scrollBy({ top: offset, behavior: "instant" });
      rect = range.getBoundingClientRect();
    }
  }
  // Fallback to window scroll
  if (rect.top < 0 || rect.bottom > globalThis.innerHeight) {
    globalThis.scrollTo({
      top: globalThis.scrollY + rect.top + rect.height / 2 - screenHeight / 2,
      behavior: "instant",
    });
  }
  return true;
};

Pros:

  • No DOM manipulation required
  • Explicit visibility checks (only scrolls when necessary)
  • Handles both container scroll AND window scroll as fallback
  • More predictable behavior

Cons:

  • More code to maintain
  • Only finds the first scrollable ancestor (may miss deeply nested cases)
  • Manual visibility calculation could have edge cases

Questions for Discussion

  1. Is the simplicity of scrollIntoView() worth the temporary DOM insertion?
  2. Are there edge cases where scrollIntoView() behaves unexpectedly?
  3. Does Option B need to recursively scroll multiple ancestors?
  4. Is there any other solution for this issue?

Contributor guide

Open the contributing guide

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start at the Find Mode highlight implementation shown in the issue and reproduce the failure on a site such as GitLab with nested scrollable containers. Compare the proposed scrolling approaches, then verify that matched text is revealed inside its container while existing window scrolling still works.

Written by the indexing model from the issue text.

Assessment

Tech stack
javascript
Domain
frontend
Issue type
Bug
Difficulty
4/5
Estimated time
3-5 days
Activity status
Active
Clarity
Mostly clear
Newbie friendliness
48/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.