959844 - properly message user when FxA sign in cannot be loaded from the Web (e.g., offline, captive portal, etc.)

Status: RESOLVED FIXED

(Firefox :: Sync, defect, P1)

Description

[Edwin Wong [:edwong]]

Desktop Firefox loads firefox accounts sign in form via https.  If it's unreachable (offline or service down) a page or message should inform the user.

Comment 1

[Edwin Wong [:edwong]]

moving to firefox:fxa group

Comment 2

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

Not going to address this in 29 (we have no strings for it). The normal error pages should appear, which I think is fine.

Comment 3

[Chris Karlof [:ckarlof]]

We need some UX around what we should show when we can load the FxA login page.

Comment 4

[Edouard Oger [:eoger]]

Attachment: connectivity-problems-detection.patch

I like how Chromium does the connectivity problems detection:
https://www.chromium.org/chromium-os/chromiumos-design-docs/network-portal-detection

I implemented it with their URL and it works pretty well, so we might want do create our own 204-test-page too. What do you think about this approach guys?

Comment 5

[Edouard Oger [:eoger]]

Okay I just talked to :markh on IRC and he told me the detection is actually implemented in toolkit/components/captivedetect/captivedetect.js

I will use this in the next patch!

Comment 6

[Edouard Oger [:eoger]]

So I was asking myself do we still want this? This sounds like a duplicate of 562917 (which handles the problem browser-wide). Do we need something specific for sync?

Comment 7

[Ryan Feeley [:rfeeley]]

Proposed using the basic Firefox Accounts template:
h1. No connection
h2. You must be connected to the internet to sign in.
button. Try again

Comment 8

[Edouard Oger [:eoger]]

Proposed solution for the detection: use the nsIHttpChannel attached to the iframe and check response status code.

Comment 9

[Edouard Oger [:eoger]]

Attachment: bug-959844.patch

As expected by markh, the testing was the most difficult part of the work.

Comment 10

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8641363 [details] [diff] [review]
bug-959844.patch

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

The general approach is fine, but I'd prefer to avoid moving to a xul:iframe and the polling.

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ +113,5 @@
>      Utils.ensureMPUnlocked();
>  
>      let iframe = document.getElementById("remote");
>      this.iframe = iframe;
> +    this.iframe.docShell.QueryInterface(Ci.nsIWebProgress);

You should be able to get the docshell via this.iframe.QueryInterface(Ci.nsIFrameLoaderOwner).frameLoader.docShell

Also, I think we should consider making the progress listener its own little object instead of tacking it onto this. The aWebProgress param on the methods should be the docShell of the frame

::: browser/base/content/test/general/browser_aboutAccounts.js
@@ +466,5 @@
>    });
>    return deferred.promise;
>  }
>  
> +// Alternative to promiseNewTabWithIframeLoadEvent

I'd prefer to find a way to avoid the polling here. I think the problem is that content_aboutAccounts is setup to expect the load event once - ie, by the time it has sent "test:iframe:load" once it has removed the event handlers so it will not be sent again.

It might simply be a matter of not removing those event handlers, or an alternative would be to send a new message to content_aboutAccounts that sets up the event handlers ready for a new load. It might be necessary to load the tab explicitly with about:blank first or something, but I'm confident it can be done.

Comment 11

[Nick Alexander :nalexander [he/him]]

eoger: is this still on your list?  mobile/android wants to copy your approach :)

Comment 12

[Edouard Oger [:eoger]]

It is still on my list, as you can see we're still struggling with the tests but we'll be done soon I think. (Unless you're talking about comment 4, which is an abandoned idea).

Comment 13

[Edouard Oger [:eoger]]

Attachment: bug-959844.patch

Here's the last version of the patch. What do we do?

Comment 14

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8645383 [details] [diff] [review]
bug-959844.patch

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

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ -128,5 @@
>    handleEvent: function (evt) {
>      switch (evt.type) {
>        case "load":
>          this.iframe.contentWindow.addEventListener("FirefoxAccountsCommand", this);
> -        this.iframe.removeEventListener("load", this);

why do we remove this? I doubt we want to end up adding multiple listeners for FirefoxAccountsCommand

Comment 15

[Mark Hammond [:markh] [:mhammond]]

Attachment: t.patch

It looks as though we get a DOMContentLoaded event, just no load event when we abort the request. The tests passes for me with this patch.

Comment 16

[Edouard Oger [:eoger]]

Attachment: bug-959844.patch

Rebased and merged with your patch Mark.

Comment 17

[Edouard Oger [:eoger]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a381c0d8e2c

Comment 18

[Edouard Oger [:eoger]]

Try looks ok

Comment 19

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8647249 [details] [diff] [review]
bug-959844.patch

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

r=me if we can reinstate the removal of the load event handler.

::: browser/base/content/aboutaccounts/aboutaccounts.js
@@ +145,5 @@
> +
> +      // Captive portals sometimes redirect users
> +      if ((aState & Ci.nsIWebProgressListener.STATE_REDIRECTING)) {
> +        failure = true;
> +      }

I missed this one :) Please cuddle the else.

@@ -128,5 @@
>    handleEvent: function (evt) {
>      switch (evt.type) {
>        case "load":
>          this.iframe.contentWindow.addEventListener("FirefoxAccountsCommand", this);
> -        this.iframe.removeEventListener("load", this);

I'm not keen on removing this - if you can add it back and it works, then great - but if you can't, I'll need to understand why

Comment 20

[Edouard Oger [:eoger]]

Attachment: bug-959844.patch

Carrying previous r+. Reverted the listener removal too.

Comment 21

[Pulsebot]

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

Comment 22

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/654c94c6320a

Comment 23

[Francesco Lodolo [:flod]]

It's nit, but has anyone thought about "Internet" vs "internet" in the string? The former seems to be more common in Firefox codebase.

Comment 24

[Mark Hammond [:markh] [:mhammond]]

(In reply to Francesco Lodolo [:flod] from comment #23)
> It's nit, but has anyone thought about "Internet" vs "internet" in the
> string? The former seems to be more common in Firefox codebase.

Ryan, thoughts?

Comment 25

[Ryan Feeley [:rfeeley]]

House stytle: Internet — always capitalized.
https://www.mozilla.org/en-US/styleguide/communications/copy-rules/

Comment 26

[Mark Hammond [:markh] [:mhammond]]

(In reply to Francesco Lodolo [:flod] from comment #23)
> It's nit, but has anyone thought about "Internet" vs "internet" in the
> string? The former seems to be more common in Firefox codebase.

Nice catch, thanks. Given how recently this landed, are we able to change this string without changing the ID?

Comment 27

[Francesco Lodolo [:flod]]

(In reply to Mark Hammond [:markh] from comment #26)
> Nice catch, thanks. Given how recently this landed, are we able to change
> this string without changing the ID?

Absolutely yes, it's just a matter of consistency for English, no need for new IDs.

Comment 28

[Mark Hammond [:markh] [:mhammond]]

Thanks - pushed the trivial fix with r=me

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

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/95e78f3ea1f5