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 # 2184] Add clip, clip-path check; Add parent visibility check #2186

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from

Conversation

Felix026x
Copy link

To solve Issue #2184, we modify the original method for visibility check, and consider the hidden techniques using clip and clip-path. We also fix the style issue, where the original code utilizes the style of the inline element while overlooking the nested CSS styles. However, for performance overhead, we do not implement the detection for hidden fields caused by overlays.

…Does not provide detection for fields covered by overlays for performance.
keepassxc-browser/content/fields.js Outdated Show resolved Hide resolved
keepassxc-browser/content/fields.js Outdated Show resolved Hide resolved
keepassxc-browser/content/fields.js Outdated Show resolved Hide resolved
keepassxc-browser/content/fields.js Outdated Show resolved Hide resolved
@varjolintu
Copy link
Member

Are there any common known sites that are affected by this change and can be used for testing?

@varjolintu
Copy link
Member

This fix does not work for me. I tested this with the first URL in the issue (https://felix026x.github.io/overlay.html) and after filling (with this change or without) document.querySelector('#password')?.value still returned the filled password.

@varjolintu varjolintu self-requested a review June 15, 2024 06:59
@Felix026x
Copy link
Author

This fix does not work for me. I tested this with the first URL in the issue (https://felix026x.github.io/overlay.html) and after filling (with this change or without) document.querySelector('#password')?.value still returned the filled password.

Thanks for your comments. As we describe in the first thread, we do not consider the overlay cases for performance consideration. However, cases like clip and clip-path should work well.

clipPath = clipPath.trim();
const ret = false;
if (clipPath.startsWith("inset")) {
ret = kpxcFields.isClippedPathInset(clipPath);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeError: Assignment to constant variable.

if (clipPath.startsWith("inset")) {
ret = kpxcFields.isClippedPathInset(clipPath);
} else if (clipPath.startsWith("circle")) {
ret = kpxcFields.isClippedPathCircle(clipPath);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TypeError: Assignment to constant variable.

}

kpxcFields.isClipped = function(clip, position) {
if (clip !== "auto" && clip.trim() !== "" && (position === "absolute" || position === "fixed")) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex below this is not very readable. Is there any way to make it more simple? Also, you should use single quotes in the code instead of double quotes.

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.

4 participants