Find Mode: Fix scrolling to matches in scrollable containers (e.g., GitLab)
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
- Is the simplicity of scrollIntoView() worth the temporary DOM insertion?
- Are there edge cases where scrollIntoView() behaves unexpectedly?
- Does Option B need to recursively scroll multiple ancestors?
- Is there any other solution for this issue?
Contributor guide
First steps
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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