Closed Bug 1144385 Opened 9 years ago Closed 9 years ago

Add option to edit login to password doorhanger

Categories

(Firefox for Android Graveyard :: General, defect)

ARM
Android
defect
Not set
normal

Tracking

(firefox40 fixed, fennec40+)

RESOLVED FIXED
Firefox 40
Tracking Status
firefox40 --- fixed
fennec 40+ ---

People

(Reporter: liuche, Assigned: liuche)

References

Details

Attachments

(6 files, 2 obsolete files)

When clicking on "Edit login" (or whatever string we decide) from the passwords doorhanger, display a dialog that is pre-filled with the login information and allows editing. Checkbox for "Show password," and button text is "Cancel" and "Remember".

Clicking "Remember" will close both popup and doorhanger. Clicking "Cancel" will dismiss the dialog and return user to the popup.
Depends on: 1088220
Possible strings we discussed:
Show details
Show login
Login details
Quick mock of what the L dialog might look like.
Attached file MozReview Request: bz://1144385/liuche (obsolete) —
/r/6233 - Bug 1144385 - Strings for editing login in password doorhanger. r=MattN

Pull down this commit:

hg pull review -r 29134d70773797f80b2e75af63659d4f3b803ccc
Attachment #8584888 - Flags: review?(MattN+bmo)
Attached image Screenshot: WIP Edit dialog (obsolete) —
Assignee: nobody → liuche
Attachment #8584888 - Flags: review?(MattN+bmo) → review+
Comment on attachment 8584888 [details]
MozReview Request: bz://1144385/liuche

https://reviewboard.mozilla.org/r/6231/#review5235

Ship It!
Landing the strings, have one more thing to do for dialog, and reviewer is on PTO today. The plan is to get this into 39, landing either 3/30 or 3/31 (likely uplifting) to match Desktop editable-login capability.

https://hg.mozilla.org/integration/fx-team/rev/5ea88a179268
Flags: needinfo?(francesco.lodolo)
Keywords: leave-open
Target Milestone: --- → Firefox 39
It would be nice to know why you're NI someone ;-)

It's OK to pre-land strings, breaking string freeze on Aurora is not. Eventually we need to make sure sheriffs merge this in mozilla-central before merge day. Or are you expecting to land more strings?
Flags: needinfo?(francesco.lodolo)
Sorry about that, flod - I haven't pre-landed strings before, and it wasn't clear to me if I needed to let l10n know. To answer your question, yes, these are all the strings I need in 39 - I won't need to land anymore.
(In reply to Chenxia Liu [:liuche] from comment #9)
> Sorry about that, flod - I haven't pre-landed strings before, and it wasn't
> clear to me if I needed to let l10n know. To answer your question, yes,
> these are all the strings I need in 39 - I won't need to land anymore.

That explains it then :-) 
No need to warn l10n unless you think it will miss merge day.

Pre-landing strings is not great, but at least it avoids breaking string freeze. As a rule of thumb, if you pre-land strings you should give as much context as possible in the localization note, even better with a link to a UI mock-up/reference. In this case the localization notes seem already pretty detailed, so we're good.
Comment on attachment 8584888 [details]
MozReview Request: bz://1144385/liuche

/r/6233 - Bug 1144385 - Add option to edit login to password doorhanger. r=margaret

Pull down this commit:

hg pull review -r 6b2e0f5406b0360ffe8f59788d15faab4e88ebe6
Attachment #8584888 - Flags: review+
WIP - need to update the callbacks and do some button refactor so we can send the new username/password to Gecko.
Comment on attachment 8584888 [details]
MozReview Request: bz://1144385/liuche

/r/6233 - Bug 1144385 - Refactor buttons into DoorHanger. r=margaret
/r/6335 - Bug 1144385 - Add option to edit login to password doorhanger. r=margaret
/r/6337 - Bug 1144385 - Handle edited login. r=margaret

Pull down these commits:

