keepassxreboot / keepassxc-browser

KeePassXC Browser Extension
GNU General Public License v3.0
1.78k stars 188 forks source link

Fix the coordinate shift caused by CSS zoom #2341

Closed Xia0xia0Xia0 closed 1 month ago

Xia0xia0Xia0 commented 2 months ago

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()
varjolintu commented 2 months ago

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.

Xia0xia0Xia0 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 2 months ago

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 commented 1 month ago

I have removed the browser version check.