open in non-active tab (`gf`, `C`) and open multiple links (`cf`) are missing link hints compared to `f`

[vogler]

SurfingKeys: 0.9.63
Browser: Chrome 81.0.4044.122
URL: https://www.golem.de/

Context

On some websites (e.g. https://www.golem.de), gf and C do not show all link hints, wheras f and cf do. I would expect them to behave the same.

Example for f which shows hints for the images and titles:

and C which is missing those hints:

[alexalekseyenko]

createHintsForClick is defined such that, when tabbed is not set to true, a different method is used for finding candidate elements:

Surfingkeys/content_scripts/hints.js

Lines 348 to 370 in fa10d12

	if (behaviours.tabbed) {
	elements = Array.from(getElements('a[href]:not([href^=javascript])'));
	elements = filterInvisibleElements(elements);
	} else {
	if (cssSelector === "") {
	elements = getVisibleElements(function(e, v) {
	if (isElementClickable(e)) {
	v.push(e);
	}
	});
	elements = filterOverlapElements(elements);
	} else if (Array.isArray(cssSelector)) {
	elements = filterInvisibleElements(cssSelector);
	} else {
	elements = getVisibleElements(function (e, v) {
	if (e.matches(cssSelector) && !e.disabled && !e.readOnly) {
	v.push(e);
	}
	});
	elements = filterInvisibleElements(elements);
	elements = filterOverlapElements(elements);
	}
	}

and neither 'cf:

Surfingkeys/pages/default.js

Lines 126 to 128 in 80929a3

	mapkey('cf', '#1Open multiple links in a new tab', function() {
	Hints.create("", Hints.dispatchMouseClick, {multipleHits: true});
	});

nor 'f':

Surfingkeys/content_scripts/normal.js

Lines 1068 to 1075 in fa10d12

	self.mappings.add("f", {
	annotation: "Open a link, press SHIFT to flip overlapped hints, hold SPACE to hide hints",
	feature_group: 1,
	repeatIgnore: true,
	code: function() {
	Hints.create("", Hints.dispatchMouseClick);
	}
	});

set tabbed to true.

In the case of 'cf', this seems like it might be an oversight, given that we are actually opening the links in new tabs, so I would propose the following change:
#1370
In the case of 'f', it's less clear what the correct behavior should be. Currently, it's using the isElementClickable logic:

Surfingkeys/content_scripts/utils.js

Lines 51 to 55 in 4a274fe

	function isElementClickable(e) {
	var cssSelector = "a, button, select, input, textarea, summary, *[onclick], *[contenteditable=true], *.jfk-button, *.goog-flat-menu-button, *[role=button], *[role=link], *[role=menuitem], *[role=option], *[role=switch], *[role=tab], *[role=checkbox], *[role=combobox], *[role=menuitemcheckbox], *[role=menuitemradio]";
	if (runtime.conf.clickableSelector.length) {
	cssSelector += ", " + runtime.conf.clickableSelector;
	}

which doesn't always produce the results I expect either.

@brookhong
What is the intended behavior for hints with tabbed: true vs hints with tabbed: false? They often generate different numbers of hints, but it's not always clear why.

[brookhong]

When tabbed is true, current implementation expects that there is href on an element for Surfingkeys to open it in new tab, please see https://github.com/brookhong/Surfingkeys/blob/master/content_scripts/hints.js#L592-L600. Theose clickable elements without href will not be included in hints with tabbed: true, since it may not make sense to open it in a new tab, for example, those page-inside javascript actions.

Yes, this kind of assumption is not accurate, since some elements without href do have a target URL, but I currently haven't had a good approach for that. Any suggestion that does work is welcomed.

[alexalekseyenko]

I think I found a problem in the filterOverlapElements function. When testing whether the result of elementFromPoint:

Surfingkeys/content_scripts/utils.js

Line 267 in 4a274fe

var el = document.elementFromPoint(be.left + be.width/2, be.top + be.height/2);

contains the element in question:

Surfingkeys/content_scripts/utils.js

Line 268 in 4a274fe

return !el || el.shadowRoot && el.childElementCount === 0 || el.contains(e) || e.contains(el);

el.contains(e)

can return false when e is tucked away inside shadow roots. For instance, 'f' will incorrectly filter out anchor links on this page.

We could do a recursive outward search like so:

function isContainedWithin(inner, outer) {
  if (!inner) {
    return false;
  }
  if (outer.contains(inner)) {
    return true;
  }
  return isContainedWithin(inner.getRootNode().host, outer);
}

It doesn't seem to be much slower on that particular page, but I haven't done a proper analysis of the performance. What do you think?

[alexalekseyenko]

In case it's helpful, this is what I'm proposing:
#1374

[vogler]

@brookhong How about also including those links without href in the hints, but clicking on them with ctrl/cmd pressed?
Of course this may lead to situations where the link is erroneously opened in the current tab. Maybe filter them by cursor: pointer to reduce the number of those cases. Or display those hints in a different color to indicate that they might open in the current tab.

Alternatively, one might try to cover some of the most common approaches like onclick="location.href = ..." and extract the URL to open in a new tab - Idk if that's already done.

[DavidGretzschel]

To clarify (and update) the issue:

The title should make the distinction between the two separate behaviours in the f-group and the cf-group (which includes cf, gf, af and C).

f-group:

cf, gf, af, C group:

This is a big issue for me.
It makes it really hard for me to intuitively use the more efficient vim-style navigation, since I can't predict what can be navigated to.
And I'm afraid, that the latter group being so pared down, forces me to use the mouse, anyway, since I can't replace ctrl-click apparently.
[I suppose, I could get around that with pressing yt to create a tab-copy and navigate to where I want from there, but it's convoluted and some sites don't load fast copies either.]

Afraid, I don't have the insight to solve the technical problem underlying it.
Just wanted to clarify the issue a little and emphasize, that it's very important to me.

[vogler]

@DavidGretzschel What is af?
Seems like the behavior changed since I opened the issue - can confirm cf is also missing hints.

[DavidGretzschel]

af is "open link in new active tab". You can press ? to see an overview of all commands. Am Mi., 15. Dez. 2021 um 16:33 Uhr schrieb Ralf Vogler < ***@***.***>:

…

@DavidGretzschel <https://github.com/DavidGretzschel> What is af? Seems like the behavior changed since I opened the issue - can confirm cf is also missing hints. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1221 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AFVBKOIUCR4BSRUCL3NDLX3URCYMDANCNFSM4MVV3GCQ> .

[vogler]

Ah, I remapped a - I thought ? would still show the composite mappings, but apparently it's smart about filtering unreachable strings.

map('a', 'E'); // af: link in active tab, ab: bookmark to selected folder
map('s', 'R'); // search selected with *, edit url, markdown, show last action...

[onguarde]

i actually prefer less hints on some websites. eg. google search. Reduced visual clutter and easier to press one letter instead of 2 letters to navigate to a link.

cf, (open multiple links in background)

f, (go to link in current tab)

some links does not even make sense to be clicked on, so it's just annoying clutter.

making this a customisable option may be a good idea, with sane defaults.

[b0o]

@onguarde You can create hints yourself using CSS selectors. Here's one I use for Google search results:

// Using an array just to make the code more readable
const googleSearchResultSelector = [
  "a h3",
  "h3 a",
  "a[href^='/search']:not(.fl):not(#pnnext,#pnprev):not([role]):not(.hide-focus-ring)",
  "g-scrolling-carousel a",
  ".rc > div:nth-child(2) a",
  ".kno-rdesc a",
  ".kno-fv a",
  ".isv-r > a:first-child",
  ".dbsr > a:first-child",
  ".X5OiLe",
  ".WlydOe",
  ".fl",
].join(",")
api.mapkey("of", "Select Google result", () => api.Hints.create(googleSearchResultSelector), { domain: /google\.com/i })

You can select more elements if you want, just inspect the Google results page and figure out the selectors you need. Unfortunately, Google's class names are a nightmare.

[onguarde]

@b0o that looks interesting, thanks for sharing.
i'm just not so keen to maintain the nightmarish class names. With search cards in the result page, it will be quite complex to cover all bases.

cf, (open multiple links in background) seems to be a good enough generic solution.