Fix clickable elements with class 'btn' #4566
Conversation
Fixes philc#3550 Update `content_scripts/link_hints.js` to make elements with class 'btn' clickable. * Add logic to `getLocalHintsForElement` to check for class 'btn' and mark elements as clickable. --- For more details, open the [Copilot Workspace session](https://copilot-workspace.githubnext.com/philc/vimium/issues/3550?shareId=XXXX-XXXX-XXXX-XXXX).
content_scripts/link_hints.js
Outdated
| if (!isClickable && className?.toLowerCase().includes("btn")) { | ||
| isClickable = true; | ||
| possibleFalsePositive = true; | ||
| } |
There was a problem hiding this comment.
This could be part of the same if statement by doing something like:
if (!isClickable && (className?.toLowerCase().includes("button") || className?.toLowerCase().includes("btn")))
There was a problem hiding this comment.
Agreed, I've made a change to address this.
…redundant check for class 'button' * Merge the if statement for 'button' and 'btn' classes * Remove redundant check for class 'button'
|
@philc I think this PR is ready for review, let me know what you think |
|
Thanks for the PR. Link hint detection is the main hot/expensive path in Vimium, so we should carefully understand and introduce each change there. Can you provide a few examples of websites which use btn as the class name for elements which aren't already detected by Vimium? |
content_scripts/link_hints.js
Outdated
| // clickables are often wrapped in elements with such class names. So, when we find clickables | ||
| // based only on their class name, we mark them as unreliable. | ||
| const className = element.getAttribute("class"); | ||
| if (!isClickable && className?.toLowerCase().includes("button")) { | ||
| if (!isClickable && (className?.toLowerCase().includes("button") || className?.toLowerCase().includes("btn"))) { |
There was a problem hiding this comment.
We should extract the className and call toLowerCase() on it only once rather than twice.
Thanks Phil. I came across this when using the default bootstrap toggles during development: https://getbootstrap.com/docs/5.3/forms/checks-radios/#toggle-buttons How expensive is the detection, have you been able to profile it before? |
|
On the page you linked, I couldn't find the Bootstrap controls which aren't showing hints -- all of them (except the disabled ones) show hints. Was it for an older version of Bootstrap? Benchmarking requirers running the link hint code on a page several times in a loop (see examples in #3063, #2410). I'm sure your change wouldn't be detectable in the runtime. The cost of this change is more about having more rules, and the documentation for why they're there, in a performance-critical part of the code. |
|
Checking in to note that the two reported cases of this issue (this, and #3550) both reference Bootstrap toggle controls, which uses a label with class "btn" as the control's underlying element. In my tests on the Bootstrap's docs page for that control, this portion of link_hints.js is correctly detecting that the label is clickable: I'm not against merging in this change, but we should have at least one current, reproducible example of an element with a "btn" class in the wild that think should be fixed, as part of making this change, for documentation purposes. |
|
Thanks Phil, turns out the missing links on the bootstrap docs pages were caused by another extension. I had installed vimium-c on a lightweight Linux machine for testing, and my settings sync downloaded it on my main machine as well. That extension is the one with this bug :D I can see the buttons highlighted using vimium's main branch, so I think this PR is not needed anymore. I'm not sure if #3550 refers to any other unseen issues but I don't see it on my end. Thanks again for your help! |
Fixes #3550
Update
content_scripts/link_hints.jsto make elements with class 'btn' clickable.getLocalHintsForElementto check for class 'btn' and mark elements as clickable.For more details, open the Copilot Workspace session.