Closed Bug 1185780 Opened 4 years ago Closed 4 years ago

disable save if you delete the entire password in about:logins

Categories

(Firefox for Android :: Logins, Passwords and Form Fill, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 42
Tracking Status
firefox42 --- fixed

People

(Reporter: ally, Assigned: ally)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

No description provided.
As discussed with ally, we'll go ahead and include this into the first mvp for passwords.

I suggest to grey out the button to represent "disable".

Anthony, is that good with you?
Flags: needinfo?(alam)
Assignee: nobody → ally
Sounds reasonable. 

Ally, do you already have a disabled grey in the palette from the Accounts team? I see from my original design files it was spec'd at #C0C9DD but I'm not sure if they changed it.

If not, our "disabled grey" in our palette works (#BFBFBF)
Flags: needinfo?(alam)
Attached patch aboutLoginsDisableSaveOnEmptyPwd (obsolete) — Splinter Review
The default behavior of a disabled button according to our base css is to disappear the button (visibility:none iirc). This is the behavior of the patch above. 

It occurs to me you might prefer the button to remain visible but a gray color to indicate that it can't be pushed. What would you like the disabled update button to look like?
Flags: needinfo?(alam)
Exactly the same, just with the grey provided above :) 

Thanks Ally!
Flags: needinfo?(alam)
Flags: needinfo?(ally)
thanks
Flags: needinfo?(ally)
Attachment #8638241 - Attachment is obsolete: true
Attachment #8638806 - Flags: review?(liuche)
Attachment #8638801 - Flags: review?(alam) → review+
Comment on attachment 8638806 [details] [diff] [review]
aboutLoginsDisableSaveOnEmptyPwd

Review of attachment 8638806 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry this took so long! Thanks ally, this works great.

::: mobile/android/chrome/content/aboutLogins.js
@@ +210,5 @@
> +
> +      if (newPassword === "") {
> +        updateBtn.disabled = true;
> +        updateBtn.classList.add("disabled-btn");
> +      } else if ((newPassword != "") && (updateBtn.disabled === true)) {

For correctness, shouldn't the newPassword comparison use a !== as well?

::: mobile/android/chrome/content/aboutLogins.xhtml
@@ +65,5 @@
>          <input type="password" id="password" name="password" value="password" class="edit-login-input" />
>          <button id="password-btn"></button>
>        </div>
>        <div class="edit-login-div">
> +        <button id="update-btn" class="update-btn">&aboutLogins.update;</button>

Is it good form to have the id and class be the same?

::: mobile/android/themes/core/aboutLogins.css
@@ +107,5 @@
>    border-width: 0px;
>    margin-top: 10px;
>  }
> +.disabled-btn {
> +  background-color: #BFBFBF;

Can you add a comment here, referencing this as disabled_grey from colors.xml in our Java code? You can match what you did above.
Attachment #8638806 - Flags: review?(liuche) → review+
https://hg.mozilla.org/mozilla-central/rev/b4f95c2cc243
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
You need to log in before you can comment on or make changes to this bug.