Closed Bug 1126608 Opened 9 years ago Closed 9 years ago

Select login from Site Identity doorhanger

Categories

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

x86
Android
defect

Tracking

(firefox41 fixed, relnote-firefox 41+, fennec41+)

RESOLVED FIXED
Firefox 41
Tracking Status
firefox41 --- fixed
relnote-firefox --- 41+
fennec 41+ ---

People

(Reporter: antlam, Assigned: liuche)

References

Details

Attachments

(10 files, 3 obsolete files)

175.88 KB, image/png
Details
255.43 KB, image/png
Details
116.43 KB, image/png
antlam
: feedback+
Details
300.34 KB, image/png
Details
167.39 KB, image/png
antlam
: feedback+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
Details
39 bytes, text/x-review-board-request
Margaret
: review+
Details
A reliable fall back where auto-fill on any website is unsuccessful. At this point, we still know what site the user is on, and maybe their username. 

Use case:
---

 1) User taps input field to log in
 2) "Quick-view" of about:passwords (filtered by the site) shows up. 
 3) User chooses correct log in that they've actually already saved
 4) "Quick-view" goes away and log in form is automatically submitted.

As discussed, "2" needs to look different from about:passwords because tapping an item will dismiss the list > fill > and trigger the log in form to submit.
Just making a note here from conversations with Mark and Chenxia, this would probably leverage system UI in the beginning not unlike what we currently do for the list of devices when a user taps "Send to device" in the Share overlay work.
"Select another login" would popup an Android dialog in this mock
Attached image prev_pw_fill_select_mock1.png (obsolete) —
Example of Android dialog (simple dialog on L depicted here)
The Prompt.jsm system would support this, if we need to execute from JS. We might be able to create something easily from Java using Prompts too. I'd start with reusing Prompts rather than creating a new UI from scratch, unless we really need to.
(In reply to Mark Finkle (:mfinkle) from comment #5)
> I'd start with reusing Prompts rather than creating a new UI from scratch,
> unless we really need to.

+1 for reusing. Maybe not obvious (heh) but this was supposed to be a quick mock of system default UI (i.e. nothing new hopefully
See Also: → 1122759
Anthony, now that we no longer allow autocomplete=off to block autofilling a username for a website (bug 1025703), if there is a single username for a page it should get autofilled.

So in this case, if the username is not autofilled it's because:
1) There are multiple logins for the url and we didn't know which one to autofill
2) There are no logins for this url

What's the username that's being suggested in case 1 (most recently used? random?) and case 2 (or should we not show anything - "Select login")?

And in the case 2, what should the suggestion list be from, if we don't have any other logins for the site? Mark and I talked about the idea of not bringing up about:passwords, and rather having a filterable list (maybe with a text box) where the user can search all of their passwords from the overlay/prompt page. Alternatively, we can open up about:passwords, but there's a context switch there.
Talked on Vidyo.. 

Main takeaways: 
 - If there are multiple logins: suggest "Use 'lastused@email.com' to login", Select another login, and buttons: Cancel / Use login
 - "Select another login -> Dialog of all "facebook.com" logins, "Show all my logins" expands and shows complete list of logins (does not go to about:passwords). [design to come, missing from the doc I just sent you.]

 - If there are no logins: suggest "Would you like to add a new login for this site?", and buttons: Cancel / Add login
Attached image prev_pw_fill_select_mock2.png (obsolete) —
Flow!
Attachment #8563103 - Attachment is obsolete: true
Since we're viewing these bugs in a cross-platform setting, I've changed the title to make it easier to visually match it with the Desktop bug.
Priority: -- → P1
Summary: Design for better manual fill of login credentials → Mechanism on Firefox for Android to manually search and copy/paste login credentials when the they're not auto-filled
Assignee: nobody → alam
Whiteboard: [blocked]
Summary: Mechanism on Firefox for Android to manually search and copy/paste login credentials when the they're not auto-filled → Overlay to manually search and copy/paste login credentials when the they're not auto-filled
Summary: Overlay to manually search and copy/paste login credentials when the they're not auto-filled → Doorhanger and overlay to manually search and copy/paste login credentials when the they're not auto-filled
Attachment #8566921 - Attachment is obsolete: true
This is currently blocked on the two entry points to this UI:
a) Key icon (or unified doorhanger view from bug 1141904)
b) "Saved logins" option in autocomplete dropdown selections
Note: 

 - this is currently being tracked for 40, but we may need to see if we can get the form autocomplete working across Desktop and Mobile first since that's one of the entry points.
