904612 - First version of Firefox Account Sign up/sign in screen on Desktop

Status: VERIFIED FIXED

(Firefox :: Sync, defect)

Description

[Lloyd Hilaiel [:lloyd]]

A sister bug to the android work in issue 899217.

This is a tracking bug.

The mocks we're working off live here: https://wiki.mozilla.org/Identity/UX

Comment 1

[Lloyd Hilaiel [:lloyd]]

Attachment: fxacct.patch

experimental integration of sign-up account flows into firefox - partial.

Comment 2

[Lloyd Hilaiel [:lloyd]]

One possible implementation route is to land set up and sign-in flow screens in HTML.  Attached is a patch that does this on desktop as a proof of concept.

This approach is trivial and with a little bit of work would let us parallelize work on storage and refinement (or re-implementation in XUL) of the front end.

Comment 3

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

Comment on attachment 789601 [details] [diff] [review]
fxacct.patch

>diff --git a/browser/components/about/AboutRedirector.cpp b/browser/components/about/AboutRedirector.cpp

>+  { "accounts", "chrome://browser/content/aboutaccounts/aboutaccounts.xhtml",
>+    nsIAboutModule::ALLOW_SCRIPT },

This makes about:accounts chrome-privileged...

>diff --git a/browser/base/content/aboutaccounts/aboutaccounts.xhtml b/browser/base/content/aboutaccounts/aboutaccounts.xhtml

>+    <iframe id="remote"/>

