-
-
Notifications
You must be signed in to change notification settings - Fork 188
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
base: develop
Are you sure you want to change the base?
Conversation
…setting is present
…e setting is present
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. |
Agreed these checkboxes make this extremely unusable |
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? |
Open to mock ups! |
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. |
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. |
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. |
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):
|
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 |
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 |
} | ||
|
||
#sitePreferencesTable > li > details > summary > div > div:first-child::before { | ||
content: '►'; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Actual code review & testing after we've decided if this is the style we want to have. |
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 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. |
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. |
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 This is my suggestion: #2358 (comment). Does @droidmonkey agree? The whole idea here is to prevent the checbox clutter :) |
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 |
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 |
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 The issue with the combox not opening seems to be the following Replacing the current UPDATE: |
Uh oh, why Figma wants to contact |
If figma does not work for you I can send a big screenshot instead if you want, let me know if yes :)
Like I said you can check out the mock up on my branch, I fixed the weird click behavior and everything seems to work |
Figma worked for me |
That
I'll try to provide some help (if you need it) later when I have the time. |
Maybe the simplest solution could actually be just a custom Screen.Recording.2024-10-08.at.18.31.50.mp4 |
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 :)
Sorry my bad, I only remembered 3/3/3 and forgot that it was supposed to be one 😅 I won't be able to work on this anymore today but will definitely try out some more ideas tomorrow :) |
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 |
Here's a video: |
Thank you for the video, I did not try it with this few entries. I think adding a |
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.
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. |
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 |
I checked the branch quickly and found these issues:
|
A side effect of the
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.
Yeah I forgot to add that change to the commit, sorry about that, should be fixed now |
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: |
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 featuresYou 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-SubmitHaving 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 featuresKeeping 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
-------------------------------------------------------- [ X | / | ☑️ ] Auto-submit login forms Other UI suggestions for the overriding checkboxesIt 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 Let me know what you think about these suggestions :) |
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.
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.