Summary: Doorhanger and overlay to manually search and copy/paste login credentials when the they're not auto-filled → Select login from doorhanger to copy
Going to try out adding the login selection doorhanger to the SiteIdentityPopup, which is summonable.

Cases where the login doorhanger is added are:
- "password" form detected
- Logins includes a saved password for this site
Summary: Select login from doorhanger to copy → Select login from Site Identity doorhanger
Whiteboard: [blocked]
Assignee: alam → liuche
Make the text styling consistent between Site Identity and other doorhangers.

Anthony, any thoughts/corrections? This is a strict updating of text styles and padding, so the layout isn't the "new" site identity layout.

I could make some text/padding change to "Encrypted" if you'd like, though - it seems pretty understated. But if we're going to change it anyways later, it might not matter. (In any case, it looks better than what we have now.)
Attachment #8595668 - Flags: feedback?(alam)
Comment on attachment 8595668 [details]
Screenshot: Part 1 - Making Site Identity popup consistent with other doorhangers

+ for good looking start! couple questions though - will attach my working mock to help.
Attachment #8595668 - Flags: feedback?(alam) → feedback+
Talked with antlam, and we decided on the following:

We display the login doorhanger in the Site Identity Popup when:
- We haven't autofilled a login (implies a single login)
- There's a login that matches the website url (not sure how well this will match)

Display the "Select another login" link if there is more than one login.

We're leaving off the "Show all" logins for now.
QA Contact: ioana.chiorean
Have some WIP commits at https://reviewboard.mozilla.org/r/8071/

Figured out the WebIDE/debugger; problems, and now Gecko is passing the relevant logins from Gecko to SiteIdentityPopup. All that remains is finishing the UI (which should match the "Edit login" UI and shouldn't be too bad).

Though I do realize that as a fallback, maybe we shouldn't rely on detecting a DOMFormHasPassword event, maybe - some websites hide their password fields, though I guess they need to added/converted at some point...

(Note to self, check out bugzilla and adp, and see how this works with those pages with DOMFormHasPassword.)
This works (mostly) for a single login, and copying the password to the doorhanger works.

https://reviewboard.mozilla.org/r/8301/

Still need to do the following:
- The login select doorhanger actually needs to be associated with the tab, because we'll need to update the SiteIdentityPopup when we switch tabs. Currently, switching between tabs doesn't update this doorhanger correctly.
- Add actionText for opening up a listDialog for many logins
Attached file MozReview Request: bz://1126608/liuche (obsolete) —
/r/8301 - Bug 1126608 - Make Site Identity text styling consistent with other doorhangers. r=ally
/r/8303 - Bug 1126608 - Add Doorhanger:Logins message from Gecko. r=ally
/r/8305 - Bug 1126608 - Add "Select" LoginDoorhanger to SiteIdentityPopup. r=ally
/r/8307 - Bug 1126608 - Add buttons. r=ally

Pull down these commits:

hg pull -r e77225cc4c5d0062c6c246846e8cd9f988a6301b https://reviewboard-hg.mozilla.org/gecko/
Anthony, I added two strings that aren't in this spec for toasts. Let me know if you want them changed.

On successful copy of a password:
"Password copied to clipboard"

On the failure to copy:
"Couldn't copy password"
Attachment #8603647 - Flags: feedback?(alam)
Comment on attachment 8603647 [details]
Screenshot: Toast for successful password copy

woo! 

The strings sound good too.
Attachment #8603647 - Flags: feedback?(alam) → feedback+
Comment on attachment 8602871 [details]
MozReview Request: bz://1126608/liuche

/r/8301 - Bug 1126608 - Make Site Identity text styling consistent with other doorhangers. r=ally
/r/8303 - Bug 1126608 - Add Doorhanger:Logins message from Gecko. r=ally
/r/8305 - Bug 1126608 - Add select login doorhanger to SiteIdentityPopup. r=ally
/r/8307 - Bug 1126608 - Add buttons. r=ally
/r/8591 - Bug 1126608 - Add list of logins. r=ally

Pull down these commits:

hg pull -r d18a3118207cc4cacadb11be62b8be949d433053 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8602871 - Flags: review?(ally)
I'm going to add that this block bug 1147064, because makes some button changes.

Both of these bugs should be tracking 41.
Blocks: 1147064
tracking-fennec: --- → ?
Comment on attachment 8602871 [details]
MozReview Request: bz://1126608/liuche

https://reviewboard.mozilla.org/r/8299/#review7303

Ship It!

::: mobile/android/base/resources/layout/site_identity.xml:13
(Diff revision 2)
> -                  android:orientation="horizontal"
> +                  android:padding="@dimen/doorhanger_padding"

Is this reordering of :padding & :orientation intentional? Why is this change neccessary?

::: mobile/android/base/toolbar/SiteIdentityPopup.java:217
(Diff revision 2)
> +

please collapse this to 
// Set options.
final JSONObject options = new JSONObject();
final JSONObject titleObj = siteLogins.getTitle();
options.put("title", titleObj);

to be consistent with // Set buttons. & // Set message.

::: mobile/android/base/toolbar/SiteIdentityPopup.java
(Diff revision 2)
> -

Please put that space back. :)

