Allow editing login from "Update login" doorhanger

RESOLVED FIXED in Firefox 41

Status

()

Firefox for Android
General
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: liuche, Assigned: liuche)

Tracking

Trunk
Firefox 41
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox41 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

We already have this in the "Remember login" doorhanger, but we should add it to the "Update login" doorhanger too.
Mentor: liuche@mozilla.com
Whiteboard: [good next bug][lang=java]
status-firefox41: affected → ---
Assignee: nobody → liuche
Mentor: liuche@mozilla.com
Whiteboard: [good next bug][lang=java]
Created attachment 8621395 [details]
MozReview Request: Bug 1167740 - Allow editing login from "Update login" doorhanger. r=margaret

Bug 1167740 - Allow editing login from "Update login" doorhanger. r=margaret
Attachment #8621395 - Flags: review?(margaret.leibovic)

Updated

3 years ago
Attachment #8621395 - Flags: review?(margaret.leibovic) → review+

Comment 2

3 years ago
Comment on attachment 8621395 [details]
MozReview Request: Bug 1167740 - Allow editing login from "Update login" doorhanger. r=margaret

https://reviewboard.mozilla.org/r/10975/#review9669

This looks alright to me, but I have some small comments to address, and I'd also like for us to file a follow-up bug to make sure that we're not overriding doorhanger parameters passed from JS.

::: mobile/android/components/LoginManagerPrompter.js:153
(Diff revision 1)
> -    _showLoginNotification : function (aTitle, aBody, aButtons, aActionText) {
> +    _showLoginNotification : function (aTitle, aBody, aButtons, aUsername, aPassword) {

Update the documentation above this.

::: mobile/android/components/LoginManagerPrompter.js:163
(Diff revision 1)
> +                       password: aPassword }

Nit: identation is off.

(Also we should land a patch to make this file 2-space indented :)

::: mobile/android/components/LoginManagerPrompter.js:283
(Diff revision 1)
> -                    self._updateLogin(aOldLogin, aNewPassword);
> +                   let password = aNewPassword;

The indentation in here also looks off.

::: mobile/android/base/widget/LoginDoorHanger.java:164
(Diff revision 1)
> -                    builder.setPositiveButton(R.string.button_remember, new DialogInterface.OnClickListener() {
> +                    builder.setPositiveButton(mButtonConfig.label, new DialogInterface.OnClickListener() {

So is this just fixing an oversight that we weren't using the label that's passed from JS?

Should we also be updating the cancel button label this way? Can we file a bug to audit the way we're creating these doorhanger to make sure we're using the parameters sent from JS? It would be really confusing for someone trying to update this in the future if the strings in JS are actually going unused.

::: mobile/android/components/LoginManagerPrompter.js:286
(Diff revision 1)
> +                    }

I would just do:

let password = response ? response["password"] : aNewPassword;
https://reviewboard.mozilla.org/r/10975/#review9681

The string for the dialog button wasn't supposed to be pulled from JS, so I don't think we need to file a follow-up for it, but if you'd still like me to, I can do so, but someone else should probably do the audit :)

> So is this just fixing an oversight that we weren't using the label that's passed from JS?
> 
> Should we also be updating the cancel button label this way? Can we file a bug to audit the way we're creating these doorhanger to make sure we're using the parameters sent from JS? It would be really confusing for someone trying to update this in the future if the strings in JS are actually going unused.

No, that was initally hard-coded intentionally because that code was only handling a single specific case, but I guess that maybe I could have pulled it from the JS because they happened to be same.

Basically, these positive/negative buttons are for the _dialog_ that gets launched when you click on the blue actionText link (and not for the _doorhanger_ itself). All that dialog code is handled on a case-by-case basis Java-side, because
1) JS shouldn't really care about what the dialog looks like (e.g., "edit this login" needs to display the username and password, and the rest of the styling doesn't matter as far as JS knows), and
2) it would be really ugly to use the JS dialog API to create a custom password-edit dialog in a callback or something.

With this patch, I just reused the code to make the dialog and pulled the positive button string from the JS object.

We should not be pulling the negative string from JS because that's specific to the doorhanger negative action, which is not necessarily the same as the dialog action (in this case, they are different: "Never" vs "Cancel").

I do think that an audit for clarity + adding comments of all this could be helpful, because apparently it's still confusing to everyone who reviews it... :/
https://hg.mozilla.org/integration/fx-team/rev/e4c461d6bea8
https://hg.mozilla.org/mozilla-central/rev/e4c461d6bea8
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox41: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 41

Updated

3 years ago
Blocks: 1175279
You need to log in before you can comment on or make changes to this bug.