Closed
Bug 959844
Opened 10 years ago
Closed 9 years ago
properly message user when FxA sign in cannot be loaded from the Web (e.g., offline, captive portal, etc.)
Categories
(Firefox :: Sync, defect, P1)
Tracking
()
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: edwong, Assigned: eoger)
References
Details
(Whiteboard: [fxsync])
Attachments
(1 file, 5 obsolete files)
13.64 KB,
patch
|
eoger
:
review+
|
Details | Diff | Splinter Review |
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.
Updated•10 years ago
|
Whiteboard: [qa+]
Comment 2•10 years ago
|
||
Not going to address this in 29 (we have no strings for it). The normal error pages should appear, which I think is fine.
Updated•9 years ago
|
Priority: -- → P2
Updated•9 years ago
|
Summary: properly message user when FxA sign in form is not accessible → properly message user when FxA sign in cannot be loaded from the Web (e.g., offline, captive portal, etc.)
Updated•9 years ago
|
Priority: P2 → P1
Comment 3•9 years ago
|
||
We need some UX around what we should show when we can load the FxA login page.
Flags: needinfo?(rfeeley)
Reporter | ||
Updated•9 years ago
|
Rank: 15
Assignee | ||
Updated•9 years ago
|
Assignee: nobody → edouard.oger
Assignee | ||
Comment 4•9 years ago
|
||
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?
Assignee | ||
Comment 5•9 years ago
|
||
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!
Assignee | ||
Comment 6•9 years ago
|
||
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•9 years ago
|
||
Proposed using the basic Firefox Accounts template: h1. No connection h2. You must be connected to the internet to sign in. button. Try again
Flags: needinfo?(rfeeley)
Reporter | ||
Updated•9 years ago
|
Whiteboard: [qa+] → [fxsync]
Assignee | ||
Comment 8•9 years ago
|
||
Proposed solution for the detection: use the nsIHttpChannel attached to the iframe and check response status code.
Reporter | ||
Updated•9 years ago
|
Iteration: --- → 42.3 - Aug 10
Assignee | ||
Comment 9•9 years ago
|
||
As expected by markh, the testing was the most difficult part of the work.
Attachment #8632385 -
Attachment is obsolete: true
Attachment #8641363 -
Flags: review?(markh)
Comment 10•9 years ago
|
||
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.
Attachment #8641363 -
Flags: review?(markh) → review-
Updated•9 years ago
|
Flags: firefox-backlog+
Comment 11•9 years ago
|
||
eoger: is this still on your list? mobile/android wants to copy your approach :)
Flags: needinfo?(edouard.oger)
Assignee | ||
Comment 12•9 years ago
|
||
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).
Flags: needinfo?(edouard.oger)
Assignee | ||
Comment 13•9 years ago
|
||
Here's the last version of the patch. What do we do?
Attachment #8641363 -
Attachment is obsolete: true
Flags: needinfo?(markh)
Updated•9 years ago
|
Iteration: 42.3 - Aug 10 → 43.1 - Aug 24
Comment 14•9 years ago
|
||
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•9 years ago
|
||
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.
Flags: needinfo?(markh)
Assignee | ||
Comment 16•9 years ago
|
||
Rebased and merged with your patch Mark.
Attachment #8645383 -
Attachment is obsolete: true
Attachment #8646764 -
Attachment is obsolete: true
Attachment #8647249 -
Flags: review?(markh)
Assignee | ||
Comment 17•9 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=6a381c0d8e2c
Assignee | ||
Updated•9 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 18•9 years ago
|
||
Try looks ok
Comment 19•9 years ago
|
||
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
Attachment #8647249 -
Flags: review?(markh) → review+
Assignee | ||
Comment 20•9 years ago
|
||
Carrying previous r+. Reverted the listener removal too.
Attachment #8647249 -
Attachment is obsolete: true
Attachment #8647609 -
Flags: review+
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Updated•9 years ago
|
Target Milestone: Firefox 29 → ---
Comment 21•9 years ago
|
||
https://hg.mozilla.org/integration/fx-team/rev/654c94c6320a
Keywords: checkin-needed
Comment 22•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/654c94c6320a
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → Firefox 43
Comment 23•9 years ago
|
||
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•9 years ago
|
||
(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?
Flags: needinfo?(rfeeley)
Comment 25•9 years ago
|
||
House stytle: Internet — always capitalized. https://www.mozilla.org/en-US/styleguide/communications/copy-rules/
Flags: needinfo?(rfeeley)
Comment 26•9 years ago
|
||
(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?
Flags: needinfo?(francesco.lodolo)
Comment 27•9 years ago
|
||
(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.
Flags: needinfo?(francesco.lodolo)
Comment 28•9 years ago
|
||
Thanks - pushed the trivial fix with r=me https://hg.mozilla.org/integration/fx-team/rev/95e78f3ea1f5
You need to log in
before you can comment on or make changes to this bug.
Description
•