Skip to content

Add linkText to AlphabetHints #4689

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

danielcovill
Copy link
Contributor

This should make it easier to discern scroll/frame markers from links. It's a feature that has been only available with Filter hints enabled. I may be missing something, but I see no reason why it shouldn't be visible when using the default Alphabet hints as well.

#4675

@itpropro
Copy link

@philc any feedback?

@philc
Copy link
Owner

philc commented Jun 25, 2025 via email

@philc philc changed the title Address issue #4675: Add linkText to AlphabetHints Add linkText to AlphabetHints Jun 29, 2025
@philc
Copy link
Owner

philc commented Jun 29, 2025

UX comments:

Adding "scroll" and "frame" to the hint text can add a lot of clutter to the page and obscure nearby hints. The UX rationale for only having it in filtered hints has been because the clutter in that mode is less of a problem, because you can very quickly type a few characters that you see in the anchor, or in the hint itself, to hide the many other hints on the page. With alphabet hints, you can't quickly filter out the clutter like this.

Showing "scroll" and "frame" in filtered hints isn't my favorite because it's not self-explanatory. However, at least that mode has the convention of showing the name of some links in the hints (e.g. for form controls). In alphabet hints, having the word "scroll" appear next to a hint feels even less self-explanatory and more random, because I expect the content of the hints to only be characters that I should type to navigate to the link.

I think a change in color might work better, or some other UX. Any thoughts?

Feedback about this PR:

  1. Which page are you primarily testing this on?
  2. If this PR, there should only be link text shown for frames and scrollables. I noticed the values of the form controls on the Vimium options page are also showing link text. See screenshot.
Screenshot 2025-06-28 at 6 36 07 PM

Also, I found an adjacent bug which is unrelated to this PR. I tested this change on test_harnesses/iframe.html in the Vimium repo. On that page, after hitting space to toggle the hints, typing S navigates to the first link starting with an S, but instead it should wait for further characters to be typed. Update: I believe I fixed this in 544b0ce.

@@ -792,18 +792,21 @@ class AlphabetHints {
this.hintKeystrokeQueue = [];
}

renderMarker(marker) {
let linkText = marker.linkText;
console.log(linkText);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Log statement left in.

fillInMarkers(hintMarkers) {
const hintStrings = this.hintStrings(hintMarkers.length);
if (hintMarkers.length != hintStrings.length) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this safety check no longer needed?

= added 2 commits July 5, 2025 21:00
- Added 'scroll-hint-marker' class for frames and scrollable areas in link hints.
- Implemented overlap detection and offset adjustments for overlapping hint markers.
- Updated CSS for scroll hint markers to improve visibility.
@danielcovill
Copy link
Contributor Author

danielcovill commented Jul 8, 2025

  • Thanks for the callouts on the review, both the log statement and safety check were sloppy mistakes. I've addressed those.
  • I am now testing using test_harnesses/iframe.html and the Vimium options page.
  • I revisited and changed the approach based on your comments. I've stopped appending the text and when a link is "Scroll." or "Frame." I'm changing the color of the hint-marker instead.
  • Additionally, I found that the hint markers for scroll/frame frequently end up stacked behind other markers so I added a bit of code to check for that scenario and add a slight offset (2px down and right) to any markers that are in the exact same spot. I think it provides a nice indication that there are markers behind one another.

Happy to remove that last bit if it exceeds the scope of this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants