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

Add feature: highlight the form fields Browserpass will fill #341

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

erayd
Copy link
Collaborator

@erayd erayd commented Jan 31, 2024

Leverages the matching logic used for the icon badge to determine if Browserpass has credentials for the tab, and if so injects the standard script & uses it to highlight the fields that will be filled if filling is triggered.

@maximbaz What do you think - is this something we want? Upside is it's easier to see what Browserpass will do, downside is some websites use the outline styling for their own purposes already.

@erayd erayd self-assigned this Jan 31, 2024
@erayd erayd requested a review from maximbaz January 31, 2024 21:16
@erayd erayd marked this pull request as draft January 31, 2024 21:31
@erayd
Copy link
Collaborator Author

erayd commented Jan 31, 2024

Known issue with it that I'm not yet sure how to solve, is it will highlight other fields on those sites as well, not just the login ones. This is because our selector rules assume that the user has triggered a fill request, and that therefore there is something on the current page that needs to be selected. It doesn't handle the "this isn't actually a login page" case well.

Applying that same logic to non-login pages of a site means that Browserpass gets increasingly more desperate to find something relevant, and may eventually end up highlighting something like a search input.

@maximbaz
Copy link
Member

Interesting! Just a few quick thoughts (I haven't tried yet, so I might say something way off):

  • What about highlighting fields, only if password field was discovered? Should solve the issue with highlighting search bar, at the cost of not highlighting username field when the form only has username field
  • Alternatively, what if we trigger highlight only when popup was opened?
  • Alternatively, what if we have a separate shortcut for it, that you only use when you are in doubt? (I don't like this too much)

@erayd
Copy link
Collaborator Author

erayd commented Feb 1, 2024

...at the cost of not highlighting username field when the form only has username field

I did consider this, but I'm not terribly happy with it due to the resulting inconsistency in behaviour between the highlight logic and the fill logic. I'm of the opinion that this feature is useful if (and only if) we can get it to behave and be consistent with the fill logic; if we can't get that consistency then we shouldn't merge it.

...what if we trigger highlight only when popup was opened?

This would be a good part-solution. It ensures consistency, but at the cost of not having any hinting available for those who fill via the hotkey (which is a very common use-case).

...what if we have a separate shortcut for it, that you only use when you are in doubt? (I don't like this too much)

I don't like this either. With that said, if we were to implement as a "only when the popup is opened" feature, then we can simply rely on that to trigger it (and thereby the CTRL+SHIFT+L hotkey), so I don't think we need an entirely new hotkey for it.

@maximbaz
Copy link
Member

maximbaz commented Feb 2, 2024

Another most-certainly-bad-idea is to make Ctrl+Shift+F on the first keypress just highlight the fields, and on the second keypress fill-in them - but it will mess with folks' muscle memory for sure...

I want to use it for a bit longer, right now I'm a bit torn - on the one hand, I like that it's not very obtrusive, on the other hand it still is a bit distracting (at least in the beginning) - even as I browse on Github, it shows up in different search bars, and my attention gets focused there for a second because I'm not used to seeing it there...

image

image

@maximbaz
Copy link
Member

maximbaz commented Feb 5, 2024

I think realistically we will receive some backlash if we don't do something about those search bar being highlighted, for example in the recent days I had to visit github, google mail and google chat often, and I couldn't really get used to stop paying attention to the highlighted search bar...

@erayd
Copy link
Collaborator Author

erayd commented Feb 5, 2024

I agree. We can't merge it unless we have a solution to that problem.

I'm struggling to think of one though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants