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

Allow autofill and autosubmit configuration on a per site basis #2358

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

Conversation

A-K-O-R-A
Copy link

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.

@varjolintu
Copy link
Member

varjolintu commented Oct 6, 2024

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.

@droidmonkey
Copy link
Member

Agreed these checkboxes make this extremely unusable

@A-K-O-R-A
Copy link
Author

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?

@droidmonkey
Copy link
Member

Open to mock ups!

@varjolintu
Copy link
Member

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.

@A-K-O-R-A
Copy link
Author

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

image
image

@A-K-O-R-A
Copy link
Author

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.

@varjolintu
Copy link
Member

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.

@A-K-O-R-A
Copy link
Author

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)

@varjolintu
Copy link
Member

varjolintu commented Oct 7, 2024

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  

@A-K-O-R-A
Copy link
Author

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
Copy link
Author

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

@varjolintu
Copy link
Member

varjolintu commented Oct 7, 2024

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 :)

}

#sitePreferencesTable > li > details > summary > div > div:first-child::before {
content: '►';
Copy link
Member

Choose a reason for hiding this comment

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

If you want to go this way, you could just use <details> and <summary>. The About page is already using those standard HTML elements.

Copy link
Author

Choose a reason for hiding this comment

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

The problem is that when I am using a list instead of a table, like I showed in the previous screenshots then the small triangle will move on top of the text as you can see here. Thats why I did that, this way the delete button will wrap with the text if there is no space, instead of taking up half of the horizontal space the entire time. While it looks a bit more inconsistent I think this is better than the previously used horizontal scrolling as for me that felt like a very bad user experience.
image

Copy link
Member

Choose a reason for hiding this comment

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

FYI: In my mockup I used <details> and <summary> only for the single cell.

Copy link
Author

Choose a reason for hiding this comment

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

I agree that the delete button now possibly moving to the left is not optimal but I do not have enough bootstrap and flexbox knowledge to solve this problem, if you have any ideas I would be very happy to try those out. I think that going with the current list approach instead of a table would be better if the current problem could be solved. But if you think that having a table with a strict separation line is better than we could go back to a table.

Copy link
Member

@varjolintu varjolintu Oct 7, 2024

Choose a reason for hiding this comment

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

I would also want to see a simpler list implementation instead of an older table. It would be easier to manage from JavaScript too. I can try make some mockups later and see if I have the same issue.

EDIT: Made a few tests and it seems the table is much easier to manage in this case.

@A-K-O-R-A
Copy link
Author

A-K-O-R-A commented Oct 7, 2024

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
Copy link
Author

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?

@varjolintu
Copy link
Member

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.

@A-K-O-R-A
Copy link
Author

  • 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
Copy link
Author

A-K-O-R-A commented Oct 8, 2024

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.

@varjolintu
Copy link
Member

@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.

@A-K-O-R-A
Copy link
Author

A-K-O-R-A commented Oct 8, 2024

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?

@varjolintu
Copy link
Member

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.

@A-K-O-R-A
Copy link
Author

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 :)

@varjolintu
Copy link
Member

varjolintu commented Oct 8, 2024

@A-K-O-R-A This is my suggestion: #2358 (comment). Does @droidmonkey agree? The whole idea here is to prevent the checbox clutter :)

@droidmonkey
Copy link
Member

droidmonkey commented Oct 8, 2024

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?

@A-K-O-R-A
Copy link
Author

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
Copy link
Author

A-K-O-R-A commented Oct 8, 2024

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
Copy link
Author

A-K-O-R-A commented Oct 8, 2024

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

@varjolintu
Copy link
Member

Uh oh, why Figma wants to contact 127.0.0.1? Seems the thing is not that easy to do with plain Bootstrap :/

@A-K-O-R-A
Copy link
Author

A-K-O-R-A commented Oct 8, 2024

Uh oh, why Figma wants to contact 127.0.0.1?

If figma does not work for you I can send a big screenshot instead if you want, let me know if yes :)

Seems the thing is not that easy to do with plain Bootstrap :/

Like I said you can check out the mock up on my branch, I fixed the weird click behavior and everything seems to work
(The actual features of the settings do not take effect on the mock up branch because I started from a very early commit)

@droidmonkey
Copy link
Member

Figma worked for me

@varjolintu
Copy link
Member