...which means that this frame defaults to having a chrome docshell - bad thing to load remote content in (that content gets chrome privileges). Need to mark this as mozframetype="content" to avoid that (though it would be nice to avoid having about:accounts be privileged at all - not sure if that's possible).

Aside from that, this looks fine as a skeleton.

Comment 4

[James Bonacci [:jbonacci]]

Any Depends or Blocks bugs in the various categories we want to include here?
Do we have a Meta bug for Desktop?

Comment 5

[vfilippov]

Attachment: fxacct20130915

Update patch to support postMessage

Comment 6

[vfilippov]

Attachment: fxacct20130915-2.diff

Adds postMessaging and mozframetype

Comment 7

[vfilippov]

Attachment: fxacct20130915-3.diff

Updates the server to `accounts.dev.lcip.org`

Comment 8

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 790995 [details] [diff] [review]
fxacct20130915-3.diff

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

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ +11,5 @@
> +
> +let wrapper = {
> +  init: function () {
> +    let iframe = document.getElementById("remote");
> +    iframe.addEventListener("load", wrapper.initRemotePage, false);

We never remove that listener anywhere, can you call iframe.addEventListener("load", this) and then add a handleEvent() method to the wrapper? This should then also call iframe.removeEventListener("load", this);

@@ +13,5 @@
> +  init: function () {
> +    let iframe = document.getElementById("remote");
> +    iframe.addEventListener("load", wrapper.initRemotePage, false);
> +    let uri = this._getAccountsURI();
> +    iframe.src = uri.spec;

I think we can start loading the iframe at the end of the document without waiting for the page to be loaded. That should result in better load times.

@@ +20,5 @@
> +  uninit: function () {
> +  },
> +
> +  onLogin: function (data) {
> +    Cu.reportError("Received: 'login'. Data:" + JSON.stringify(data));

Maybe dump()? Or Services.console.logStringMessage()? Seeing a bunch of error reports might be confusing. Would also be good to define this at the top and name it 'debug()' or the like.

@@ +40,5 @@
> +    return Services.io.newURI(url, null, null);
> +  },
> +
> +  initRemotePage: function () {
> +    let iframe = document.getElementById("remote").contentDocument;

init() should store a reference to the iframe.

@@ +43,5 @@
> +  initRemotePage: function () {
> +    let iframe = document.getElementById("remote").contentDocument;
> +    iframe.addEventListener("FirefoxAccountsCommand",
> +      function onCommand(e) { wrapper.handleRemoteCommand(e); },
> +      false);

Would be great to use handleEvent() here as well.

@@ +49,5 @@
> +
> +  handleRemoteCommand: function (evt) {
> +    switch (evt.detail.command) {
> +      case "create":
> +        this.onCreate(evt.detail.data);

evt.detail.data should be saved in a variable.

@@ +64,5 @@
> +    }
> +  },
> +
> +  injectData: function (type, content) {
> +    let report = this._getReportURI();

Looks like that method is missing?

::: browser/base/content/aboutaccounts/aboutaccounts.xhtml
@@ +1,1 @@
> +<?xml version="1.0" encoding="UTF-8"?>

Nit: can we name this aboutAccounts.xhtml, matching the DTD file? Same for the .css and .js file.

@@ +7,5 @@
> +  %htmlDTD;
> +  <!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd">
> +  %brandDTD;
> +  <!ENTITY % securityPrefsDTD SYSTEM "chrome://browser/locale/preferences/security.dtd">
> +  %securityPrefsDTD;

What's the security prefs DTD for?

@@ +24,5 @@
> +   <script type="text/javascript;version=1.8"
> +     src="chrome://browser/content/aboutaccounts/aboutaccounts.js" />
> +  </head>
> +  <body onload="wrapper.init();"
> +        onunload="wrapper.uninit();">

We try to avoid using attributes for events. Please use addEventListener() in the .js file.

@@ +25,5 @@
> +     src="chrome://browser/content/aboutaccounts/aboutaccounts.js" />
> +  </head>
> +  <body onload="wrapper.init();"
> +        onunload="wrapper.uninit();">
> +    <iframe style="width: 100%; height: 100%" mozframetype="content" id="remote" />

These style rules should be moved to the .css file. You should also give it a border:0, that looks a little nicer.

::: browser/locales/en-US/chrome/browser/aboutAccounts.dtd
@@ +1,5 @@
> +<!-- This Source Code Form is subject to the terms of the Mozilla Public
> +   - License, v. 2.0. If a copy of the MPL was not distributed with this
> +   - file, You can obtain one at http://mozilla.org/MPL/2.0/. -->
> +
> +<!ENTITY aboutaccounts.pagetitle "&brandShortName; Accounts">

Nit: aboutAccounts.pageTitle maybe?

Comment 9

[Zachary Carter [:zaach]]

Attachment: fxacct20130816.patch

Fixed nits and made everything camelCase.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 791479 [details] [diff] [review]
fxacct20130816.patch

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

Apart from a few issues mentioned below this looks good to me. Are we planning on landing this on elm first and extending the wrapper<->iframe API later?

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ +8,5 @@
> +
> +Cu.import("resource://gre/modules/Preferences.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +function log (msg) {

Nit: log(msg)

@@ +19,5 @@
> +  init: function () {
> +    let iframe = document.getElementById("remote");
> +    iframe.addEventListener("load", this);
> +    let uri = this._getAccountsURI();
> +    iframe.src = uri.spec;

So we call Services.io.newURI() below just to retrieve nsIURI.spec again? Couldn't we just omit the newURI() call and pass the value that formatURLPref() returned?

@@ +25,5 @@
> +
> +    window.addEventListener("unload", function unload () {
> +      window.removeEventListener("unload", unload, false);
> +      this.uninit();
> +    }.bind(this), false);

The problem here is that removeEventListener() won't work as 'unload' points the un-binded version of unload(). How about just doing addEventListener("unload", this) and handling the "unload" event in handleEvent() as well?

@@ +29,5 @@
> +    }.bind(this), false);
> +  },
> +
> +  uninit: function () {
> +    this.iframe = null;

this.iframe.contentDocument.removeEventListener("FirefoxAccountsCommand", this);
this.iframe = null;

@@ +37,5 @@
> +    switch (evt.type) {
> +      case "load":
> +        this.initRemotePage();
> +        this.iframe.removeEventListener("load", this);
> +      break;

Nit: please use the same indentation for break as this.iframe...

@@ +40,5 @@
> +        this.iframe.removeEventListener("load", this);
> +      break;
> +      case "FirefoxAccountsCommand":
> +        this.handleRemoteCommand(evt);
> +      break;

Nit: please use the same indentation for break as this.handleRemoteCommand...

@@ +66,5 @@
> +  },
> +
> +  initRemotePage: function () {
> +    let doc = this.iframe.contentDocument;
> +    doc.addEventListener("FirefoxAccountsCommand", this, false);

I wonder, shouldn't we add the event listener to iframe.contentWindow? The document goes away when navigating (i.e. logging in, etc.) but contentWindow as the outer window stays the same.

@@ +70,5 @@
> +    doc.addEventListener("FirefoxAccountsCommand", this, false);
> +  },
> +
> +  handleRemoteCommand: function (evt) {
> +    var data = evt.detail.data;

Nit: please use 'let'.

@@ +90,5 @@
> +  },
> +
> +  injectData: function (type, content) {
> +    let auth = this._getAccountsURI();
> +    let authUrl = auth.spec;

Same here. Looks like we just use nsIURI.spec again.

::: browser/base/content/aboutaccounts/aboutaccounts.xhtml
@@ +23,5 @@
> +     src="chrome://browser/content/aboutAccounts/aboutAccounts.js" />
> +  </head>
> +  <body>
> +    <iframe mozframetype="content" id="remote" />
> +    <script>wrapper.init()</script>

You could move the <script> tag loading aboutAccounts.js to here. So you wouldn't have to have that extra tag and could call wrapper.init() in the JS file.

::: browser/base/jar.mn
@@ +53,5 @@
>          content/browser/abouthealthreport/abouthealth.css     (content/abouthealthreport/abouthealth.css)
>  #endif
> +        content/browser/aboutAccounts/aboutAccounts.xhtml   (content/aboutAccounts/aboutAccounts.xhtml)
> +        content/browser/aboutAccounts/aboutAccounts.js      (content/aboutAccounts/aboutAccounts.js)
> +        content/browser/aboutAccounts/aboutAccounts.css     (content/aboutAccounts/aboutAccounts.css)

Hmm... you didn't rename the actual files. I thought that shouldn't work but looks like it does.

::: services/sync/services-sync.js
@@ +71,5 @@
>  pref("services.sync.log.logger.engine.apps", "Debug");
>  pref("services.sync.log.logger.userapi", "Debug");
>  pref("services.sync.log.cryptoDebug", false);
> +
> +pref("firefox.accounts.remoteUrl", "http://accounts.dev.lcip.org/flow");

I'm not sure that's the right place for this pref. I think this is something Firefox-specific that would change for another toolkit-app.

Comment 11

[Zachary Carter [:zaach]]

(In reply to Tim Taubert [:ttaubert] from comment #10)
> Comment on attachment 791479 [details] [diff] [review]
> fxacct20130816.patch
> 
> Review of attachment 791479 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Apart from a few issues mentioned below this looks good to me. Are we
> planning on landing this on elm first and extending the wrapper<->iframe API
> later?

If that works better. The next patch will extend the API and persists the session info/assertions returned from the frame.

> 
> @@ +25,5 @@
> > +
> > +    window.addEventListener("unload", function unload () {
> > +      window.removeEventListener("unload", unload, false);
> > +      this.uninit();
> > +    }.bind(this), false);
> 
> The problem here is that removeEventListener() won't work as 'unload' points
> the un-binded version of unload(). How about just doing
> addEventListener("unload", this) and handling the "unload" event in
> handleEvent() as well?

I was little unsure about having the wrapper handle events from two different windows, but will do.

..snip..

> 
> ::: browser/base/jar.mn
> @@ +53,5 @@
> >          content/browser/abouthealthreport/abouthealth.css     (content/abouthealthreport/abouthealth.css)
> >  #endif
> > +        content/browser/aboutAccounts/aboutAccounts.xhtml   (content/aboutAccounts/aboutAccounts.xhtml)
> > +        content/browser/aboutAccounts/aboutAccounts.js      (content/aboutAccounts/aboutAccounts.js)
> > +        content/browser/aboutAccounts/aboutAccounts.css     (content/aboutAccounts/aboutAccounts.css)
> 
> Hmm... you didn't rename the actual files. I thought that shouldn't work but
> looks like it does.

Oops, those should have been updated too. I'll fix that.

> 
> ::: services/sync/services-sync.js
> @@ +71,5 @@
> >  pref("services.sync.log.logger.engine.apps", "Debug");
> >  pref("services.sync.log.logger.userapi", "Debug");
> >  pref("services.sync.log.cryptoDebug", false);
> > +
> > +pref("firefox.accounts.remoteUrl", "http://accounts.dev.lcip.org/flow");
> 
> I'm not sure that's the right place for this pref. I think this is something
> Firefox-specific that would change for another toolkit-app.

Where should we put it?

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Zachary Carter [:zaach] from comment #11)
> > I'm not sure that's the right place for this pref. I think this is something
> > Firefox-specific that would change for another toolkit-app.
> 
> Where should we put it?

app/profile/firefox.js

Comment 13

[Lloyd Hilaiel [:lloyd]]

Attachment: fxacct20130823.patch

Comment 14

[Lloyd Hilaiel]

NOTE: new development host for sign-up flow is http://idp-mocks.lcip.org/flow (right zaach?)

Comment 15

[Lloyd Hilaiel [:lloyd]]

Attachment: fxacct20130823-1.patch

Comment 16

[Lloyd Hilaiel [:lloyd]]

Comment on attachment 794577 [details] [diff] [review]
fxacct20130823.patch

fxacct20130823-1 changes default url = 'firefox.account.remoteUrl'

Comment 17

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 794584 [details] [diff] [review]
fxacct20130823-1.patch

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

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ +5,5 @@
> +"use strict";
> +
> +const {classes: Cc, interfaces: Ci, utils: Cu} = Components;
> +
> +Cu.import("resource://gre/modules/Preferences.jsm");

We don't use Preferences.jsm, this can be removed.

@@ +9,5 @@
> +Cu.import("resource://gre/modules/Preferences.jsm");
> +Cu.import("resource://gre/modules/Services.jsm");
> +
> +function log(msg) {
> +  dump(msg);

This should be dump(msg + "\n") because that's not done automatically, alas.

@@ +26,5 @@
> +  },
> +
> +  uninit: function () {
> +    this.iframe.contentDocument.removeEventListener("FirefoxAccountsCommand", this);
> +    this.iframe = null;

This is missing:

window.removeEventListener("unload", this);

and

this.iframe.removeEventListener("load", this);

in case we're closed really early. These are no-ops in case there's nothing to remove anymore.

@@ +62,5 @@
> +  },
> +
> +  _getAccountsURI: function () {
> +    return Services.urlFormatter.formatURLPref("firefox.accounts.remoteUrl");
> +

Nit: empty line.

@@ +67,5 @@
> +  },
> +
> +  initRemotePage: function () {
> +    let doc = this.iframe.contentDocument;
> +    doc.addEventListener("FirefoxAccountsCommand", this, false);

This listener should be added to the contentWindow, not the document. The document changes when we navigate to a different page in the iframe (i.e. signing in etc.).

::: browser/base/content/aboutaccounts/aboutaccounts.xhtml
@@ +23,5 @@
> +     src="chrome://browser/content/aboutaccounts/aboutaccounts.js" />
> +  </head>
> +  <body>
> +    <iframe mozframetype="content" id="remote" />
> +    <script>wrapper.init()</script>

We should call wrapper.init() from the JS file and move the script tag loading aboutaccounts.js to this position.

Comment 18

[Lloyd Hilaiel [:lloyd]]

Attachment: fxacct20130823-2.patch

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 794691 [details] [diff] [review]
fxacct20130823-2.patch

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

r=me with the two nits fixed.

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ +40,5 @@
> +      case "FirefoxAccountsCommand":
> +        this.handleRemoteCommand(evt);
> +        break;
> +      case "unload":
> +        window.removeEventListener("unload", this, false);

This line can be removed as we call .uninit() right after that.

@@ +67,5 @@
> +  },
> +
> +  initRemotePage: function () {
> +    let doc = this.iframe.contentWindow;
> +    doc.addEventListener("FirefoxAccountsCommand", this, false);

The variable name is wrong here.

@@ +103,5 @@
> +};
> +
> +window.addEventListener("load", function() {
> +  wrapper.init();
> +});

If we put the <script src=aboutaccounts.js> tag in the XHTML file *after* the iframe, we woudn't even need to wait for the document to be loaded and could start loading the page even earlier. But I'm okay with leaving this as is for now. We can always tweak this later.

Comment 20

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

Comment on attachment 794691 [details] [diff] [review]
fxacct20130823-2.patch

>diff -r 1d6bf2bd4003 -r ce408c45b8b4 browser/base/content/aboutaccounts/aboutaccounts.js

>+function log(msg) {
>+  dump(msg + "\n");
>+};

We shouldn't check in this kind of stdout spam, so just comment this out.

>+let wrapper = {

>+  init: function () {

>+    window.addEventListener("unload", this, false);

Hmm, why is this needed? There is no need to explicitly tear down this stuff when the window's going away, AFAIK.

>+      case "load":
>+        this.initRemotePage();
>+        this.iframe.removeEventListener("load", this);

initRemotePage seems like a bit of a confusing name. But stepping back a little, why is this tied to the first page load only? Don't we want to capture these messages even after the content frame is navigated (either by itself or by the user)? Seems like we should make this a listener on the chrome iframe.

>+  initRemotePage: function () {
>+    let doc = this.iframe.contentWindow;
>+    doc.addEventListener("FirefoxAccountsCommand", this, false);

Don't you need to pass "true" as the magical fourth "receive untrusted events" flag? Hmm, this is probably working because the event listener is on the content window, rather than the chrome window, but that will change if you address my comment above.

Comment 21

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

One other thing: we should have some test coverage for this. browser/base/content/test/browser_aboutHealthReport.js is an example that we could crib off of.

Comment 22

[James Bonacci [:jbonacci]]

:lloyd
This is old: http://idp-mocks.lcip.org/flow
REF: https://bugzilla.mozilla.org/show_bug.cgi?id=904612#c14

This is what you want: http://accounts.dev.lcip.org/flow

Comment 23

[Zachary Carter [:zaach]]

Attachment: fxacct20120826.patch

New patch, with tests per Gavin's suggestion. I've reverted back to using the iframe's contentDocument to attach the command listener to. The account screens are all in a "single page web app" so we shouldn't have to worry about navigation, and the other suggested methods of attaching the listener were not working for me. We can revisit if it does become an issue (although it's also how about:healthreport attaches their command listener).

Tim or Gavin, can we land this? If there are more nits, Lloyd, feel free to fix and land if it's before I wake up tomorrow.

Comment 24

[Tim Taubert [:ttaubert] (inactive)]

(In reply to Zachary Carter [:zaach] from comment #23)
> I've reverted back to using
> the iframe's contentDocument to attach the command listener to. The account
> screens are all in a "single page web app" so we shouldn't have to worry
> about navigation

Ah okay, I didn't know that. Should we worry about users using the context menu to reload the iframe? The scenario of users choosing "Show only this frame" could probably be handled by an error message on the server side if window.top == window.

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 795805 [details] [diff] [review]
fxacct20120826.patch

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

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ +28,5 @@
> +    this.iframe.contentDocument.removeEventListener("FirefoxAccountsCommand", this);
> +    this.iframe.removeEventListener("load", this);
> +    this.iframe = null;
> +    window.removeEventListener("unload", this);
> +  },

As per Gavin's comment, we can definitely remove the whole uninit() method. This all is torn down automatically when the page is closed.

@@ +66,5 @@
> +  },
> +
> +  initRemotePage: function () {
> +    let doc = this.iframe.contentDocument;
> +    doc.addEventListener("FirefoxAccountsCommand", this, false);

I couldn't get this to work with Gavin's suggestion of adding the listener to the iframe, not in capturing mode, not with requesting untrusted events. I think we should at least add the listener to the iframe's contentWindow. This will always be the same outer window and not go away on reload or navigation.

this.iframe.contentWindow.addEventListener("FirefoxAccountsCommand", this);

We can add the listener in the handleEvent() function. That way we don't need to come up with a better name for initRemotePage(), a function with a single line.

::: browser/base/content/test/browser_aboutAccounts.js
@@ +2,5 @@
> + * http://creativecommons.org/publicdomain/zero/1.0/
> + */
> +
> +XPCOMUtils.defineLazyModuleGetter(this, "Promise",
> +  "resource://gre/modules/commonjs/sdk/core/promise.js");

Let's use Promise.jsm.

@@ +10,5 @@
> +registerCleanupFunction(function() {
> +  // Ensure we don't pollute prefs for next tests.
> +  try {
> +    Services.prefs.clearUserPref("firefox.accounts.remoteUrl");
> +  } catch (ex) {}

clearUserPref() doesn't throw anymore.

Comment 26

[Zachary Carter [:zaach]]

Attachment: fxacct20120827.patch

Thanks guys! I like removing code.

Comment 27

[Zachary Carter [:zaach]]

https://hg.mozilla.org/projects/elm/rev/571b2854e11f

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

Backed out:

https://hg.mozilla.org/projects/elm/rev/a4d249ce59ac

Relanded with proper description and bug number:

https://hg.mozilla.org/projects/elm/rev/ff718cdbd54b


If you're landing patches, please make sure they always have a description that says: "Bug 123456 - Description of what the patch does; r=gavin,ttaubert".

Comment 29

[James Bonacci [:jbonacci]]

I *think* QA already tested this.
When did this "officially" appear in ELM nightly?

Comment 30

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

You can see Nightly builds produced on https://tbpl.mozilla.org/?tree=Elm (they show up as "N" in the rightmost list of builds). AFAIK they're on the same scheduler as other nightlies, so they're produced daily around 4AM pacific. So typically a fix will be in an Elm nightly no more than 24hrs after it lands.

about:buildconfig will give you a build's changeset, you can look it up on https://hg.mozilla.org/projects/elm or use |hg debugancestor| to find out if it contains a specific changeset.

Comment 31

[Tim Taubert [:ttaubert] (inactive)]

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

Comment 32

[James Bonacci [:jbonacci]]

Cleaning up Resolved/Fixed bugs from December's first release.
Verified that we now have a working first-release of FxA to Desktop/Android Nightly.
Re-open as needed.