Include elements with pointer cursor style as clickable links. #3063
Conversation
(this is a follow up to philc#2410) The pointer cursor is a UI hint that something can be clicked on the page. This is often the simplest way to detect if a non-input, non-anchor element should have a hint. For example, the side bar in Feedly or the month overview in the top left of Google Calendar. This change uses `computedStyleMap` to determine the value of an element's cursor. We only want to know the first element that sets its cursor to pointer. All children of that first element will also inherit the style and because this method includes inherited styles we need a special check to make sure a given style is in fact the first pointer in the tree. As for performance, computing the style of an element is relatively expensive. Using `computedStyleMap` over `getComputedStyle` yields a ~25% performance improvement. I also experimented with a cache that saves class names that did not cause an element to have a pointer style. Depending on the number of elements, this improved performance by another ~10% . Ultimately, I left out the caching solution as it comes at the cost of memory, additional code complexity, and increases the number of false negatives, e.g. a `menu` class appears on both the `ul` and `li` tags but only creates a pointer on the `li`. Another option would be to limit the number of times we run `computedStyleMap` to something like 10k times per call to `getLinkHints` I did a similar test suite as in the previous pull request, this time with 100 loops. Every page experienced worse performance (as expected), however all pages (except twitter) had median execution times under 15ms. Twitter's 20k element test had a median of 90ms (control of 43ms) and the 50k element test went from 136ms to 274ms (the caching solution was most helpful here at 195ms).
|
test is failing because phatomjs does not support hasPointerCursor = if element.computedStyleMap?
then (element) -> element.computedStyleMap().get("cursor").value == "pointer"
else (element) -> document.defaultView.getComputedStyle(element, null) == "pointer"
isClickable ||= element.style.cursor == "pointer" or
element.className != "" and
hasPointerCursor(element) and
not hasPointerCursor(element.parentElement)but what is the preferred solution for this? |
|
For anyone interested in getting the functionality of this PR before it's merged (or #3004 is merged), I hacked together an extension to help Vimium find more clickable elements: Vimium Helper - more wasteful of CPU resources, but doesn't impact Vimium performance. |
|
Tyler, great writeup. I appreciate the careful performance benchmarking and commentary. I know you've iterated on this approach many times over the last two years. On the balance, I think it doesn't belong in Vimium main repo:
As I begin to review close the majority of our open issues, this PR is the first place I'm going to return to if it seems like we have a chronic gap in our link hints which can be solved by pointer events. |
|
Since I've created the Vimium Helper workaround, the need for the PR to be merged is lower. Closing is fine by me. As far as how many people are asking for it to be fixed - the helper instruction has 81 avg weekly users and I think the only place it's really advertised is in this PR. I certainly feel the need for it, but that's because my expectation is to never use the mouse, especially for frequent sites and actions. And there are a few sites in my workflow that simply don't have vimium hints without this fix. |
(this is a follow up to #2410)
The pointer cursor is a UI hint that something can be clicked on the page. This is often the simplest way to detect if a non-input, non-anchor element should have a hint. For example, the side bar in Feedly or the month overview in the top left of Google Calendar.
This change uses
computedStyleMapto determine the value of an element's cursor. We only want to know the first element that sets its cursor to pointer. All children of that first element will also inherit the style and because this method includes inherited styles we need a special check to make sure a given style is in fact the first pointer in the tree.As for performance, computing the style of an element is relatively expensive. Using
computedStyleMapovergetComputedStyleyields a ~25% performance improvement. I also experimented with a cache that saves class names that did not cause an element to have a pointer style. Depending on the number of elements, this improved performance by another ~10% . Ultimately, I left out the caching solution as it comes at the cost of memory, additional code complexity, and increases the number of false negatives, e.g. amenuclass appears on both theulandlitags but only creates a pointer on theli. Another option would be to limit the number of times we runcomputedStyleMapto something like 10k times per call togetLinkHintsI did a similar test suite as in the previous pull request, this time with 100 loops. Every page experienced worse performance (as expected), however all pages (except twitter) had median execution times under 15ms. Twitter's 20k element test had a median of 90ms (control of 43ms) and the 50k element test went from 136ms to 274ms (the caching solution was most helpful here at 195ms).