keepassxc-browser icon indicating copy to clipboard operation
keepassxc-browser copied to clipboard

Allow autofill and autosubmit configuration on a per site basis

Open A-K-O-R-A opened this issue 1 year ago • 45 comments

This is a continuation of #1700, this time adding buttons in the site preferences table. Given the security implications of the features I think it makes more sense to enable them on a per site basis which this PR makes possible.

image

Notes

The changes to the options.css file are necessary to allow the site preferences table to grow bigger on 4K screens and should not affect any other screen sizes.

Fixes #2087.

A-K-O-R-A avatar Oct 06 '24 19:10 A-K-O-R-A

Actually a plan to redesign this has been on my TODO list for a while. In that pre-design I ended up replacing the separate columns with a combobox that has items with checkboxes. In that way we could save some space. Too many checkboxes everywhere makes the UI look messy, in my opinion. Those wouldn't also work great with a smaller screen.

varjolintu avatar Oct 06 '24 19:10 varjolintu

Agreed these checkboxes make this extremely unusable

droidmonkey avatar Oct 06 '24 20:10 droidmonkey

I agree that this is not the best solution visually but I felt like redesigning the site preferences settings view was out of scope for this feature pull request.

Do you have any suggestions how to solve this problem? I think simply making the view a list of only urls that are expandable sections could solve this problem. By moving all the settings to the expandable sections there would be a lot more space available to list all the individual options for a site config. If you want I can create a small mock up tomorrow showcasing what I mean.

Besides this do you have any other suggestions and feedback? And is this layout problematic a blocker or is there something else I can do to improve the situation?

A-K-O-R-A avatar Oct 06 '24 22:10 A-K-O-R-A

Open to mock ups!

droidmonkey avatar Oct 06 '24 22:10 droidmonkey

The three checkboxes we now have on each row is the maximum amount of them. I'm also open for other ideas if the combobox with checkbox items is not good enough.

varjolintu avatar Oct 07 '24 03:10 varjolintu

The padding and styling is obviously not perfect yet but this is my suggestion:

image image

A-K-O-R-A avatar Oct 07 '24 12:10 A-K-O-R-A

The design could easly be improved by using a flush list instead of a table or even an accordion though I do not really like the UX of being unable to select the url.

A-K-O-R-A avatar Oct 07 '24 12:10 A-K-O-R-A

In my opinion the row should include the URL, plus Edit and Remove buttons. The expanded area could then include the additional settings for the site. It's too complicated if the URL cannot be directly removed from the row.

varjolintu avatar Oct 07 '24 13:10 varjolintu

image

Would this be an acceptable solution if I get the small triangle to be on the left side instead of the top? (That would also fix the big size of the rows)

A-K-O-R-A avatar Oct 07 '24 14:10 A-K-O-R-A

Something like that yes. I would also eliminate the extra need for height by grouping the normal options to the left and the "automatic" options to the right. The "Ignore" setting could actually be on the row too like it's now.

One expanded entry could look like this (no colspan, sorry):

Page URL Ignore Actions
https://example.com Enable all features [Edit] [Delete]
[ ] Username-only Detection [ ] Auto-Submit login forms  
[ ] Improved Input Field Detection [ ] Automatically fill in single-credential entries  
[ ] Allow Cross-Origin iframes [ ] Automatically fill in single TOTP entries  

varjolintu avatar Oct 07 '24 14:10 varjolintu

image

I adopted your suggestion as much as my CSS skills allow. Expanding the entry works by simply clicking anywhere on the list entry which I think is intuitive enough. Also adding more content after the URL would once again result in text wrapping on smaller screens which I think should be avoided, on a 720p screen some of my URLs barely even fit with the current version and would most definitely not fit if the ignore setting was added right after the URL

A-K-O-R-A avatar Oct 07 '24 15:10 A-K-O-R-A

image image

I think putting the ignore selector in the left column looks better than simply putting it on top. Moving the ignore selector to the bottom would result in a more clean UI in my opinion but that might be personal preference and I do not know which of the options gets used more often and should be higher up

A-K-O-R-A avatar Oct 07 '24 16:10 A-K-O-R-A

This is a very crude idea what I had in mind (don't care about the paddings, styles etc.): Screenshot 2024-10-07 at 21 27 11 With flex-wrap you can ensure the content fits the screen even when the width is smaller. One thing I was wondering about the three new checkboxes for automatic fill: how are these handled when then settings are global? Are those enabled/disabled by default by the global setting, but can overridden per preference? Screenshot 2024-10-07 at 21 27 17

I'd prefer to have the Ignore option directly in the row, because then user can directly see if the site is enabled/disabled without expanding any extra settings area.

Another idea I had is just to replace the three checkboxes in the row with a similar kind of selection: Screenshot 2024-10-07 at 21 29 43

Feel free to use these mockups as well for some ideas :)

varjolintu avatar Oct 07 '24 18:10 varjolintu

With flex-wrap you can ensure the content fits the screen even when the width is smaller.

The current solution already uses flex-wrap and does ensure the content fits: image

One thing I was wondering about the three new checkboxes for automatic fill: how are these handled when then settings are global? Are those enabled/disabled by default by the global setting, but can overridden per preference?

Enabling the local settings overrides the global settings being disabled, I don't think any other behavior would be intuitive or useful

A-K-O-R-A avatar Oct 07 '24 19:10 A-K-O-R-A

image

Solved the button alignment and made the URL break text on every character instead of words as I think this is a more sensible default.

What else would be needed for the approval of this PR?

A-K-O-R-A avatar Oct 07 '24 19:10 A-K-O-R-A

What else would be needed for the approval of this PR?

Actual code review & testing after we've decided if this is the style we want to have.