varjolintu commented Oct 8, 2024

That mockup_2 has multiple issues when I tested it:

  • The table cuts the dropdown menu
  • There are three dropdowns instead of one
  • The dropdown position changes when open if you scroll the table etc.

I'll try to provide some help (if you need it) later when I have the time.

@varjolintu
Copy link
Member

Maybe the simplest solution could actually be just a custom <div> that is positioned with the input? The look is not a native one though. And the default selected value must be empty, or we could put a placeholder there.

Screen.Recording.2024-10-08.at.18.31.50.mp4

@A-K-O-R-A
Copy link
Author

  • The table cuts the dropdown menu

  • The dropdown position changes when open if you scroll the table etc.

Could you explain these two points are bit more? I did not yet have time to extensively test it but I didn't notice these issues at all. Thank you for testing so quick btw :)

  • There are three dropdowns instead of one

Sorry my bad, I only remembered 3/3/3 and forgot that it was supposed to be one 😅
Could you try adding 9 options to a drop down and see if that even fits on a 720p screen?

I won't be able to work on this anymore today but will definitely try out some more ideas tomorrow :)

@A-K-O-R-A
Copy link
Author

Maybe the simplest solution could actually be just a custom <div> that is positioned with the input? The look is not a native one though. And the default selected value must be empty, or we could put a placeholder there.

I guess that would be a solution, though it would add more complexity. I don't see why the bootstrap dropdown shouldn't work and I'll try to replicate the problems you mentioned earlier tomorrow. Could you record a video of those things happening on my mock_2 branch? That would be super helpful :)

@varjolintu
Copy link
Member

Maybe the simplest solution could actually be just a custom <div> that is positioned with the input? The look is not a native one though. And the default selected value must be empty, or we could put a placeholder there.

I guess that would be a solution, though it would add more complexity. I don't see why the bootstrap dropdown shouldn't work and I'll try to replicate the problems you mentioned earlier tomorrow. Could you record a video of those things happening on my mock_2 branch? That would be super helpful :)

Here's a video:
https://github.com/user-attachments/assets/50a06b03-fb75-423e-93c9-4d7244c2c511

@A-K-O-R-A
Copy link
Author

Thank you for the video, I did not try it with this few entries. I think adding a min-height: 20rem to the table-responsive div is a very reasonable solution to this problem, I added a commit to the mockup branch doing exactly that. I also implemented the single checkbox with 9 entries. The screenshots are in the figma link I shared and you can try it out yourself on the mockup_2_single_checkbox branch on my fork

@A-K-O-R-A
Copy link
Author

I feel like the single checkbox approach is what you guys have decided on so I created some different variations of this design and labelled them A,B,C and D in the figma board. It would be nice if you could decide on which one you like the most so we can have a final decision on what to implement. If this is not the case and you are not happy with these designs then I am open to new suggestions as always. The missing divider lines in the checkbox menus and missing border lines in the tables are visible in the browser despite not showing up on the screenshots.

My favorite would be C (the icon is just a unicode character), but as this design is not my preferred design anyways I think it makes more sense for you two to decide on which one will be implemented independently of my opinion.

The dropdown position changes when open if you scroll the table etc.

I think vertical position changes will inevitably happen on smaller screens with the dropdown approach no matter what, especially on screensizes where the checkbox menu simply does not fit on the screen having the checkbox move to a fitting location where space gets used optimally is an important feature. If you mean the sudden horizontal jumping movement then I honestly have no clue why that is happening. But in general I think trying to solve these problems in a responsive way is just very hard and at the end huge dropdown checkbox menus are just not a great combination with a horizontally scrollable table on a small screen and I think there is not much we can do about that.

@varjolintu
Copy link
Member

I think "Enable all features" option is no longer needed with the new checkboxes. I'll check the branch when I have the time.

About the horizontal jumping: this is why my mockup is just using the similar kind of element positioning that the extension already does for Autocomplete Menu, icons etc. With position: absolute you don't need to worry about issues like this. If the element is positioned inside some table cell etc. it will definitely cause issues. It must also made sure that the dropdown is positioned at the top side of the field if there's no space at the bottom for displaying it. Our Autocomplete Menu's positioning also handles that.

@varjolintu
Copy link
Member

