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

Always use getBoundingClientRect for getViewportTopLeft #2649

Merged
merged 2 commits into from
Sep 15, 2017

Conversation

mrmr1993
Copy link
Contributor

With this PR, we end up working with absolute positions and pixel offsets for everything in link hints, so nothing needs to be scaled.

This should fix all of the link hint zoom/scaling issues. It passes all the testcases I could think of.

Ping @gdh1995 @smblott-github.

@gdh1995
Copy link
Contributor

gdh1995 commented Sep 15, 2017

Good job! In my tests it also works well with various documentElement's styles including margin, zoom, and border.

@smblott-github smblott-github merged commit 29bb5cc into philc:master Sep 15, 2017
@smblott-github
Copy link
Collaborator

Thanks, @mrmr1993.

I'll try whatever tests I can think of later this morning, and push as 1.60.3, if all seems well.

@smblott-github
Copy link
Collaborator

Looks great to me. Thanks again, @mrmr1993.

I pushed 1.60.3.

Fingers crossed.

There some other issues from 1.60 which we need to look at, but nothing as systematic as this hint-positioning thing.

@wchargin
Copy link

Hi there,

Just want to note that, like the original fix to link hint positioning, this breaks link-hint positioning on Chrome <61. That is, every recent version of Vimium has had link-hint positioning working in exactly one of Chrome {60,61}.

With that said, please consider keeping this patch in. If you can find a way for this to work in both versions of Chrome, fantastic—but I've already changed Chrome versions from 60 to 61 back to 60 and now to 61 again, just to get link hints rendering correctly in Vimium. This is pretty annoying, and it'd be great to not have to downgrade once again if this is rolled back.

Thank you! <3

@gdh1995
Copy link
Contributor

gdh1995 commented Sep 16, 2017

@wchargin Could you provide a URL of those pages where Vimium's hints are broken on Chrome 60? I've tested a complexed page on Chrome 60.0.3112.113 (stable, x64) on Win 10 x64, but no errors were found.

@wchargin
Copy link

It was broken on any page on which you zoomed in. For instance, https://news.ycombinator.com/.

However, after downgrading again to the same version of Chrome on which it was previously broken (60.0.3112.90 (Official Build) (64-bit) for Linux x64), and with the same version of Vimium (1.60.3), I'm not able to reproduce this. I don't understand this: I'm absolutely certain that these two versions were previously broken at the time that I wrote the comment, so there must be some factor in play that I'm missing. Hacker News (link above) is the page on which I noticed the issue and is what triggered me to write the comment. The only other remotely related system upgrade that I installed in the meantime was adobe-flashplugin from 1:20170808.1-0ubuntu0.16.04.1 to 1:20170912.1-0ubuntu0.16.04.1, but I assume that that shouldn't have an impact as Vimium doesn't use Flash.

@mrmr1993 mrmr1993 deleted the linkhints-scaled-display-fix branch November 2, 2017 14:56
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

Successfully merging this pull request may close these issues.

None yet

4 participants