::: mobile/android/base/widget/LoginDoorHanger.java:101
(Diff revision 2)
> +        // TODO: Should this pass in a data blob too? Select Passwords needs to.

Should I expect to see these two TODOs as part of this bug?

::: mobile/android/chrome/content/browser.js
(Diff revision 2)
> -

Ahem :)

::: mobile/android/chrome/content/browser.js:4444
(Diff revision 2)
> +      var displayHost;

You used 'let' in the change above in browser.js...Why the switch back to var?

::: mobile/android/base/widget/LoginDoorHanger.java:190
(Diff revision 2)
> +                case SELECT:

I would like Margaret to look over this last patch in the series. This is the one I am least comfortable reviewing on my own.
Attachment #8602871 - Flags: review?(ally) → review+
https://reviewboard.mozilla.org/r/8303/#review7307

...grumble, grumble Rb. Doesn't post the comments on the individual reviews when you do the big review.

::: mobile/android/base/toolbar/BrowserToolbar.java:857
(Diff revision 2)
> +        urlDisplayLayout.destroy();

Is this needed only to destroy the site id popup?
Margaret, could you look over https://reviewboard.mozilla.org/r/8591/ , the last patch in the series? I'm afraid I don't feel entirely comfortable with it and would like a second set of eyes. Thank you!
Flags: needinfo?(margaret.leibovic)
https://reviewboard.mozilla.org/r/8299/#review7347

> Please put that space back. :)

I'm going to keep that line deletion there - the double line spaces looks bad, and this change won't mess up any hg annotate notes.

> Is this reordering of :padding & :orientation intentional? Why is this change neccessary?

Whoops, swapped them back.

> You used 'let' in the change above in browser.js...Why the switch back to var?

Copy-pasted this from LoginManagerPrompter, but I might as well update the var => let.

> Ahem :)

Same note as above, newline deletion.

> Should I expect to see these two TODOs as part of this bug?

Whoops, forgot to remove that, done.
Comment on attachment 8602871 [details]
MozReview Request: bz://1126608/liuche

/r/8301 - Bug 1126608 - Make Site Identity text styling consistent with other doorhangers. r=ally
/r/8303 - Bug 1126608 - Add Doorhanger:Logins message from Gecko. r=ally
/r/8305 - Bug 1126608 - Add select login doorhanger to SiteIdentityPopup. r=ally
/r/8307 - Bug 1126608 - Add buttons. r=ally
/r/8591 - Bug 1126608 - Add list of logins. r=ally

Pull down these commits:

hg pull -r 55df0d26f0ec1188daf0a02d2b10b93c42efdec1 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8602871 - Flags: review+ → review?(ally)
Comment on attachment 8602871 [details]
MozReview Request: bz://1126608/liuche

/r/8301 - Bug 1126608 - Make Site Identity text styling consistent with other doorhangers. r=ally
/r/8303 - Bug 1126608 - Add Doorhanger:Logins message from Gecko. r=ally
/r/8305 - Bug 1126608 - Add select login doorhanger to SiteIdentityPopup. r=ally
/r/8307 - Bug 1126608 - Add buttons. r=ally
/r/8591 - Bug 1126608 - Add list of logins. r=ally

Pull down these commits:

hg pull -r 55df0d26f0ec1188daf0a02d2b10b93c42efdec1 https://reviewboard-hg.mozilla.org/gecko/
Attachment #8602871 - Flags: review?(margaret.leibovic)
https://reviewboard.mozilla.org/r/8303/#review7399

I had a look at other js-java message passing code, lot of boilerplate

::: mobile/android/chrome/content/browser.js:4437
(Diff revision 3)
> +   * _getShortDisplayHost

If this was copied over js, why is it not possible re-use the existing function?

::: mobile/android/chrome/content/browser.js:4437
(Diff revision 3)
> +   * _getShortDisplayHost
> ::: mobile/android/chrome/content/browser.js:4437
> (Diff revision 3)
> > +   * _getShortDisplayHost
> 
> If this was copied over js, why is it not possible re-use the existing
> function?
> 
> ::: mobile/android/chrome/content/browser.js:4437
> (Diff revision 3)
> > +   * _getShortDisplayHost

Hm, Margaret, is it good JS practice to make the LoginManagerPrompter._getShortDisplayHost an exported function and call it from browser.js? Or is it better to copypasta it? I guess adding more not-strictly-necessary code to browser.js isn't a good thing.
(In reply to Chenxia Liu [:liuche] from comment #35)
> > ::: mobile/android/chrome/content/browser.js:4437
> > (Diff revision 3)
> > > +   * _getShortDisplayHost
> > 
> > If this was copied over js, why is it not possible re-use the existing
> > function?
> > 
> > ::: mobile/android/chrome/content/browser.js:4437
> > (Diff revision 3)
> > > +   * _getShortDisplayHost
> 
> Hm, Margaret, is it good JS practice to make the
> LoginManagerPrompter._getShortDisplayHost an exported function and call it
> from browser.js? Or is it better to copypasta it? I guess adding more
> not-strictly-necessary code to browser.js isn't a good thing.

I don't think we could do that easily, since LoginManagerPrompter is an xpcom component. If we're really worried about copy/paste we could make some sort of utility module for helper code like this.

Some digging reveals we actually do already have a helper method in browser.js that does something very similar: 
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#7050

So maybe we can just refactor that to use part of it.

Or really, this method isn't really that long, it's just a bunch of lead-up to calling convertToDisplayIDN, so I don't think it would be too bad to duplicate it (if the alternative is loading more scripts).
Flags: needinfo?(margaret.leibovic)
https://reviewboard.mozilla.org/r/8305/#review7431

::: mobile/android/base/toolbar/SiteIdentityPopup.java:136
(Diff revision 3)
> +                    addLoginsToTab(data);

If we're always adding these logins to the tab, it feels like it might be better to have this "Doorhanger:Logins" listener in Tabs.java, where we already have a bunch of other GeckoEventListeners.

I suppose the one complication would be that you would want to add this doorhanger if we receive the message while the site identity popup is already showing, but maybe we could do that with a TabEvent.

Just something to consider, maybe for a follow-up bug.

::: mobile/android/base/toolbar/SiteIdentityPopup.java:152
(Diff revision 3)
> +        Tabs.getInstance().getSelectedTab().setSiteLogins(siteLogins);

We should clear this value on location change, right?
https://reviewboard.mozilla.org/r/8591/#review7435

Looks fine to me.

::: mobile/android/base/widget/LoginDoorHanger.java:190
(Diff revision 2)
> +                        builder.setTitle("Copy password from");

This should be localized.
tracking-fennec: ? → 41+
Comment on attachment 8602871 [details]
MozReview Request: bz://1126608/liuche

r+ on the commit I was responsible for reviewing.
Attachment #8602871 - Flags: review?(margaret.leibovic) → review+
https://reviewboard.mozilla.org/r/8307/#review7457

::: mobile/android/base/toolbar/SiteIdentityPopup.java:172
(Diff revision 3)
> +            @Override

I still don't care for the buttonclick code here, I feel like there should be a cleaner or more elegant way to add the new behavior. That said, I still don't have any better ideas. A descendant class seems like overkill. I'm not sure modifying the constructor to take an onclickhandler would be that different from what you've already got.

Given that we already have a follow up bug for clean up, I think this is fine for getting the feature out the door.
https://reviewboard.mozilla.org/r/8305/#review7403

Looks reasonable enough to my untrained eye, but that is why I've asked Margaret to look this section over.

::: mobile/android/base/toolbar/SiteIdentityPopup.java:54
(Diff revision 3)
> +    private final static String FORMAT_S = "%s";

Why do we need this?
Blocks: 1165030
https://reviewboard.mozilla.org/r/8305/#review7473

> If we're always adding these logins to the tab, it feels like it might be better to have this "Doorhanger:Logins" listener in Tabs.java, where we already have a bunch of other GeckoEventListeners.
> 
> I suppose the one complication would be that you would want to add this doorhanger if we receive the message while the site identity popup is already showing, but maybe we could do that with a TabEvent.
> 
> Just something to consider, maybe for a follow-up bug.

Filed bug 1165030.

> We should clear this value on location change, right?

Hm, yeah, you're right. I think this would make more sense too if we're tracking the Logins in each Tab, because we can just clear it along with site identity, etc, on the onLocationChange.

For now, I'll clear this on Content:LocationChange, and I'l handle it better in bug 1165030.
https://reviewboard.mozilla.org/r/8305/#review7475

> Why do we need this?

You can see that this string is used later in this file. "%s" is a placeholder string that we use in localized strings to indicate that we expect to substitute a string in dynamically. In this case, we want to substitute the username into the string of the doorhanger content. See |addSelectLoginDoorhanger| for usage.
https://reviewboard.mozilla.org/r/8307/#review7477

> I still don't care for the buttonclick code here, I feel like there should be a cleaner or more elegant way to add the new behavior. That said, I still don't have any better ideas. A descendant class seems like overkill. I'm not sure modifying the constructor to take an onclickhandler would be that different from what you've already got.
> 
> Given that we already have a follow up bug for clean up, I think this is fine for getting the feature out the door.

Hm, do you think defining the button listener anonymously in-function is ugly? Your suggestion of defining an  OnButtonClickListener class is actually what we do for ContentNotificationButtonListener, but I didn't do that because nothing else in SiteIdentityPopup uses that OnClickListener (it's very specific to the LoginDoorhanger, as specified by the Popup), unlike ContentNotificationButtonListener, which both TrackingProtection and MixedContent use.