I checked the branch quickly and found these issues:

  • The page has a vertical scrollbar even if the table is empty
  • The checkboxes and texts are not respecting the default site (e.g. in General tab)
  • The row style has changed? In Figma the buttons were aligned on the right and not the center.

Sometimes the menu fits, sometimes it cuts out:
Screenshot 2024-10-09 at 19 49 01
Screenshot 2024-10-09 at 19 49 08

@A-K-O-R-A
Copy link
Author

A-K-O-R-A commented Oct 9, 2024

The page has a vertical scrollbar even if the table is empty

A side effect of the min-height fix, I do not think this is an issue because the area is only as big as a single checkbox menu, if the area would not be big enough then the checkbox would simply clip at the bottom of the table and be unusable so this is kind of necessary with this checkbox approach. But adding special treatment for the case where the table is fully empty is definitely easy and can be done.

The checkboxes and texts are not respecting the default site (e.g. in General tab)

If you mean default settings then yes? This is still a mock up that does not have any functionality because I do not want to waste time implementing that every time I try out a newly suggested design. The behavior in combination with the feature disable selector was never discussed. I think selecting "Disable Auto-Submit" would not override the site specific enabled auto-submit but I have not tested it.

The row style has changed? In Figma the buttons were aligned on the right and not the center

Yeah I forgot to add that change to the commit, sorry about that, should be fixed now

@A-K-O-R-A
Copy link
Author

I think we should clear up how the check boxes are supposed to work, do you want to remove the previous selector with "Disable all features" and instead just have a list of checkboxes in a similar style to discord? What I mean by that is that you can choose to override the global behavior in both directions, disable and enable but can also keep the global setting, I think discord solved that in a very intuitive way as shown below:

image

@A-K-O-R-A
Copy link
Author

A-K-O-R-A commented Oct 9, 2024

Before deciding on a design I think we should go back to actually defining what settings should be available, what different options per setting should be available and what kind of combinations so that we can actually design a UI around those. Because deciding a UI and then changing the UI elements afterwards to reflect the new options could potentially create new problems. So here are my opinions on this:

🚫 Enable all features

You have previously said that the "Enable all features" option should be removed, which makes sense given all global options can now also be enabled for a single site.

🚫 Disable Auto-Submit

Having a site specific enable and disable checkbox for a single setting does not make sense if we do not explicitly describe the behavior that the user can expect. My suggestion would be to either (A, preferred) remove the invariant and either merge the two checkboxes in a similar style to the picture in my comment above, or (B) add a detailed description before the table explaining which checkbox overrides the other.

I think B is just a confusing and bad user experience but it would fulfill the goal of minimal change that you mentioned in the beginning.

🚫 Disable all features

Keeping this would introduce the same problem I mentioned in the paragraph about the "Disable Auto-Submit" option. I think the default behavior is not clear without reading an explanation text and in my opinion adding one is not a good solution even if it would mean minimal change.

✔️ "Disable new/modified credentials", "Username-only Detection", "Improved Input Field Detection", "Allow Cross-Origin iframes"

I would just keep those settings as they are now

My suggested layout

  • Disable new/modified credentials"
  • Username-only Detection
  • Improved Input Field Detection
  • Allow Cross-Origin iframes

--------------------------------------------------------  

[ X |  /  | ☑️ ]  Auto-submit login forms
[ X |  /  | ✔️ ]  Automatically fill in single-credential entries
[ X |  /  | ✔️ ]  Automatically fill in single TOTP entries

Other UI suggestions for the overriding checkboxes

It would also be possible to just set the Auto-submit checkbox (and the other two) to the same setting as the global one and save the overridden setting once the checkbox gets clicked for the first time. But I think this introduces a very confusing user experience where it is unclear if the setting is actually the global preference or the site specific one. It would also make it impossible to remove a single setting override.

But there are other designs possible besides the discord one, for example the checkbox could show the global setting at first and be grayed out if there is no site specific preference. Then once the user clicks the checkbox won't be grayed out anymore and will reflect the site specific preference. If the user then wants to remove the change there could be a small reset icon which then grays out the checkbox again and reflects the global setting like before.

⟳ ☑️ Auto-submit login forms
     ☑ Automatically fill in single-credential entries
     ☐ Automatically fill in single TOTP entries

Let me know what you think about these suggestions :)
I would stop creating new table mock ups for now until we decided on the exact amount and behavior of the settings

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.

3 participants