Add scroll marker using fake scrollbar implementation by arogl · Pull Request #436 · brandon1024/find

arogl · 2026-03-28T02:22:17Z

Fixes

#368

Changes Proposed in this Pull Request:

Add scollbar marker to options
Show markers on scrollbar

Additional Comments and Documentation:

Produced with the help of claude

pzhlkj6612 · 2026-03-28T03:18:56Z

interesting! do you have screenshots?

arogl · 2026-03-28T04:51:17Z

Options screen

No Matches

Matches on news.com.au

It has light and dark scrollbar capability, but not extensively tested

follow existing style.

Less noise.

- Can put "enable" or "disable" in description instead of messages. - Can omit "_enabled" from js code to shorten code lines.

pzhlkj6612

The code and L10N strings in this PR don't seem to be consistent with the existing style and design, and there are some useless if-else and try-catch statements.

I've modified your changes in arogl#1. PLease check. The code in "content/highlighter.js" needs more cleanup and I'm working on it.

You can directly merge that PR if it looks good to you. The merge commit can be preserved, and you don't need to force-push. The maintainer will squash all commits during PR merging.

I believe that keeping things consistent will help future developers. I also believe that we will compose some linter rules for this project then.

cleanup

pzhlkj6612 · 2026-04-17T18:15:49Z

Thank you for merging the code.

Now I'm continuing working on it. I think all markers in the virtual scrollbar should be added and removed simultaneously with the addition or removal of occurrences on the page, then the procedure will look more tidy and more integrated.

brandon1024 · 2026-04-30T09:09:20Z

Let me know when these changes are ready and I’ll have a final review 👍

arogl · 2026-05-01T07:46:09Z

Sorry forgot about this one.

pzhlkj6612 · 2026-05-01T11:29:22Z

Thanks. I'm working on removing all try-catch blocks these days. The code in this PR looks to be against the existing code style.

ref: https://developer.mozilla.org/en-US/docs/Web/API/Document/scroll_event (src: https://github.com/mdn/content/blob/3e5fd6765f891b6fedae20ce1e31e2fdefe55b3c/files/en-us/web/api/document/scroll_event/index.md#scroll-event-throttling)

"document.scrollingElement" has been widely used since 2016.

pzhlkj6612

Thanks. I'm working on removing all try-catch blocks these days. The code in this PR looks to be against the existing code style.

I'm back and finished a mass PR. Please see arogl#2. Commit messages in it are also important.

Cleanup, reorganization, encapsulation.

arogl · 2026-08-06T08:15:05Z

Thanks. I'm working on removing all try-catch blocks these days. The code in this PR looks to be against the existing code style.

I'm back and finished a mass PR. Please see arogl#2. Commit messages in it are also important.

I merged in and tested locally, it worked

brandon1024 · 2026-08-06T08:52:34Z

Let me know when this is ready for final review 👍

pzhlkj6612

Let me know when this is ready for final review 👍

Looks ready for final review! Let's move on and have this feature in the upcoming new version.

brandon1024 · 2026-08-12T09:42:27Z

                    for (let currIndex = 0; currIndex < len; currIndex++) {
                        charMap[charIndexMap[offset + currIndex]].ignorable = true;
                    }
-


Can you revert these whitespace changes? I don't like seeing code all bunched up like this, too difficult to follow.

Can you revert these whitespace changes? I don't like seeing code all bunched up like this, too difficult to follow.

Addressed by arogl#3.

Maybe @arogl is not familiar with or forget to read git-diff. It's very clear that we have some unneeded changes there: 2f047ec~1...bb8795a .

brandon1024 · 2026-08-12T09:48:53Z

Changes look good to me, aside from the whitespace comment. I'll do a few tests locally, but should be able to merge soon.

brandon1024 · 2026-08-12T09:56:41Z

LGTM. Once the whitespace fixes are done, can you squash your changes? I'll merge once done.

pzhlkj6612 · 2026-08-12T19:34:05Z

I'm cleaning unimportant changes in files such as manifest_firefox.json.

JS: - We don't need to move the "charMap.length = count" line. - We don't need to remove those empty lines and affect readability a little bit? JSON: - Really, copy-paste is quick, but git-diff is a bit hard to read. - The EOF empty line is a pain for GitHub since the Web UI editor will always add that line. We should keep it.

pzhlkj6612

We're almost there!

In my view, there are still some minor flaws in this code. The comments below are for good changes, while my suggestions are in the PR: arogl#3

pzhlkj6612 · 2026-08-12T22:46:42Z

            }
        }

-        // Collect occurrence IDs from highlight spans


nice clean. that was a leftover.

pzhlkj6612 · 2026-08-12T22:50:42Z

-        if (options && options.scroll_markers) {
-            scrollbarMaker.setActive(index);
+        if (options.scroll_markers) {
+            Find.Content.ScrollbarHighlightMaker.setActive(index);


Nice removal of the undefined check with options since we have already checked it above.

pzhlkj6612 · 2026-08-12T22:59:31Z

@brandon1024

LGTM. Once the whitespace fixes are done, can you squash your changes? I'll merge once done.

Maybe we can keep the history in this PR? When merging PR, you're able to choose "squash and merge", and then all of our reviews and comments are still meaningful and the "develop" branch should have only one commit. Force-pushing is not recommended, at least in my opinion.

Docs: Merging a pull request - GitHub Docs | https://docs.github.com/en/pull-requests/how-tos/merge-and-close-pull-requests/merging-a-pull-request#merging-a-pull-request .

Restore unneeded changes from JS and JSON files

pzhlkj6612

we're almost there.

pzhlkj6612 · 2026-08-13T08:39:33Z

+    /**
+     * Initialize the component with the given search options, discarding
+     * any previously mounted overlay. Must be called before mount().
+     *
+     * @private
+     * @param {object} newOptions - The search and highlight options
+     * */
+    self.init = function (newOptions) {
+        self.destroy();
+        options = newOptions;
+        occTopPositionMap = new Map();
+    };


Suggested change

/**

* Initialize the component with the given search options, discarding

* any previously mounted overlay. Must be called before mount().

*

* @private

* @param {object} newOptions - The search and highlight options

* */

self.init = function (newOptions) {

self.destroy();

options = newOptions;

occTopPositionMap = new Map();

};

/**

* Initialize the component with the given search options, Must be

* called before mount().

*

* @private

* @param {object} newOptions - The search and highlight options

* */

self.init = function (newOptions) {

options = newOptions;

occTopPositionMap = new Map();

};

We already have a Find.Content.ScrollbarHighlightMaker.destroy() call in "self.restore()" of "content/highlighter.js", so any leftover will be cleaned up. So, we can safely remove it from here.

So, we can safely remove it from here.

So, we can safely remove the self.destroy() call from here.

Comments should be updated, too.

I don't know if the suggested changes above are readable on your web browser? GitHub's UI is glitch sometime, so you may need to refresh it multiple times.

pzhlkj6612 · 2026-08-13T08:56:02Z

+        if (overlay && overlay.parentNode) {
+            overlay.parentNode.removeChild(overlay);
+        }
+        if (globalStyle && globalStyle.parentNode) {
+            globalStyle.parentNode.removeChild(globalStyle);
+        }


Maybe we can apply lint rules in the future for using the convenient "Optional chaining" stuff? I dunno. I prefer optional chaining. Now let's don't change it and keep the style consistent with other code lines. Here I'm just making notes.

pzhlkj6612 · 2026-08-13T09:17:28Z

+#find-ext-scroll-marker-container {
+    position: absolute;
+    top: 0;
+    left: 0;
+    right: 0;
+    height: 100%;
+}


Mouse can't drag and drop the thumb, unless we do this:

Suggested change

#find-ext-scroll-marker-container {

position: absolute;

top: 0;

left: 0;

right: 0;

height: 100%;

}

#find-ext-scroll-marker-container {

position: absolute;

top: 0;

left: 0;

right: 0;

height: 100%;

pointer-events: none;

}

(Then the same declaration for each marker in .find-ext-scroll-marker {...} can be removed.)

Embarrassingly, my previous implementation didn't care about the style of the marker container, and then the mouse could normally interact with the thumb. Bug was hidden.

HOWEVER, since we don't have any overflow related things applied to the container, a zero-pixel-width and zero-pixel-height DIV does not hurt anything. Directly remove this rule from here and the corresponding class naming thing from "scrollbar.js" looks simpler and more acceptable to me.

Suggested change

#find-ext-scroll-marker-container {

position: absolute;

top: 0;

left: 0;

right: 0;

height: 100%;

}

What do you thing?

06be275: directly removing the marker container is not a bad idea but looks like my intent about using that container didn't express clearly. Let me explain more:

No track.querySelectorAll(...), but a simple markerContainer.children. The code will be more understandable to future developers including all of us a month from now.

Managing common properties is simple. For example, the pointer-events: none; declaration.

Better dev experience: in Developer Tools, we can collapse all makers into one parent element and see a clear view, like this:

Thus, if you're going to inspect something other than markers, for example the thumb, then those markers won't bother you.

That's it. I really recommend you use a container.

pzhlkj6612

It works. Now let's wait for @brandon1024 to test this PR.

arogl added 2 commits March 28, 2026 11:58

Add scroll marker fake scrollbar implementation

