1167740 - Allow editing login from "Update login" doorhanger

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Chenxia Liu [:liuche]]

We already have this in the "Remember login" doorhanger, but we should add it to the "Update login" doorhanger too.

Comment 1

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1167740 - Allow editing login from "Update login" doorhanger. r=margaret

Bug 1167740 - Allow editing login from "Update login" doorhanger. r=margaret

Comment 2

[:Margaret Leibovic]

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;

Comment 3

[Chenxia Liu [:liuche]]

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... :/

Comment 4

[Chenxia Liu [:liuche]]

https://hg.mozilla.org/integration/fx-team/rev/e4c461d6bea8

Comment 5

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/e4c461d6bea8