Skip to content
This repository has been archived by the owner on May 27, 2019. It is now read-only.

autosubmit default value switched to false #202

Closed
wants to merge 4 commits into from

Conversation

arslanm
Copy link

@arslanm arslanm commented Dec 30, 2017

Users should be able to verify the input before the form's submitted.

@maximbaz
Copy link
Member

I personally have it also set to false, my concern is that this will literally affect everyone who uses the current default of true - one day they wake up and find out that autosubmit is not working anymore.

I'm curious, what makes you want to change the default value instead of just changing the option for yourself?

If you really want to proceed with this, I think we should do the change in two stages:

  1. Release a version that will write the current value in the local storage (so those people who haven't changed the default of true will save true in their local storage).
  2. Release a new version in a week changing the default, which will now affect only new users.

@arslanm
Copy link
Author

arslanm commented Dec 31, 2017

With local storage disabled, every time you launch a new Firefox instance you need to turn it off on the settings page. This patch is applied to function restore_options() which runs on DOMContentLoaded so it shouldn't affect current users who have local storage enabled. First time users and those with no local storage will have autosubmit = false as default.

@maximbaz
Copy link
Member

What about people who have local storage enabled, but like the default value "true" and thus have never even opened the options page? Their local storage is empty, and changing the default value will affect all of them.

That's why I was suggesting the two stage process, where we first ensure to save the current value in local storage, and only as a second step change the default that will only affect new users.

@maximbaz
Copy link
Member

I should also mention that this is not the only piece of code that needs to be changed to adapt the default value, this file is only about the options page. There are also these places, where the default value of true actually makes a difference on the behavior of browserpass:

https://github.com/dannyvankooten/browserpass/blob/76805fa2a131c4443c1208ca40b360896bc4dc43/chrome/background.js#L13

https://github.com/dannyvankooten/browserpass/blob/76805fa2a131c4443c1208ca40b360896bc4dc43/chrome/inject.js#L218-L222

@arslanm
Copy link
Author

arslanm commented Dec 31, 2017

I see what you mean. I was under the impression earlier that the auto submit flag was automatically pushed to local storage somewhere else. In any case I think it's practical to have auto submit flag on the main window rather than on the options page.

Edit: I only tested this on Firefox Dev Edition 58.0b13 but I presume it should work on other Firefox versions as well as on Chrome browsers.

Also I just realized I haven't tested whether the form is actually submitted when AutoSubmit is on; may need to modify background.js as well. I'll take a look at it later.

@maximbaz
Copy link
Member

maximbaz commented Jan 1, 2018

Hey, thanks for looking into this - and happy new year! 🙂

Regarding your last message, I don't really think auto-submit toggle should be moved out to the main window. The main window should contain features that you need to often interact with, and I see the auto-submit toggle as a decision that people make only once, either choose to save time when submitting forms and allow auto-submit, or choose to verify the input and disable auto-submit. Why would you change it more often?

I still think that changing the default value to false for new users makes a lot of sense, everyone I personally know who uses browserpass has disabled the auto-submit, including myself. But again, it should be changed for new users only, not affecting the current users.

@arslanm
Copy link
Author

arslanm commented Jan 1, 2018

Happy new year to you too @maximbaz :)

Before migrating to pass I was a 1Password user, and before that LastPass. Both apps provide an autosubmit flag that can be changed easily. I don't have LastPass any more but here's 1Password's menu -- easily accessible.

Users will probably no reason to change Auto Submit behavior as often as one might assume but I think it is better than keeping an option page with a single parameter to change. I'd rather move the parameter to main window and remove the options page unless more parameters are introduced. Regardless, future parameters may be introduced to the main window under a gear icon. But I don't think an extension as simple as this need more params.

Anyway. With this patch I know what the AutoSubmit flag is set to as soon as I click the extension icon and I can change its value with a single click. Yes it'll stay there every time I click the icon but so is the Search input field, or the copy user/pass icons, both of which I rarely use.

@arslanm
Copy link
Author

arslanm commented Jan 1, 2018

With the last commit auto submit flag works correctly. FYI. I think this is all I can contribute to browserpass for now. Happy new year and cheers!

@maximbaz
Copy link
Member

maximbaz commented Jan 1, 2018

We have some requests that might add more configuration options, such as #34, #77 and #98, so having an options page is good as it simplifies future improvements. Furthermore, one could argue that copy user/pass does not consume extra space in the main window, and these buttons together with the search box can anyway be used often, much more often than changing auto submit flag.

So I think having a separate options section for specific things that you change almost never is helpful for us and helps keeping the main window clean. We can discuss the idea to add gear icon which improves accessibility of the options section, but let's not clutter the main screen with rarely used toggle and let's not remove the framework for adding new options.

Let me know if you are interested in working on bringing the options section to the main window hidden under the gear icon and/or on changing the default value of the toggle, otherwise I'm afraid this PR cannot be accepted as it is.

@maximbaz
Copy link
Member

maximbaz commented Jan 1, 2018

I'll take care of changing the default value to false for new users myself, the first stage will be released soon, and the second stage will be released in a week or two.

So the question remains only about reworking the options section and bringing it into the main window behind the gear icon. If you want to do it, let's discuss how it would look like before writing code, as I'm not entirely convinced that it is worth the effort. Otherwise, let's keep the options as they are today.

@maximbaz
Copy link
Member

Closing due to no activity.

@maximbaz maximbaz closed this Jan 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants