8000 Fix #5525: add gg/G follows focus setting by bovine3dom · Pull Request #5530 · tridactyl/tridactyl · GitHub
[go: up one dir, main page]
Skip to content

Fix #5525: add gg/G follows focus setting - #5530

Draft
bovine3dom wants to merge 3 commits into
masterfrom
fix_5525
Draft

Fix #5525: add gg/G follows focus setting#5530
bovine3dom wants to merge 3 commits into
masterfrom
fix_5525

Conversation

@bovine3dom
@bovine3dom bovine3dom commented Sep 13, 2026
Copy link
Copy Markdown
Member

Fix #5525

ggscrollsroot is a terrible name and needs to be changed before merging

@bovine3dom bovine3dom mentioned this pull request Sep 13, 2026
Open
@archerhush
archerhush commented Sep 22, 2026
Copy link
Copy Markdown

The current draft does not build because scrollto compares the Promise returned by config.getAsync("ggscrollsroot") directly with "true":

TS2367: This comparison appears to be unintentional because the types 'Promise<any>' and '"true"' have no overlap.

Did you forget to use 'await'?

Since scrollto is synchronous, changing the lookup to config.get(...) fixes the build:

- config.getAsync("ggscrollsroot") === "true"
+ config.get("ggscrollsroot") === "true"

After rebuilding and loading the extension temporarily, I tested:

:set ggscrollsroot false

After ;; focused a link inside my scrollable iframe, j/k scrolled the iframe, and gg/G correctly moved that same iframe context to its top/bottom.

With:

:set ggscrollsroot true

gg and G retained the existing root-document behavior.

So the intended implementation works for my reproduction once the synchronous config lookup is used.

Thank you.

@bovine3dom
bovine3dom commented Sep 22, 2026
Copy link
Copy Markdown
Member Author

yeah sorry i saw i had used the wrong function but didn't think anyone would be looking at this PR and decided i would fix it later

thank you for taking the time to test it

bovine3dom
bovine3dom commented Sep 22, 2026
View reviewed changes
Comment thread src/excmds.ts Resolved
@bovine3dom
bovine3dom commented Sep 22, 2026
Copy link
Copy Markdown
Member Author

I need to look at this again because it's not working like I would expect it to :(

set scrolltofollowfocus true
# G/gg in a nested iframe. the whole page seems to scroll rather than just the most nested frame

@Chic-Tweetz
Chic-Tweetz commented Sep 23, 2026 •
edited
Loading
Copy link
Copy Markdown
Contributor

Chic-Tweetz@44fb645
I've made :scrollto behave more like :scrollpx (and chucked a --target=root|focused check in there)

I also added a getter for the current scroll focus, but that's just because I thought it might be useful to expose after making those commands #5525 (comment)

and that seems to behave pretty well

couple of things stick out though

  • it might be good to add a scrollIntoView in :scrollto if the scrolled element isn't the root, in case it's off-screen
  • in @archerhush's iframe test https://archerhush.github.io/test/iframes.html the iframes don't get scroll focus when doing a normal f hint because the iframes don't exist until a link is selected... not sure there's much that can be done about that
    after an additional ;; hint the nested frames scroll properly

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

needs review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

gg and G do not reliably scroll a separately scrollable area after I focus an element inside it with ;;

3 participants

@bovine3dom @archerhush @Chic-Tweetz
0