764213 - Implement provisional Desktop UI for website sign-in with Persona

Status: RESOLVED FIXED

(Firefox :: General, enhancement)

Description

[Matthew N. [:MattN]]

Native support for BrowserID/Persona for signing into websites.

Comment 1

[Mardeg]

Please let this implementation access bookmarklets for initial on-the-fly password generation *and* generation for when asked to "Verify".

Comment 2

[Matthew N. [:MattN]]

Attachment: WIP v.1 Doorhanger + popup window for auth. (needs tests)

This needs tests and moving some functions in urlbarbindings to methods but otherwise it's ready for feedback.

(In reply to Mardeg from comment #1)
> Please let this implementation access bookmarklets for initial on-the-fly
> password generation *and* generation for when asked to "Verify".

This patch currently uses a popup window for the authentication flow so bookmarklets and thing like form and password manager just work.

Comment 3

[Jason Smith [:jsmith]]

Why is the nominated for k9o? I thought sign into browser was cut from k9o or am I mistaken (or has it changed?)?

Comment 4

[Matthew N. [:MattN]]

(In reply to Jason Smith [:jsmith] from comment #3)
> Why is the nominated for k9o? I thought sign into browser was cut from k9o
> or am I mistaken (or has it changed?)?

Because at the time that I set the flag, it was not deferred on the product draft: 
"P2: After sign in to the browser, sign in to (browserID supporting) websites does not require additional sign-in.".  I realize it said "After sign in to the browser" but that part was deferred.  As of this morning this is now marked as deferred but I'm not sure if that was intentional or not.

Note that this bug is not for sign into the browser, that is bug 749070.  This is for the ability to natively sign into a website using BrowserID.

Comment 5

[Ben Adida [:benadida]]

this is a P1 for B2G/basecamp, and so it seems like a P1 for k9o.

Comment 6

[Asa Dotzler [:asa]]

This is not necessary for Desktop Kilimanjaro. It was a P2 which means "nice to have" but not "blocker". It's still nice to have, so if dev and review time isn't blocking other P1s, then I'm happy to see this work continue. If, on the other hand, there are blockers that would be delayed by continued effort here, we should put the priority on those blockers.

B2G probably has a hard requirement on something like this, but they should have their own bug (do they?).

Comment 7

[Justin Dolske [:Dolske]]

Comment on attachment 632488 [details] [diff] [review]
WIP v.1 Doorhanger + popup window for auth. (needs tests)

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

I'm marking f- just because there's more to do, basically looks good though!

I didn't look too closely at the standard doorhanger setup/calling stuff. Figured I'll just test that manually in a final review.

Speaking of... Needs tests. But you knew that! ;-)

Also cleared the feedback request for gavin, feel free to flag him again if there was specific doorhanger stuff you wanted his input on.

::: mozilla-central/browser/base/content/urlbarBindings.xml
@@ +1288,5 @@
> +          this.step = this.step;
> +        }
> +
> +        // Fire notification with the chosen identity when main button is clicked
> +        this.button.addEventListener("command", function(evt) {

There's quite a bit of code in this anonymous function callback... Can you make this a local |function| or |<method>|?

@@ +1322,5 @@
> +        this.addEmailLink.value = gNavigatorBundle.getString("identity.newIdentity.label");
> +        this.addEmailLink.accessKey = gNavigatorBundle.getString("identity.newIdentity.accessKey");
> +        this.addEmailLink.addEventListener("click", function addEmailClick(evt) {
> +          this.step = 0;
> +        }.bind(this));

A closure would make these much less verbose...

  let self = this;
  addEL("click", function(){ self.step = 0 })

@@ +1382,5 @@
> +          return this.identity.step;
> +        </getter>
> +        <setter><![CDATA[
> +          let deck = document.getAnonymousElementByAttribute(this, "anonid", "identity-deck");
> +          let paneIndex = val % deck.children.length;

Why the modulo?

::: mozilla-central/browser/components/nsBrowserGlue.js
@@ +32,5 @@
>                                    "resource:///modules/PageThumbs.jsm");
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "SignInToWebsiteUX",
> +                                  "resource:///modules/SignInToWebsite.jsm");
> +

Random grumble: these don't actually load very lazily, since we immediately load them to init. :( Widespread problem, don't need to start fixing it here.

::: mozilla-central2/browser/modules/SignInToWebsite.jsm
@@ +15,5 @@
> +Cu.import("resource://gre/modules/identity/Identity.jsm");
> +
> +function log(msg) {
> +  dump("SignInToWebsiteUX: " + msg + "\n");
> +}

Copy the log() function from Identity.jsm so we're all using the same log throughout.

I like that it uses arguments, which has the benefit of not generating string garbage that's immediately unreff'd when debug == false.

@@ +52,5 @@
> +          this._showLoggedInUI(aData, aSubject);
> +        } else {
> +          this._removeLoggedInUI(aSubject);
> +        }
> +        break;

Add |default| case w/error logging

@@ +60,5 @@
> +  /**
> +   * The website is requesting login so the user must choose an identity to use.
> +   */
> +  requestLogin: function SignInToWebsiteUX_requestLogin(aOptions) {
> +    let windowID = aOptions.QueryInterface(Ci.nsIPropertyBag).getProperty("rpId");

Move the QI into observe(), so that it's ready-to-use as an arg here

@@ +62,5 @@
> +   */
> +  requestLogin: function SignInToWebsiteUX_requestLogin(aOptions) {
> +    let windowID = aOptions.QueryInterface(Ci.nsIPropertyBag).getProperty("rpId");
> +    log("requestLogin for " + windowID);
> +    let [win, browserEl] = this._getUIForID(windowID);

Suggest calling this _getUIForWindowID(), since "ID" can imply something else in identity-land. ;-)

s/browserEl/browser/?  Eh, don't care.

@@ +70,5 @@
> +    let mainAction = {
> +      label: win.gNavigatorBundle.getString("identity.next.label"),
> +      accessKey: win.gNavigatorBundle.getString("identity.next.accessKey"),
> +      callback: function() {
> +        let requestNot = win.PopupNotifications.getNotification("identity-request", browserEl);

I wish we chose a name other than "Notification" because it's (1) long and (2) abbreviates poorly. sigh.

@@ +90,5 @@
> +    let requestOptions = aOptions.QueryInterface(Ci.nsIPropertyBag).enumerator;
> +    while (requestOptions.hasMoreElements()) {
> +      let opt = requestOptions.getNext().QueryInterface(Ci.nsIProperty);
> +      options.identity[opt.name] = opt.value;
> +    }

Oh, xpcom we love you. Hmm, actually...

A brief consult with mrbkap suggests that we can actually just pass in / receive a JS object through the nsISupports param and it should magically work. And now I remember having the same discussion with him a year or two ago for the prompt code rewrite...

EG, the following is legit:

  Services.obs.notifyObservers({foo: "bar"}, "bacon", null);
  ...
  function observe(subject, topic, data) {
    if (topic == "bacon)
      assert(subject.foo == "bar")
  }

So I think we can ditch the propertybag stuff entirely. Obviously this will need to coordinate with Identity.jsm...

@@ +127,5 @@
> +
> +  /**
> +   * Return the window and <browser> for the given outer window ID.
> +   */
> +  _getUIForID: function(aWindowID) {

Ponder: |window| is such an overloaded term... Maybe this should just be a simple getBrowserForWindowID(), and callers can do the |let win = browser.ownerDocument.defaultView| themselves. Makes it clear in the caller exactly what |window| we're talking about.

Alternatively, [chromeWin, browser] = getUI() would make it clearer.

@@ +140,5 @@
> +                           .getInterface(Ci.nsIWebNavigation)
> +                           .QueryInterface(Ci.nsIDocShell).chromeEventHandler;
> +      let win = browser.ownerDocument.defaultView;
> +      return [win, browser];
> +    } else {

Don't need and else-after-return.

::: mozilla-central/browser/themes/gnomestripe/browser.css
@@ +1244,5 @@
>    list-style-image: url(chrome://global/skin/icons/information-16.png);
>  }
>  
> +#identity-notification-icon {
> +  list-style-image: url(chrome://mozapps/skin/profile/profileicon.png);

Have you checked with shorlander about exactly what icon we want to use here? Might we worth a followup bug.

I'd also suggest, in the interm, making a copy of this icon and giving it a name more specific to this use.

Comment 8

[:Gavin Sharp [email: [e-mail]]]

(In reply to Justin Dolske [:Dolske] from comment #7)
> > +XPCOMUtils.defineLazyModuleGetter(this, "SignInToWebsiteUX",
> > +                                  "resource:///modules/SignInToWebsite.jsm");
> > +
> 
> Random grumble: these don't actually load very lazily, since we immediately
> load them to init. :( Widespread problem, don't need to start fixing it here.

Any reason not to just import() directly, rather than adding a lazy getter that will be immediately triggered?

Comment 9

[Matthew N. [:MattN]]

Comment on attachment 632488 [details] [diff] [review]
WIP v.1 Doorhanger + popup window for auth. (needs tests)

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

::: mozilla-central/browser/themes/gnomestripe/browser.css
@@ +1244,5 @@
>    list-style-image: url(chrome://global/skin/icons/information-16.png);
>  }
>  
> +#identity-notification-icon {
> +  list-style-image: url(chrome://mozapps/skin/profile/profileicon.png);

Sync is also using it and it's just temporary so I'd rather leave it for now.

Comment 10

[Matthew N. [:MattN]]

Attachment: v.1 Provisional UI - Addressing feedback + browser-chrome test

Comment 11

[Axel Hecht [:Pike]]

Comment on attachment 644221 [details] [diff] [review]
v.1 Provisional UI - Addressing feedback + browser-chrome test

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

::: browser/locales/en-US/chrome/browser/browser.properties
@@ +380,5 @@
> +identity.chooseIdentity.label = Use an existing email
> +identity.newIdentity.label = Use a different email
> +identity.newIdentity.accessKey = e
> +identity.newIdentity.email.placeholder = Email
> +# LOCALIZATION NOTE (identity.newIdentity.description, identity.chooseIdentity.description): %S is the website origin (ie. https://www.mozilla.org) shown in popup notifications.

Copy this localization note instead? They're kind-of far from each other, better be on the safe side. Also, if it was just one shared note, it should be on the first one in the file.

Comment 12

[Justin Dolske [:Dolske]]

Comment on attachment 644221 [details] [diff] [review]
v.1 Provisional UI - Addressing feedback + browser-chrome test

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

Sorry for the delay in getting to this. Looks fine.

::: browser/base/content/urlbarBindings.xml
@@ +1181,5 @@
> +      <xul:image class="popup-notification-icon"
> +                 xbl:inherits="popupid,src=icon"/>
> +
> +      <xul:vbox flex="1">
> +        <xul:vbox anonid="identity-deck">

Why not an actual <deck>? (i think you explained this once but I've forgotten :)

@@ +1201,5 @@
> +          <xul:label anonid="tos" class="text-link" hidden="true"/>
> +          <xul:label anonid="privacypolicy" class="text-link" hidden="true"/>
> +          <xul:spacer flex="1"/>
> +          <xul:image anonid="throbber" src="chrome://browser/skin/tabbrowser/loading.png"
> +                     style="visibility:hidden" width="16" height="16"/>

I think you can just use hidden=true/false. (Ends up doing the same thing)

::: browser/modules/SignInToWebsite.jsm
@@ +19,5 @@
> +XPCOMUtils.defineLazyModuleGetter(this, "Logger",
> +                                  "resource://gre/modules/identity/LogUtils.jsm");
> +
> +function log(...aMessageArgs) {
> +  Logger.log.apply(Logger, ["SignInToWebsiteUX"].concat(aMessageArgs));

Wish we didn't do this, because it's generating garbage even when debugging is disabled. :(

@@ +39,5 @@
> +    Services.obs.removeObserver(this, "identity-login-state-changed");
> +  },
> +
> +  observe: function SignInToWebsiteUX_observe(aSubject, aTopic, aData) {
> +    log("observe: received", aTopic, "with", aData, "for", aSubject);

You always unwrap aSubject, just add a "if (aSubject) var options = aSubject.wrappedJSObject;" and use |options|. Adding a "var emailAddress = aData" might help with readability too.

Maybe consider (for the future) just putting |aData| into the options as a property?

Comment 13

[Matthew N. [:MattN]]

(In reply to Justin Dolske [:Dolske] from comment #12)
> ::: browser/base/content/urlbarBindings.xml
> > +      <xul:vbox flex="1">
> > +        <xul:vbox anonid="identity-deck">
> 
> Why not an actual <deck>? (i think you explained this once but I've
> forgotten :)

Because a deck resizes to the size of its children and therefore the add an email pane would have a lot of empty space when there are more than a few emails in the identity list (the other pane).
 
> @@ +1201,5 @@
> > +          <xul:image anonid="throbber" src="chrome://browser/skin/tabbrowser/loading.png"
> > +                     style="visibility:hidden" width="16" height="16"/>
> 
> I think you can just use hidden=true/false. (Ends up doing the same thing)

hidden=true sets display:none but I don't want the contents to shift when the throbber appears.
https://mxr.mozilla.org/mozilla-central/source/toolkit/content/xul.css?rev=1fc8bc11a0cd&mark=41#40

> ::: browser/modules/SignInToWebsite.jsm
> > +function log(...aMessageArgs) {
> > +  Logger.log.apply(Logger, ["SignInToWebsiteUX"].concat(aMessageArgs));
> 
> Wish we didn't do this, because it's generating garbage even when debugging
> is disabled. :(

Good point.  I forgot about that when I made the logging functions work again in bug 782578.

https://hg.mozilla.org/integration/mozilla-inbound/rev/59707ed19e48

Try results: https://tbpl.mozilla.org/?tree=Try&rev=8fd6ae8cd858 (and also pine)

Comment 14

[Matthew N. [:MattN]]

A few caveats about this feature:
* this is provisional UI which is off by default. UX is working on the next version. 
* The identity module currently only works with identities from an identity provider (IDP) that implements BrowserID.
* No identity state is persisted between restarts of the application.
* There is currently no fallback to persona.org (bug 772657)
* Not all features of the shim are implemented natively yet and implementations may differ. (ie. bug 769850, bug 769861)

Bugs on the implementation of the BrowserID spec can be filed in Core::Identity. See open bugs at https://bugzil.la/prod:Core%20comp:Identity

Comment 15

[:Gavin Sharp [email: [e-mail]]]

(In reply to Matthew N. [:MattN] from comment #13)
> hidden=true sets display:none but I don't want the contents to shift when
> the throbber appears.

You should use collaped="true", and/or .collaped = true, then.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/59707ed19e48

Comment 17

[:Gavin Sharp [email: [e-mail]]]

(In reply to :Gavin Sharp (use [e-mail] for email) from comment #15)
> You should use collaped="true", and/or .collaped = true, then.

Nevermind, I misread what this comment was about (collapsed true also impacts height/width).

Comment 18

[Alexander L. Slovesnik]

> # LOCALIZATION NOTE (identity.loggedIn.description): %S is the website origin (ie. https://www.mozilla.org)
> identity.loggedIn.description = Signed in as: %S

It doesn't look right. "Signed in as: https://www.mozilla.org" doesn't make sense to me. Perhaps it should be "Signed in at: %S"?

Comment 19

[Matthew N. [:MattN]]

(In reply to Alexander L. Slovesnik from comment #18)
> > # LOCALIZATION NOTE (identity.loggedIn.description): %S is the website origin (ie. https://www.mozilla.org)
> > identity.loggedIn.description = Signed in as: %S
> 
> It doesn't look right. "Signed in as: https://www.mozilla.org" doesn't make
> sense to me. Perhaps it should be "Signed in at: %S"?

Sorry about that. Fixed in https://hg.mozilla.org/integration/mozilla-inbound/rev/91a3c2948a44

Comment 20

[Mounir Lamouri (:mounir)]

https://hg.mozilla.org/mozilla-central/rev/91a3c2948a44