hg pull review -r 289a4c7cef53d504375c09bb4f71430b18f3dafd
Attachment #8584888 - Flags: review?(margaret.leibovic)
The plan is to land this in nightly, let it bake for a couple of days, and then uplift it to aurora-39 (where the strings are already pre-landed).
tracking-fennec: --- → ?
tracking-fennec: ? → 39+
https://reviewboard.mozilla.org/r/6233/#review5437

I'm not sure how I feel about this patch, I need to think about it more. It seems like it just spreads a lot of the button logic out all over the place, which makes things more difficult to follow. Can you explain more about why this refactor is needed for the login doorhanger to work properly?

::: mobile/android/base/widget/DoorhangerConfig.java
(Diff revision 3)
> -    public DoorhangerConfig() {
> +    public DoorhangerConfig(DoorHanger.OnButtonClickListener listener) {

Maybe as a follow-up we should move OnButtonClickListener into DoorhangerConfig instead of DoorHanger.

::: mobile/android/base/widget/DoorHanger.java
(Diff revision 3)
> -    public void addButton(final String text, final String tag, final OnButtonClickListener listener) {
> +    protected abstract Button makeButton(String text, int id, OnButtonClickListener listener);

I'm a bit confused by this - why do we need to pass OnButtonClickListener to a helper method if we already have it in the DoorhangerConfig?

::: mobile/android/base/widget/DoorHanger.java
(Diff revision 3)
> +    protected void addButton(String text, int id, OnButtonClickListener listener) {

Why do we need these separate makeButton and addButton methods?

::: mobile/android/base/widget/DoorHanger.java
(Diff revision 3)
> +                case MIXED_CONTENT:

I suppose these are the only 2 examples of Doorhangers that we create in Java, as opposed to ones that come from JS, so that's why they have their own types? It feels oddly specific to have types for these, when it seems like the only differences are the button click handlers and the text appearence.
https://reviewboard.mozilla.org/r/6233/#review5447

Currently, the single instance of DoorHangerPopup is responsible for handling all clicks to any of the doorhangers. It checks the state of the doorhanger that was clicked and pulls any state (input selections, checkboxes) regardless of whether the doorhanger actually has those elements, and puts all of that into a JSON message to send to a JS callback. It does this by instantiating each DoorHanger and then adding buttons to it.

Technically, the doorhanger popup container should be agnostic to the layout and buttons of the doorhangers it holds; rather, each doorhanger should handle capturing its own state and making whatever JSON response needs to be passed back to Gecko. However, this has worked fine in the past because all the doorhangers are pretty simple and use the same single layout.

For the login doorhanger, one of the UI paths is to edit the login through a Dialog and pass the new username/password back to JS. Since this is a login-specific task that only happens when the user manually edits the login, I moved the JSON message making step into the doorhanger itself; I also moved button creation into the DoorHanger (instead of having DoorHangerPopup being the one to handle making and attaching buttons to each DoorHanger) - both of these make the separation between Doorhanger popup (container) and Doorhanger cleaner.

> I suppose these are the only 2 examples of Doorhangers that we create in Java, as opposed to ones that come from JS, so that's why they have their own types? It feels oddly specific to have types for these, when it seems like the only differences are the button click handlers and the text appearence.

In the old code, we'd check the instance of the doorhanger to a reference held by the SiteIdentityPopup, but since we're decoupling the message-making by each doorhanger from the doorhangerPopup container, we can't do that anymore. Eventually, the mixed content and tracking doorhangers should be their own subclass of DoorHanger because they already use a different layout from the rest of the DefaultDoorHangers, make different formats of JSON messages; eventually, this type can be moved to a boolean or Site Identity Doorhanger enum.

http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/toolbar/SiteIdentityPopup.java#297

> Why do we need these separate makeButton and addButton methods?

makeButton creates a button that handles capturing the state of the doorhanger and creating the JSON message - this will be different for different types of doorhangers because they will have different messages that need to be sent to JS.

addButton actually adds Buttons to the Doorhanger layouts, which is agnostic of what the buttons actually do. Maybe they should be named createButton and addButton?

> I'm a bit confused by this - why do we need to pass OnButtonClickListener to a helper method if we already have it in the DoorhangerConfig?

We don't actually save DoorHangerConfig as a field because it's only used in Doorhanger construction/setup. Now that makeButton is a protected method, I think this makes the signature of makeButton a little clearer. I guess it's a little indirect: setButtons(config) => addButton(..., config.onButtonClickListener) => makeButton(..., listener)

> Maybe as a follow-up we should move OnButtonClickListener into DoorhangerConfig instead of DoorHanger.

I'm okay with either - I feel like Buttons belong to the DoorHanger so the OnButtonClickListener should also belong to the DoorHanger, but you're right that it's only used in DoorHangerConfig. It gets used by LoginDoorhanger later, to trigger sending the JSON message from the "Edit login" dialog.
https://reviewboard.mozilla.org/r/6233/#review5451

> makeButton creates a button that handles capturing the state of the doorhanger and creating the JSON message - this will be different for different types of doorhangers because they will have different messages that need to be sent to JS.
> 
> addButton actually adds Buttons to the Doorhanger layouts, which is agnostic of what the buttons actually do. Maybe they should be named createButton and addButton?

Oh, I guess we could separate those two, and just call them in sequence, rather than in a nested fashion.
https://reviewboard.mozilla.org/r/6233/#review5463

Thanks for the responses to my comments. I applied these changes locally to see if looking at them in my editor would help clarify things, but I still find some of the logic difficult to follow. I think reducing the number of protected methods, or being really explict about why we need protected methods (and what methods we expect to override) could help make this more straightforward.

I don't want to block you from landing this patch, but I would like to see an updated version that addresses some of these comments and at least attempts to make things a bit simpler, but also adds some documentation to explain cases where things aren't simple.

::: mobile/android/base/toolbar/SiteIdentityPopup.java
(Diff revision 4)
> -        config.setType(DoorHanger.Type.SITE);
> +        config.setType(DoorHanger.Type.MIXED_CONTENT);

This seems like another thing that should just go in the constructor, since I don't think a DoohangerConfig should ever change its type.

::: mobile/android/base/toolbar/SiteIdentityPopup.java
(Diff revision 4)
> -            dh.addButton(mContext.getString(R.string.disable_protection), "disable", mButtonClickListener);
> +            config.addButton(mContext.getString(R.string.disable_protection), BUTTON.DISABLE.ordinal());

It's confusing to me that both DoorHanger and DoorhangerConfig have `addButton` methods, but they do different things.

It's not immeidately clear what this BUTTON enum is for and why we're passing these integers in here. Instead of passing these ids as integers, and then comparing them in the click listener, is there any way to just register a listener directly in here that will handle updating the response object? Maybe this is just all my recent JS experience wishing we could pass around functions.

::: mobile/android/base/widget/DoorhangerConfig.java
(Diff revision 4)
>      public void setButtons(JSONArray buttons) {

It's also confusing that there are two different `setButtons` methods.

::: mobile/android/base/widget/DoorHanger.java
(Diff revision 4)
>          return null;

I only see these two methods used in DefaultDoorHanger. We should remove them from DoorHanger and just make them private methods on DefaultDoorHanger

::: mobile/android/base/widget/DoorHanger.java
(Diff revision 4)
> +    protected void addButton(String text, int id, OnButtonClickListener listener) {

We don't override addButton, and it's only used in this class, so it could be private. That might be one way to reduce some confusion between `addButton` and `makeButton`.
https://reviewboard.mozilla.org/r/6335/#review5441

Overall this is looking pretty good. I just have some minor comments to address.

::: mobile/android/base/widget/DoorHanger.java
(Diff revision 1)
> -        mDividerColor = getResources().getColor(R.color.divider_light);
> +        mDividerColor = mResources.getColor(R.color.divider_light);

If we're only using mResources to get mDividerColor, let's not save mResources as a member variable, and instead just use context.getResources() here directly.

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

This could also be simplified to:

let username = aLogin.username ? this._sanitizeUsername(aLogin.username) : "";

::: mobile/android/components/LoginManagerPrompter.js
(Diff revision 1)
> +        actionText.bundle = bundle;

I think it might be more readable to just create the actionText object with these properties directly, e.g.:

let actionText = {
  text: ...,
  type: ...,
  bundle: ...,
};

::: mobile/android/base/widget/LoginDoorHanger.java
(Diff revision 1)
> +    private enum ACTION_TYPE { EDIT };

Is it standard convention to use all caps for enums like this? I thought we normally made enums CamelCase like class names.

::: mobile/android/base/widget/LoginDoorHanger.java
(Diff revision 1)
> +        final ACTION_TYPE type = ACTION_TYPE.valueOf(actionTextObj.getString("type"));

Yeah, all caps for the `type` type here looks a bit odd.

::: mobile/android/base/widget/LoginDoorHanger.java
(Diff revision 1)
> +            }

Disclaimer: I didn't build this patch to test, so I'm assuming this dialog logic all does the right thing :)

::: mobile/android/chrome/content/browser.js
(Diff revision 1)
> +   *                       bundle: <blob-object> }

Nice documentation update.
https://reviewboard.mozilla.org/r/6337/#review5467

::: mobile/android/base/widget/LoginDoorHanger.java
(Diff revision 1)
> +    private int rememberButtonId;

Let's be consistent with the m prefix - either add it to `rememberButtonId` or remove it from `mListener`.

::: mobile/android/base/widget/LoginDoorHanger.java
(Diff revision 1)
> +            mListener = listener;

This looks fishy... you can theorertically add a different listener for every button that's added, but we only hold onto one listener that handles clicks on any buttons.

If we're going to have a single listener for all buttons, I don't think it should be passed into `makeButton`. I think it should be set separately somewhere else.

::: mobile/android/base/widget/LoginDoorHanger.java
(Diff revision 1)
>                          // TODO: Send addLogin message

Where will you be addressing these TODO items?

::: mobile/android/base/widget/LoginDoorHanger.java
(Diff revision 1)
> +        rememberButtonId = id;

Where does this id come from? Can we have a constant we use somewhere instead, similar to the button constants for the tracking/mixed content notifications?

::: mobile/android/base/widget/LoginDoorHanger.java
(Diff revision 1)
> +                            Log.e(LOGTAG, "Error creating doorhanger reply message");

Pass `e` as a third argument here to log the exception.

::: mobile/android/base/widget/LoginDoorHanger.java
(Diff revision 1)
> +                           mListener.onButtonClick(response, LoginDoorHanger.this);

If I'm understanding this correctly, we need to pass around these DoorHanger instances in onButtonClick to properly remove the doorhanger in DoorHangerPopup. I wonder if there's a different way to do that removal to avoid passing around these DoorHanger instances. Perhaps something to investigate in a follow-up, especially since we don't even need this in the OnButtonClickListener implementation in SiteIdentityPopup.
https://reviewboard.mozilla.org/r/6233/#review5495

> It's also confusing that there are two different `setButtons` methods.

The naming of this is to be consistent with the existing method naming of DoorHangers - I would consider changing that naming convention to make it more specific to setThingInLayout, but both methods are setting things in their respective classes.

> This seems like another thing that should just go in the constructor, since I don't think a DoohangerConfig should ever change its type.

This was mostly added to get around parsing types from JSON, but we can change that.

> It's confusing to me that both DoorHanger and DoorhangerConfig have `addButton` methods, but they do different things.
> 
> It's not immeidately clear what this BUTTON enum is for and why we're passing these integers in here. Instead of passing these ids as integers, and then comparing them in the click listener, is there any way to just register a listener directly in here that will handle updating the response object? Maybe this is just all my recent JS experience wishing we could pass around functions.

This should be covered by bug 1149359, where we can handle positive/negative button callbacks from within each DoorHanger subclass itself (because we'll know what JSON message we want to create). We will also be able to get rid of a lot of this button **** and set those directly from each doorhanger (except DefaultDoorhanger, which will have to handle all the "default" cases).
https://reviewboard.mozilla.org/r/6335/#review5483

> Is it standard convention to use all caps for enums like this? I thought we normally made enums CamelCase like class names.

Ah, you're right, fixed.

> If we're only using mResources to get mDividerColor, let's not save mResources as a member variable, and instead just use context.getResources() here directly.

We use mResources in a few places in LoginDoorhanger, but I can make that a LoginDoorhanger-private member field.

> Disclaimer: I didn't build this patch to test, so I'm assuming this dialog logic all does the right thing :)

Yep, the last commit in this series uses this to make this a clickable text link that launches the dialog. So that works.
https://reviewboard.mozilla.org/r/6337/#review5481

> Where will you be addressing these TODO items?

Forgot to publish this - these are extraneous TODO items because this commit does all of that.

> Where does this id come from? Can we have a constant we use somewhere instead, similar to the button constants for the tracking/mixed content notifications?

This is the callback id passed from Gecko, so we have to save it, but I should definitely rename it :P

> This looks fishy... you can theorertically add a different listener for every button that's added, but we only hold onto one listener that handles clicks on any buttons.
> 
> If we're going to have a single listener for all buttons, I don't think it should be passed into `makeButton`. I think it should be set separately somewhere else.

Yeah, you're right, this is really more of a OnButtonClickHandledListener. Fixing this in the first commit and just setting it as member field of DoorHanger, to be called by the subclasses.
https://reviewboard.mozilla.org/r/6233/#review5537

Thanks for being patient with my reviews. r+ with comments addressed.

::: mobile/android/base/DoorHangerPopup.java
(Diff revision 5)
> -        removeDoorHanger(dh);
> +        removeDoorHanger(doorhanger);

I do wish there was a way doorhangers could just remove themselves, but that's out of the scope of this bug.

::: mobile/android/base/toolbar/SiteIdentityPopup.java
(Diff revision 5)
> +    public static enum BUTTON { DISABLE, ENABLE, KEEP_BLOCKING };

I think this enum should be `ButtonType`, similar to my comment about the other enum.

::: mobile/android/base/widget/DefaultDoorHanger.java
(Diff revision 5)
> +                            response.put("contentType", ("tracking"));

Looking at this again now, I wonder if we could move this logic into JS, since we're handling the clicking in JS. I suppose it's just a matter of deciding whether we want some hard-coded doorhanger logic on the Java side (what we currently have now) or on the JS side. We can think about the best way to solve these issues in the follow-up bug for splitting this up.

::: mobile/android/base/DoorHangerPopup.java
(Diff revision 5)
> +        DoorHanger.Type doorhangerType = null;

Instead of passing in null here and then having a null check in the DoorhangerConfig constructor, why not just pass in the default type here? Either that, or create a DoorhagnerConfig constructor that doesn't take a type. Passing around null as an expected parameter seems dangerous, and it would be nice to avoid if possible.
https://reviewboard.mozilla.org/r/6335/#review5541

This is looking good, but I think there are still some unresolved issues here.

::: mobile/android/base/widget/LoginDoorHanger.java
(Diff revision 1)
> +    private enum ACTION_TYPE { EDIT };

Your comment says this is fixed, but I don't see it changed here...

::: mobile/android/base/widget/LoginDoorHanger.java
(Diff revision 1)
> +                Log.e(LOGTAG, "Error getting action text string");

In the case that adding the action text fails, shouldn't we be setting hiding `mLogin`, as we would if there was no action text?

::: mobile/android/base/widget/LoginDoorHanger.java
(Diff revision 1)
>              mLogin.setVisibility(View.GONE);

It's a bit confusing to me that we're hiding this view here, but setting it to visible inside the `addActionText` helper method. Perhpas `addActionText` should just handle it all - showing `mLogin` if we have some valid data to show, or hiding it if we don't (or run into an error).

::: mobile/android/base/widget/LoginDoorHanger.java
(Diff revision 1)
> +                        // TODO: Hide doorhanger

This is where it would be great if doorhangers could just hide themselves :)

::: mobile/android/components/LoginManagerPrompter.js
(Diff revision 1)
> +        actionText.bundle = bundle;

Same thing here.

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

Mozreview says this issue is "resolved", but I don't see the code has changed here.
https://reviewboard.mozilla.org/r/6337/#review5543

r+ with review comment considered.

::: mobile/android/base/widget/LoginDoorHanger.java
(Diff revision 2)
> +                            Log.e(LOGTAG, "Error creating doorhanger reply message");

Is it okay to continue on if there was an error creating the response? I feel like we should avoid sending a broken JSON object over to JS.

::: mobile/android/base/widget/LoginDoorHanger.java
(Diff revision 2)
> +        mCallbackID = id;

This makes me nervous, but I'll trust that you'll resolve this in the follow-up!
https://reviewboard.mozilla.org/r/6335/#review5553

> Your comment says this is fixed, but I don't see it changed here...

Huh, that is really weird. It's fixed in my local repo that I pushed to reviewboard...I'll be more meticulous next time, and verify if it's a bug in reviewboard, or a problem in my understanding of how to push/publish things.

> In the case that adding the action text fails, shouldn't we be setting hiding `mLogin`, as we would if there was no action text?

Good call, I've resolved this in your next comment about addActionText. In the case where we are given a username, we still want to display the text; if there was no username, we'll want to hide the "Edit login" string.

> It's a bit confusing to me that we're hiding this view here, but setting it to visible inside the `addActionText` helper method. Perhpas `addActionText` should just handle it all - showing `mLogin` if we have some valid data to show, or hiding it if we don't (or run into an error).

I moved this into a try/catch loop that fails if there are any problems with the JSON parsing.

> Mozreview says this issue is "resolved", but I don't see the code has changed here.

I wonder if my changes didn't get pushed, or if I need to republish each commit...?

> This is where it would be great if doorhangers could just hide themselves :)

I don't think that's really possible, because the parent DoorHangerPopup inserts each DoorHanger layout into its own layout, and needs to do the removing. I suppose we could set the view itself to GONE, but we still need to let the DoorHangerPopup know that some action was taken, and when it needs to be dismissed.
Comment on attachment 8584888 [details]
MozReview Request: bz://1144385/liuche

/r/6233 - Bug 1144385 - Refactor buttons into DoorHanger. r=margaret
/r/6335 - Bug 1144385 - Add option to edit login to password doorhanger. r=margaret
/r/6337 - Bug 1144385 - Handle edited login. r=margaret
/r/6681 - Bug 1144385 - Add toast on JSON error. r=margaret

Pull down these commits:

hg pull -r 10ed297b8a07fd5eb9b27e88e27acf56cd84c0ac https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8584888 [details]
MozReview Request: bz://1144385/liuche

/r/6233 - Bug 1144385 - Refactor buttons into DoorHanger. r=margaret
/r/6335 - Bug 1144385 - Add option to edit login to password doorhanger. r=margaret
/r/6337 - Bug 1144385 - Handle edited login. r=margaret
/r/6681 - Bug 1144385 - Add toast on JSON error. r=margaret

Pull down these commits:

hg pull -r 10ed297b8a07fd5eb9b27e88e27acf56cd84c0ac https://reviewboard-hg.mozilla.org/gecko/
Comment on attachment 8584888 [details]
MozReview Request: bz://1144385/liuche

https://reviewboard.mozilla.org/r/6231/#review5603

Ship It!
Attachment #8584888 - Flags: review?(margaret.leibovic) → review+
https://reviewboard.mozilla.org/r/6337/#review5555

> Is it okay to continue on if there was an error creating the response? I feel like we should avoid sending a broken JSON object over to JS.

The original code didn't do anything special and just sent the JSON anyways, but I'll clear the response here and display a toast.
A screenshot for a login doorhanger that has no username.
Attachment #8584889 - Attachment is obsolete: true
tracking-fennec: 39+ → ?
Keywords: leave-open
Target Milestone: Firefox 39 → Firefox 40
tracking-fennec: ? → 40+
Attachment #8584888 - Attachment is obsolete: true
Attachment #8619785 - Flags: review+
Attachment #8619786 - Flags: review+
Attachment #8619787 - Flags: review+
Attachment #8619788 - Flags: review+
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.