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

Fix the coordinate shift caused by CSS zoom #2341

Merged
merged 7 commits into from
Oct 5, 2024

Conversation

Xia0xia0Xia0
Copy link
Contributor

Fixed the coordinate shift caused by css:zoom

keepassxc-browser/content/ui.js

 + kpxcUI.browserVerUpon128
 # kpxcUI.setIconPosition()

keepassxc-browser/content/autocomplete.js

# kpxcUI.setIconPosition()

keepassxc-browser/content/custom-fields-banner.js

# kpxcCustomLoginFieldsBanner.markFields()
# kpxcCustomLoginFieldsBanner.setSelectionPosition()

Fixed the coordinate shift caused by css:zoom 

+ kpxcUI.browserVerUpon128
@ kpxcUI.setIconPosition()
Fixed the coordinate shift caused by css:zoom

@ kpxcUI.setIconPosition()
Fixed the coordinate shift caused by css:zoom

@ kpxcCustomLoginFieldsBanner.markFields()
@ kpxcCustomLoginFieldsBanner.setSelectionPosition()
@varjolintu varjolintu self-requested a review September 22, 2024 08:59
@varjolintu varjolintu added the bug label Sep 22, 2024
@varjolintu varjolintu added this to the 1.9.4 milestone Sep 22, 2024
@varjolintu
Copy link
Member

When looking at https://developer.chrome.com/release-notes/128 it seems that they were using a non-standard CSS zoom before version 128. Because Chromium based browsers are usually up-to-date quite soon, I think the version check for Chrome 128 can be removed. After all this only affects the CSS zoom property and nothing else.

@varjolintu varjolintu changed the title Fixed the coordinate shift caused by css:zoom Fix the coordinate shift caused by CSS zoom Sep 22, 2024
@Xia0xia0Xia0
Copy link
Contributor Author

Xia0xia0Xia0 commented Sep 23, 2024

When looking at https://developer.chrome.com/release-notes/128 it seems that they were using a non-standard CSS zoom before version 128. Because Chromium based browsers are usually up-to-date quite soon, I think the version check for Chrome 128 can be removed. After all this only affects the CSS zoom property and nothing else.

I think the check for browser above 128 can not be canceled, because chromium below 128 handle getBoundingClientRect/getClientRects differently from chromium above 128. We can't guarantee that all users use browser above 128.

BTW, Firefox also supports CSS zoom property in version 126+ (https://developer.mozilla.org/en-US/docs/Web/CSS/zoom), and the latest ESR version is 128.0, so the necessary version check can not be canceled.

@varjolintu
Copy link
Member

I think the check for browser above 128 can not be canceled, because chromium below 128 handle getBoundingClientRect/getClientRects differently from chromium above 128. We can't guarantee that all users use browser above 128.

BTW, Firefox also supports CSS zoom property in version 126+ (https://developer.mozilla.org/en-US/docs/Web/CSS/zoom), and the latest ESR version is 128.0, so the necessary version check can not be canceled.

Usually the ESR version is enough for Firefox. And btw, you are not even checking the version 126 in the function. What I meant is that this bug is touching a very small area of users, which means the version check might not be necessary at all.

@Xia0xia0Xia0
Copy link
Contributor Author

Usually the ESR version is enough for Firefox. And btw, you are not even checking the version 126 in the function. What I meant is that this bug is touching a very small area of users, which means the version check might not be necessary at all.

Most of Firefox's third-party forked browsers are based on the ESR version, so I chose ESR version 128 instead of CSS zoom property's minimum compatible version 126.

Some third-party chromium browsers have custom versions lower than 128, such as Supermium, centBrowser, etc. I'd like to know why we won't make the extension compatible with browsers lower than 128? The number of such user groups is not small.

@varjolintu
Copy link
Member

As I said, the bug is quite minor and I highly doubt it's worthwhile to check against the browser version. If the latest Firefox ESR already supports the CSS zoom, that is good enough. Third-party Chromium browsers have the responsibility to keep their browsers up-to-date anyway.

@Xia0xia0Xia0
Copy link
Contributor Author

So can we take a step back and remove browser version checking when Chromium fully implements MV3, and keep browser version checking until then?

@varjolintu
Copy link
Member

varjolintu commented Sep 23, 2024

So can we take a step back and remove browser version checking when Chromium fully implements MV3, and keep browser version checking until then?

Chromium has been MV3-only already for some time. Is it even related to this issue?

@Xia0xia0Xia0
Copy link
Contributor Author

Xia0xia0Xia0 commented Sep 24, 2024

I have removed the browser version check.

@varjolintu varjolintu self-requested a review October 5, 2024 06:06
@varjolintu varjolintu merged commit 4792d44 into keepassxreboot:develop Oct 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants