Closed Bug 1616945 Opened 1 month ago Closed 1 month ago

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

Categories

(Toolkit :: Password Manager, defect, P2)

Desktop
All
defect

Tracking

()

RESOLVED FIXED
mozilla75
Tracking Status
firefox75 --- fixed

People

(Reporter: MattN, Assigned: sfoster)

References

()

Details

(Whiteboard: [passwords:capture-UI])

Attachments

(1 file)

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?

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

Assignee: nobody → sfoster

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?

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.

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.

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

Pushed by sfoster@mozilla.com:
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
Status: NEW → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla75
You need to log in before you can comment on or make changes to this bug.