e6fa339

Update Japanese locale, and fix formatting changes

9e2ff52

pzhlkj6612 added 5 commits April 8, 2026 08:53

Remove period mark from ja strings

b305601

follow existing style.

Fix wrong level of content in table

e4d4f64

Revert code style changes

d7892ea

Less noise.

Change names and translations to match existing style

b3a6649

- Can put "enable" or "disable" in description instead of messages. - Can omit "_enabled" from js code to shorten code lines.

Remove useless tr-catch and null-check statements

4e5c7ff

pzhlkj6612 suggested changes Apr 8, 2026

View reviewed changes

Merge pull request #1 from pzhlkj6612/arogl/cleanup

f8cc623

cleanup

Ensure highlights add and remove simultaneously

90a1b6a

pzhlkj6612 added 14 commits August 5, 2026 15:06

Remove all try-catch

db50cbe

Remove unused parameters

b705b0e

Remove an empty line

e9296d7

Make scrollbar width const

6e7538c

Remove unreliable page color check; use only one style

d9cddc1

Use local variables

bd73d98

Extract common things from the code about scrollbar; readability

65da005

Process existing elements directly instead; reduce fallbacks

5e83b02

Query markers one time only when updating style for the active one

32cbbb0

Add or remove injected "find-ext-scrollbar-style" directly

e42a39d

Remove a fallback from "createScrollMarker"

c6efb60

Simplify by using returned element from "appendChild()"

6f0b50e

Manage merged styles in ShadowRoot

f96cc3f

pzhlkj6612 added 5 commits August 5, 2026 19:20

Reduce DOM traversal by calculating markerTop during matching checking

bddcd36

Drop meaningless fallbacks or checks

d29c894

"document.scrollingElement" has been widely used since 2016.

Restore dark-mode detection by media query; hard-code colors for now

5bf97ff

Object-Oriented Programming: ScrollbarHighlightMaker

9905f25

Minor refactors for variable naming and comments

4602a4b

pzhlkj6612 suggested changes Aug 5, 2026

View reviewed changes

Merge pull request #2 from pzhlkj6612/arogl/cleanup

c4a5c0a

Cleanup, reorganization, encapsulation.

pzhlkj6612 approved these changes Aug 7, 2026

View reviewed changes

brandon1024 requested changes Aug 7, 2026

View reviewed changes

Comment thread content/highlighter.js

Comment thread content/highlighter.js Outdated

Comment thread content/highlighter.js Outdated

Comment thread content/highlighter.js Outdated

arogl added 3 commits August 9, 2026 15:36

Break scrollbar into separate files

2f047ec

Update manifest for new files

ac6826f

cur/paste error :sad:

bb8795a

arogl mentioned this pull request Aug 12, 2026

Add eslint rules and tests #439

Draft

brandon1024 reviewed Aug 12, 2026

View reviewed changes

pzhlkj6612 suggested changes Aug 12, 2026

View reviewed changes

Merge pull request #3 from pzhlkj6612/arogl/cleanup

0fe26e5

Restore unneeded changes from JS and JSON files

pzhlkj6612 suggested changes Aug 13, 2026

View reviewed changes

arogl added 2 commits August 14, 2026 11:23

Address scrollbar comments

06be275

I think this fixes the last comments.

b85f94d

arogl added a commit to arogl/find that referenced this pull request Aug 14, 2026

Update scrollbar files after brandon1024#436 feedback

2933056

pzhlkj6612 approved these changes Aug 14, 2026

View reviewed changes

Conversation

arogl commented Mar 28, 2026

Fixes

Changes Proposed in this Pull Request:

Additional Comments and Documentation:

Uh oh!

pzhlkj6612 commented Mar 28, 2026

Uh oh!

arogl commented Mar 28, 2026

Uh oh!

pzhlkj6612 left a comment

Choose a reason for hiding this comment

Uh oh!

pzhlkj6612 commented Apr 17, 2026

Uh oh!

brandon1024 commented Apr 30, 2026

Uh oh!

arogl commented May 1, 2026

Uh oh!

pzhlkj6612 commented May 1, 2026

Uh oh!

pzhlkj6612 left a comment

Choose a reason for hiding this comment

Uh oh!

arogl commented Aug 6, 2026

Uh oh!

brandon1024 commented Aug 6, 2026

Uh oh!

pzhlkj6612 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brandon1024 commented Aug 12, 2026

Uh oh!

brandon1024 commented Aug 12, 2026

Uh oh!

pzhlkj6612 commented Aug 12, 2026

Uh oh!

pzhlkj6612 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pzhlkj6612 commented Aug 12, 2026

Uh oh!

pzhlkj6612 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pzhlkj6612 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development