Skip to content
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

Missing link hints when "aria-hidden=true" #3501

Closed
gnarendran opened this issue Feb 15, 2020 · 7 comments
Closed

Missing link hints when "aria-hidden=true" #3501

gnarendran opened this issue Feb 15, 2020 · 7 comments

Comments

@gnarendran
Copy link

gnarendran commented Feb 15, 2020

After the merge of "Adds new check to see if an element is covered by another #2251" in 1.65, hints for links with "aria-hidden=true" became inconsistent/unpredictable. This issue was not present in 1.64.6. This has been already discussed: #3486 (comment) (https://www.theguardian.com/uk) and #3474 (comment) (https://news.google.com/?hl=en-IN&gl=IN&ceid=IN:en). But since those PR/pull have been closed/merged, opening this new PR as a place holder.

(Also this issue is different from #3492 and #3493, though the same merge caused them as well.)

Typically I use vimium as follows: See an interesting link X and press f to generate its link hint. It is ok to me if there are redundant/unclickable hints generated for other links, as long as the hint is generated for X, as I am going to anyway ignore others. But it would be disappointing if unpredictably hint is not generated for X. In other words, it would be nice if vimium errs on the side of generating some redundant links hints rather than on the side of ignoring some useful links.

@gdh1995 has commented on it and seems to already have some fixes for vimium-c for this issue (though I am not sure!). Meanwhile I am taking the easy way out by cloning vimium and locally reverting the commits 4d39729 and caf13c2, so that vimium works seamlessly again. Regards.

@gdh1995
Copy link
Contributor

gdh1995 commented Feb 15, 2020

For whoever is interested in this, here's code analysis:

  • Vimium excludes all elements matching [aria-hidden=true],[aria-hidden=""]
  • some pages use a blank and transparent <a aria-hidden> as a cover of a card
    • I think their intent is to make the whole card become clickable for a real mouse
    • usually, the card also include a normal <a href> behind the cover <a>, to make screen reader happy
    • but a side effect is the behind <a href> won't get hover effects (like the underline on mouse hover)
    • some website turns to use CSS rules to simulate such underline
  • before v1.65, Vimium shows hint markers for behind <a href>s.
  • since v1.65, when Vimium tries to find topest elements, it gets the cover <a aria-hidden>, and this element is not the behind <a href>, so the behind one is thrown away.
    • some other buttons (e.g. the "like" button) are also excluded by this rule

What Vimium C does now: its master branch now skips an [aria-hidden] element if its children is empty, and then behind <a href> might be matched as wanted - not ensured, though.

@lydell
Copy link

lydell commented Feb 16, 2020

Why does Vimium exclude aria-hidden elements? In my experience, sites tend to use it to hide content for non-visual assistive programs (screen readers). But Vimium is a visual tool, so I think it makes sense for Vimium to not care about aria-hidden.

(In my own extension, Link Hints, I don’t care about aria-hidden and I have not noticed getting worse hints because of that. BTW, I also use .elementFromPoint and do get a lot of hints in the problematic sites mentioned above, so I think this is fixable in Vimium without reverting.)

@gdh1995
Copy link
Contributor

gdh1995 commented Feb 16, 2020

According to specification https://www.w3.org/TR/using-aria/#fourth , [aria-hidden="true"] should never be on focusable elements, so the fact is that those websites have misused this feature.

@philc
Copy link
Owner

philc commented Feb 20, 2020

Great discussion everyone, thanks for the help. So there are two proposals: hint elements with aria-hidden=true, or mimic Vimium C's more complex change (commit). I'm inclined to just start ignoring aria-hidden because it's a simple and understandable strategy, and leans towards false positives rather than false negatives, as @gnarendran suggests. @gdh1995, is there a prominent case that motivated you to implement the more sophisticated "cover detection" logic?

I tried this out in #3505 (take it for a spin) and the guardian and google news are much more usable now.

@gdh1995
Copy link
Contributor

gdh1995 commented Feb 20, 2020

Um, it's just because I didn't what it would be if aria-hidden check was removed, and I'm used to add lots of checks to hack few elements.

@gnarendran
Copy link
Author

@philc I tried #3505 and it seems good (didn't notice any missing hint) when used on guardian.co.uk and news.google.com. There are slight differences from 1.64.6 like some hints being positioned at any given link's image (if present) rather than its text, but I guess one will get used to it! Thanks a lot for looking into this. Regards.
vimium-1 64 6
vimium-allow-aria-hidden

@philc
Copy link
Owner

philc commented Feb 21, 2020

Closing this now that I've merged #3505. Feel free to resume discussion if you find undesirable effects. Thanks everyone!

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

No branches or pull requests

4 participants