search

keepassxreboot / keepassxc-browser

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

Icons are visible on opacity:0 login; margin-top skewing icons #850

Open idontusenumbers opened 4 years ago

idontusenumbers commented 4 years ago

Seems login forms within hidden windows (via opacity 0) still get positioned into and are visible.

Example at https://artinres.com/

Look a the middle of the page image

Additionally on this page theres a margin-top that's moving the position of the body element which screws the alignment of the keepass icons image

Debug info

KeePassXC-Browser Version: 1.6.2 KeePassXC Version: 2.5.3 Operating system: Mac Browser: Chrome

droidmonkey commented 4 years ago

I have noticed this as well

idontusenumbers commented 4 years ago

@varjolintu Does this fix the margin-top induced issue as well?

varjolintu commented 4 years ago

@idontusenumbers Oh, it doesn't. Let's reopen this and make another PR for that.

varjolintu commented 4 years ago

https://github.com/keepassxreboot/keepassxc-browser/pull/840 - I'd suggest the fix is (maybe) made to this PR as it fixes the relative placement.

EDIT: getBoundingClientRect() should get the correct position. In this case it just doesn't handle the content that is added after checking the position.

varjolintu commented 4 years ago

What an annoying problem. 1.7.0 still didn't fix it, but I'm making some slow progress. The problem is that I don't want to cause any extra slowdowns. At this point I can identify when the popup goes away, but it still gives a false positive for the input field visibility. So I need to somehow handle that + CSS animations.

idontusenumbers commented 4 years ago

Instead of placing the keepassxc buttons at the root of the DOM, have you considered placing them as siblings of the text box they are being added to?

varjolintu commented 4 years ago

@idontusenumbers Wouldn't that cause elements or the page content to be resized?

idontusenumbers commented 4 years ago

Setting it to absolute position prevents it from affecting the layout.

It's not a flawless idea though, especially for highly manipulated inputs.

varjolintu commented 4 years ago

@idontusenumbers It's already set to absolute position related to the input field. How would it differ from the current implementation to add the icon element as a sibling and still use position: absolute?

idontusenumbers commented 4 years ago

If it were in the same parent, changes to position, opacity, display:none, or any other tricky manipulations that might effect the rendering on an ancestor would automatically apply to the keepassxc trigger too.

varjolintu commented 4 years ago

@idontusenumbers If that's the case, I need to test if that works. Although I'm not in favor to modify any actual page elements. Now the content script elements are added at the end of the document body.

Chealer commented 5 months ago

@idontusenumbers while these issues are somewhat related, they are also clearly different. Please file a different ticket for each (I guess at this point only the second one since the primary one is solved IIUC).