Adds new check to see if an element is covered by another #2251
Conversation
|
The right-hand Suggested Groups pane on Facebook.... Before...
After...
We're now missing a lot of hints. Any ideas, @poacher2k? |
|
Good find! It seems that the case with these missing links is that the wrapping anchor tag itself doesn't have a any box size, and therefore only its children are found with I'll have to do some thinking here, as this is in theory correct behavior (click candiate is not in fact a visible element), but in practice not quite so. Let me know if you come up with any ideas as well! |
Also adds 0.1 pixels inwards to corner coordinates to handle some edge cases where another element would be found instead
|
@smblott-github I added a fix, and as far as I can see, things are working really smoothly now. Did some more extensive testing on facebook, reddit and github, and it seems to improve things a lot. |
(This message is written by @smblott-github.) This is @poacher2k's philc#2251 rebased, tweaked and squashed into a single commit. It provides a new method of detecting when an overlay hides a clickable element. There is some possibility that this may have to be reverted (for example, if we find we're missing too many important hints). For that reason, I squashed this into a single commit. Closes philc#2251. Adds another check to find out if elem is visible Also adds 0.1 pixels inwards to corner coordinates to handle some edge cases where another element would be found instead Removes redundant contains-check Reverts change that made tests fail Tweak philc#2251 (better overlay detection).
(This message is written by @smblott-github.) This is @poacher2k's philc#2251 rebased, tweaked and squashed into a single commit. It provides a new method of detecting when an overlay hides a clickable element. There is some possibility that this may have to be reverted (for example, if we find we're missing too many important hints). For that reason, I squashed this into a single commit. Closes philc#2251. Adds another check to find out if elem is visible Also adds 0.1 pixels inwards to corner coordinates to handle some edge cases where another element would be found instead Removes redundant contains-check Reverts change that made tests fail Tweak philc#2251 (better overlay detection).
|
@poacher2k... See 5a32ecd. I'm going to try it out for a bit before merging. (Tests aren't passing for now.) |
|
I'm also trying it out, but yeah - I see tests aren't passing. Any clue on why, or hints on how to debug the tests that are failing? |
(This message is written by @smblott-github.) This is @poacher2k's philc#2251 rebased, tweaked and squashed into a single commit. It provides a new method of detecting when an overlay hides a clickable element. There is some possibility that this may have to be reverted (for example, if we find we're missing too many important hints). For that reason, I squashed this into a single commit. Closes philc#2251. Adds another check to find out if elem is visible Also adds 0.1 pixels inwards to corner coordinates to handle some edge cases where another element would be found instead Removes redundant contains-check Reverts change that made tests fail Tweak philc#2251 (better overlay detection).
(This message is written by @smblott-github.) This is @poacher2k's philc#2251 rebased, tweaked and squashed into a single commit. It provides a new method of detecting when an overlay hides a clickable element. There is some possibility that this may have to be reverted (for example, if we find we're missing too many important hints). For that reason, I squashed this into a single commit. Closes philc#2251. Adds another check to find out if elem is visible Also adds 0.1 pixels inwards to corner coordinates to handle some edge cases where another element would be found instead Removes redundant contains-check Reverts change that made tests fail Tweak philc#2251 (better overlay detection).
|
@poacher2k... I'm still using this branch which includes your work tweaked and squashed. Are you too? Do you have any sense as to whether it's faster or slower than the status quo? I good test is to scroll a looooong way down on Twitter or Google Plus and then activate hints. |
|
@smblott-github: I'll do some tests on this after work today! I believe the tweaked version (with only one check instead of two) is definitively faster than the one with two, but you might be talking about the main vimium branch? |
That's what I think. |
|
Then I think we should go for that one. The reason I reverted the tweak was because Travis failed to build with the tweak and succeeded without it. |
|
Tests are fixed over in my branch of this: 1c66503. The problem was that the tests were making too many assumptions about the implementation. |
|
That's great! I feared I had broken something. Will be testing with your branch soon. |
|
I tested on twitter.com scrolled down far enough to make document.body.scrollHeight ~157k, and did some timings from F was pressed until hints appeared. On master: ~0.7s (tweak referring to only having Seems pretty solid to me, will do some more testing when I come back home. |
|
Tried going as far down I could on Twitter, Reddit and Facebook - all sites with loads of elements - and it never really goes more than ~0.9s for the link hints to show. It should be said that at the bottom of Reddit/Twitter, the entire browsing experience gets sluggish - not just vimium. |
|
@poacher2k... Thanks for that. Now, a problem. From Facebook....
|
|
Hmm, I can't reproduce this it seems.
Do you experience it every time? Definitively worrisome, as that's the most important link for sure! |
|
Is there any interest or point in me working this into the current codebase? As far as I can see, this is still an issue that when solved could improve the user experience on several websites. Some examples: |
|
Looking at the #2251 (comment) I realise I actually did reproduce it in #2251 (comment) . Not really sure what is happening here (and the post is long gone), but I believe the "44" label may have been underneath the main post thumbnail and not actually visible although correctly placed. I'll take extra notice of this if/when doing more work on this. |
|
I looked at this and followed the comment history, but couldn't find a the original rationale, or recent examples. However, I did reproduce this modern example from medium. 1st screenshot below is before this patch, 2nd is after. You can see that the 2nd screenshot omits many link hints which aren't clickable by the mouse because they're under an overlay, so they probably shouldn't be exposed by link hints (even though they are indeed clickable if the overlay is removed). I've squashed this to one commit, tidied up some comments, and merged it into master in caf13c2. We can now easily test and revert this if it surfaces many suboptimal edge cases once it hits a wider audience. I didn't want to lose this PR in the upcoming migration away from coffeescript. CC @gdh1995 in case you've modified Vimium-C's linkhints in this way and have any commentary, or want to take this for a test spin before wider release.
|
|
I haven't implemented this, maybe because of concerns about speed / code size. But this indeed looks well, and now I agree that it should be executed. |
|
Wow @philc , this is certainly a pleasant surprise! Thank you so much for looking at this! I've been eagerly wanting this ever since I made the PR in 2016! :) |
|
This seems to break "Traverse shadow DOM to find links #3406" - particularly if the test on element.contains rejects shadow elements... |
This idea is from philc/vimium#2251 and merges some logic in philc/vimium#3474
|
Follow-up discussion about @gnarendran's comment is in #3474 |
|
I believe this change makes me miss a lot of links that I could navigate before without the mouse, now I have to partially use the mouse some times (😞). |
|
@RomainBitard : would be great with some links and screenshots so that those issues can be ironed out in that case :) |
|
@RomainBitard : You are not alone, there are already followup issues #3492 #3493 #3501(closed). But I do not know if these three are the only regressions, or if you have encountered a different one. I guess that is why @poacher2k is requesting your problem links. Regards. |
See philc#2251 for discussion.
Replaces #2166