varjolintu avatar Oct 07 '24 19:10 varjolintu

I don't understand the "Ignore" column header anymore, I think that has outlived its purpose. I like this mockup the best:

image

Also the subtext on some of the checkboxes should be moved to tooltips instead:

image

droidmonkey avatar Oct 07 '24 21:10 droidmonkey

I don't understand the "Ignore" column header anymore, I think that has outlived its purpose. I like this mockup the best:

We could add those options as checkboxes, and the URL in the row is dimmed is "Disable all features" is used. At least in that case the URL could clearly see directly from the row that this URL is disabled for the extension.

varjolintu avatar Oct 08 '24 03:10 varjolintu

And/or an icon indicating the state

droidmonkey avatar Oct 08 '24 04:10 droidmonkey

  • changed the code use a global variable like suggested and removed the unnecessary safety checks
  • added comments to the option.html file
  • removed the tooltips and adjusted the layout a bit so the space gets used optimally

image image image

A-K-O-R-A avatar Oct 08 '24 08:10 A-K-O-R-A

We could add those options as checkboxes, and the URL in the row is dimmed is "Disable all features" is used. At least in that case the URL could clearly see directly from the row that this URL is disabled for the extension.

I don't understand the "Ignore" column header anymore, I think that has outlived its purpose. I like this mockup the best:

I like your suggestions and agree that they would be better than the current ignore selector, but I am a bit confused as this is starting to feel like more of a rewrite of the site preferences system instead of just a feature addition. If this the goal then that is totally fine and I am willing to implement the suggestions, I just want to make sure I am not misunderstanding the goal of this pull request.

A-K-O-R-A avatar Oct 08 '24 09:10 A-K-O-R-A

@A-K-O-R-A The goal is not a full redesign. Which is why I'd also prefer the combobox with checkboxes. It's a minimal change and removes the clutter from the URL row.

varjolintu avatar Oct 08 '24 09:10 varjolintu

Ok so the plan is to keep the old table with the horizontal scroll like it was before, right? And then remove the 3 check box rows and add one row with a 6 entry dropdown checkbox or two rows with 3 dropdown entries each?

A-K-O-R-A avatar Oct 08 '24 09:10 A-K-O-R-A

Ok so the plan is to keep the old table with the horizontal scroll like it was before, right? And then remove the 3 check box rows and add one row with a 6 entry dropdown checkbox or two rows with 3 dropdown entries each?

Each row should have only one dropdown with checkboxes as grouped. The disabled options could be done as checkboxes too in a separate group, so 3/3/3 in total.

varjolintu avatar Oct 08 '24 09:10 varjolintu

I hope this does not come across as rude and I am sorry if I maybe misinterpreted things but I am kind of confused about what you want me to do? As of my understanding we still are deciding on the design, do you still want me to make suggestions / create new mockups, because I feel like everyone suggested something slightly different and we explored every possibility but there is no consensus? Is the goal to come to a common consensus with me or will you decide as maintainers what the final design should be and then ping me so I can implement it?

My suggestion would be that I create a mockup of each individual variation discussed in this thread and then we vote on which one is the best. I think we explored all variations and should come to a decision on what is the best design, let me know if you think this is a good idea :)

A-K-O-R-A avatar Oct 08 '24 09:10 A-K-O-R-A

@A-K-O-R-A This is my suggestion: https://github.com/keepassxreboot/keepassxc-browser/pull/2358#issuecomment-2399377060. Does @droidmonkey agree? The whole idea here is to prevent the checbox clutter :)

varjolintu avatar Oct 08 '24 10:10 varjolintu

Well the one thing that hasn't been properly mocked up yet is the combo box drop-down suggestion. @A-K-O-R-A appreciate your efforts, I think @varjolintu can mock up his idea to properly compare. Also, it would be much easier to see the changes if you didn't have to block out the entire table, can you take your screenshots on a new browser profile with "fake" domains?

droidmonkey avatar Oct 08 '24 10:10 droidmonkey

Well the one thing that hasn't been properly mocked up yet is the combo box drop-down suggestion. @A-K-O-R-A appreciate your efforts, I think @varjolintu can mock up his idea to properly compare. Also, it would be much easier to see the changes if you didn't have to block out the entire table, can you take your screenshots on a new browser profile with "fake" domains?

Yes currently working on that, only now saw that there is an import/export options for the settings

A-K-O-R-A avatar Oct 08 '24 10:10 A-K-O-R-A

I think sending all the screenshots again will clutter this conversation so I created a public Figma file to collect all my screenshots in different sizes. I will also try to add any future mock ups to the list, the 3 screen sizes are 4K, 1080p and 720p

https://www.figma.com/design/ZapaHmKj3RdcJkVbPy8XP7/KeePassXC?node-id=0-1&t=z0KjAr0LVkwm7hG1-1

A-K-O-R-A avatar Oct 08 '24 11:10 A-K-O-R-A

Regarding the combo checkbox, this seems possible in Bootstrap but when putting it in the table it does not open for some reason and the behavior when clicking seems really weird https://stackblitz.com/edit/wee2u7?file=index.html

The issue with the combox not opening seems to be the following https://stackoverflow.com/questions/68750341/bootstrap-5-uncaught-typeerror-popper-namespace-createpopper-is-not-a-functi

Replacing the current bootstrap.min.js with bootstrap.bundle.min.js seems to solve the problem, the bootsrap docs also explain this: https://getbootstrap.com/docs/5.3/getting-started/contents/#js-files

UPDATE: I added a mock up of the combobox version to the figma page, you can also check it out on my fork at https://github.com/A-K-O-R-A/keepassxc-browser/tree/mockup_2

A-K-O-R-A avatar Oct 08 '24 11:10 A-K-O-R-A