Actually, the LoginDoorhanger constructor *does* take an OnButtonClickListener, that's where this listener is passed in.

There's not much I can do about the action that the listener though - the Clipboard code is kind of messy, but I don't think there's a better way. Added a comment though.
https://reviewboard.mozilla.org/r/8591/#review7479

> This should be localized.

Good call, I realize localized it, but just didn't use the strings.
https://reviewboard.mozilla.org/r/8303/#review7345

> Is this needed only to destroy the site id popup?

Yep, this is patterned after DoorHangerPopup.destroy, which is used to unregister listeners.
re: comment 44
Flags: needinfo?(ally)
(In reply to Chenxia Liu [:liuche] from comment #44)
> https://reviewboard.mozilla.org/r/8307/#review7477
> 
> > I still don't care for the buttonclick code here, I feel like there should be a cleaner or more elegant way to add the new behavior. That said, I still don't have any better ideas. A descendant class seems like overkill. I'm not sure modifying the constructor to take an onclickhandler would be that different from what you've already got.
> > 
> > Given that we already have a follow up bug for clean up, I think this is fine for getting the feature out the door.
> 
> Hm, do you think defining the button listener anonymously in-function is
> ugly? Your suggestion of defining an  OnButtonClickListener class is
> actually what we do for ContentNotificationButtonListener, but I didn't do
> that because nothing else in SiteIdentityPopup uses that OnClickListener
> (it's very specific to the LoginDoorhanger, as specified by the Popup),
> unlike ContentNotificationButtonListener, which both TrackingProtection and
> MixedContent use.

Yup, exactly. But I don't think that's enough of a reason to make you rework the patch.
Flags: needinfo?(ally)
Attachment #8602871 - Flags: review?(ally)
Attachment #8602871 - Attachment is obsolete: true
Attachment #8619231 - Flags: review+
Attachment #8619232 - Flags: review+
Attachment #8619233 - Flags: review+
Attachment #8619234 - Flags: review+
Attachment #8619235 - Flags: review+
Release Note Request (optional, but appreciated)
[Why is this notable]: Password Manager improvement
[Suggested wording]: Select from your saved logins from the site identity popup when logging into a site
[Links (documentation, blog post, etc)]:
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.