1616945 - Don't check field.value !== field.defaultValue for fields which aren't relevant to usernames/passwords

Status: RESOLVED FIXED

(Toolkit :: Password Manager, defect, P2)

Description

[Matthew N. [:MattN]]

In http://mozilla-1388674.dev.zuma-design.com/ the button.value != button.defaultValue and we shouldn't be considering non-login fields anyways (e.g. <select> or checkboxes). IMO we should use the checks we use elsewhere to see if a field is eligible to be username or a password.

Problematic code: https://searchfox.org/mozilla-central/rev/5a10be606f2d76ef22f1f44565749490de991d35/toolkit/components/passwordmgr/LoginManagerChild.jsm#2240-2241

I mentioned this problem in my code review:

what about looking for any edit to text fields in the LoginForm for now (in this bug) and then later improving it to be specific to username/password fields based on bug reports?

Comment 1

[Sam Foster [:sfoster] (he/him)]

Thanks, I was just filing this too but you beat me to it. Yeah it looks like all the forms used to test this use <input type="submit">. I'll fix the check to use hasBeenPasswordType || isUsernameFieldType() and add new tests

Comment 2

[Billy Noah]

Thanks - I'm not that familiar with the code involved here but have a question. In my test case referenced above, even when no fields have been modified, the browser still prompts to save. I understand this may be peripherally related to field type as you mentioned, but more importantly, I'm concerned that this is happening even when there's been no user interaction at all.

Since this bug is based Bug 1388674 / Only prompt to save logins if a field in a login form was modified, shouldn't we at least need to see something modified before the prompt happens, regardless of what the field type is?

Comment 3

[Billy Noah]

To be be clear, if I were to change my test case to a basic username / password login form, I'd still not expect to be prompted to save if nothing has been modified, so I'm not sure that checking field type will necessarily resolve this.

Comment 4

[Matthew N. [:MattN]]

The problem is that <button> is considered modified because it doesn't have a defaultValue property.

Sam, it seems like we should also assert that defaultValue isn't undefined.

Comment 5

[Sam Foster [:sfoster] (he/him)]

Attachment: Bug 1616945 - Exclude fields with no defaultValue when evaluating if a form was modified and needs login capture. r?MattN

Comment 6

[Billy Noah]

(In reply to Matthew N. [:MattN] (PM me if request are blocking you) from comment #4)

The problem is that <button> is considered modified because it doesn't have a defaultValue property.

Sam, it seems like we should also assert that defaultValue isn't undefined.

Thanks for explaining. That makes sense.

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3d7075ed204f
Exclude fields with no defaultValue when evaluating if a form was modified and needs login capture. r=MattN

Comment 8

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/3d7075ed204f

Comment 9

[Billy Noah]

Hello, this is better now but still a problem. I'll refer back to my test case from bug 1388674:
http://mozilla-1388674.dev.zuma-design.com/

Previously if you clicked "Cancel" without changing anything, you'd be prompted to save your password. The recent changes in version 75 have resolved this. But the problem remains that editing any field in the form still results in that prompt.

For instance, change port 587 to port 5871 and click "Cancel". I see two problems with this:

1. Neither the username, now password field has been touched.
2. The form has not been submitted. Why are we asking to save a password for a form when the user has simply navigated away without submitting the form?

This specifically affects my me because anytime I visit my admin panel and change any setting at all I am continually prompted to save my password even when (a) that field hasn't been touched and (b) I haven't submitted the form.

Can someone please open a new bug?