Last Comment Bug 435325 - Offline-mode error page should switch to online mode when clicking 'Try Again'
: Offline-mode error page should switch to online mode when clicking 'Try Again'
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: Document Navigation (show other bugs)
: Trunk
: All All
: -- enhancement with 6 votes (vote)
: mozilla7
Assigned To: Steffen Wilberg
:
Mentors:
: 189696 325129 448265 477580 494173 539459 (view as bug list)
Depends on: 707570
Blocks: 61689 663253 663608
  Show dependency treegraph
 
Reported: 2008-05-22 16:20 PDT by Atul Varma [:atul]
Modified: 2013-09-10 14:28 PDT (History)
28 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
hacky approach using BrowserOnClick, Firefox only (2.23 KB, patch)
2010-10-05 13:37 PDT, Steffen Wilberg
no flags Details | Diff | Splinter Review
real patch (9.17 KB, patch)
2010-10-05 13:43 PDT, Steffen Wilberg
bzbarsky: review-
Details | Diff | Splinter Review
with test (12.78 KB, patch)
2010-10-09 11:59 PDT, Steffen Wilberg
bzbarsky: review+
l10n: feedback-
Details | Diff | Splinter Review
dropped overrideLongDesc2 (12.34 KB, patch)
2010-10-12 14:21 PDT, Steffen Wilberg
l10n: feedback+
Details | Diff | Splinter Review
ignore the first "about:blank" DOMContentLoaded event (12.16 KB, patch)
2010-10-13 11:30 PDT, Steffen Wilberg
no flags Details | Diff | Splinter Review
unbitrotted (13.05 KB, patch)
2010-12-03 15:32 PST, Steffen Wilberg
no flags Details | Diff | Splinter Review
also set the browser.offline pref (13.44 KB, patch)
2010-12-03 17:00 PST, Steffen Wilberg
no flags Details | Diff | Splinter Review
also includes changes for mobile (16.32 KB, patch)
2011-04-25 04:52 PDT, Steffen Wilberg
gavin.sharp: review+
faaborg: ui‑review+
Details | Diff | Splinter Review
final patch (16.58 KB, patch)
2011-06-11 02:37 PDT, Steffen Wilberg
steffen.wilberg: review+
steffen.wilberg: ui‑review+
Details | Diff | Splinter Review

Description Atul Varma [:atul] 2008-05-22 16:20:53 PDT
Additionally, the "Try Again" button should be removed from the page, because there's no use in "trying again" if you're still in offline mode.

Ideally, display of the "Work Online" button should be conditional, only being displayed if we know for sure that a network connection is actually available.  One (admittedly naive) approach to this would be to actually attempt to connect to the site in question via Ajax at a set interval, enabling or disabling the button depending on the result of the Ajax ping.

This bug was created from the conversation on 426932:

  https://bugzilla.mozilla.org/show_bug.cgi?id=426932#c16
Comment 1 Martijn Wargers [:mwargers] (not working for Mozilla) 2008-05-23 02:10:09 PDT
(In reply to comment #0)
> Additionally, the "Try Again" button should be removed from the page, because
> there's no use in "trying again" if you're still in offline mode.

Well, I think Firefox should go out of the offline mode and try again.
Comment 2 Boris Zbarsky [:bz] 2008-05-24 20:57:18 PDT
Aren't the error pages unprivileged?  That is, doesn't this need UI support?
Comment 3 Atul Varma [:atul] 2008-05-29 17:19:06 PDT
I believe Boris is correct; the template for the error pages appears to be located at mozilla/docshell/resources/content/netError.xhtml, and aside from the fact that the directory is called "content", everything in netError.xhtml seems to be using unprivileged code.

Given that the "Try Again" button is simply bound to call "location.reload()", it seems as though the way the error page works is by "pretending" to be the URL that the user is trying to go to.  This suggests that giving this page chrome privileges (so it can disable offline mode) may create security vulnerabilities.

I think there are potentially other ways to get around this issue, though, so I'm going to investigate some more.  If anyone else has any ideas though, please feel free to post them here.
Comment 4 Joseph Farmer 2008-06-08 12:19:11 PDT
This may sound silly but let me describe the process as it occurred to me:
Start Firefox.  
Get "offline mode" message.  What does that mean?  I checked the preferences and didn't see anything.  The proxy settings didn't do anything.  What is "offline mode?"  My network is fine.
Can't google it, I'm in offline mode.  
Start Seamonkey.  That works, google "firefox offline mode."  See a message where somebody accidentally turned it on in the "File" menu.  Is there such an option?  Start Firefox.  Check "File" menu.  Ah, that's it.  How did that get checked?  No idea.

Simple fix:  Add text to error message: "check to see if off-line mode is selected in the file menu."

Comment 5 Ria Klaassen (not reading all bugmail) 2008-07-28 09:30:48 PDT
*** Bug 448265 has been marked as a duplicate of this bug. ***
Comment 6 Robert O'Callahan (:roc) (email my personal email if necessary) 2008-08-31 15:01:38 PDT
Seems to me we could expose a DOMWindowUtils method to toggle online/offline state, and make it check the URL principal of the originating document to ensure it's about:neterror. Even if someone subverts about:neterror, it's not really a security issue to switch the browser from offline to online or back again --- just an annoyance.
Comment 7 Henrik Skupin (:whimboo) 2009-01-14 03:36:57 PST
*** Bug 189696 has been marked as a duplicate of this bug. ***
Comment 8 John David Galt 2009-01-14 18:24:06 PST
I still believe the online/offline mechanism ought to work as I suggested in bug 284004, so that it would automatically do the right thing for dialup users.

A summary:
1.  All applications (TB, FF, and SeaMonkey) should have the -offline command line option restored (bug 64172), so users can have shortcuts that determine this status explicitly.

2.  Mail clients should start up online by default.

3.  Browsers should start up online by default if started by clicking on a remote URL.  They should start up offline if started by clicking on a local URL.  (For this purpose, URLs that begin with "about:" or "file:" are local, all others are remote.)  When restoring a browser session, the state from the old session should be preserved.

4.  Attempting to download mail/news or visit a remote URL when offline should cause the application to go online, automatically and silently.  Exception: remote accesses that weren't requested by the user (such as attempts to check for updated versions of Mozilla or add-ons) should be automatically disabled while offline.

5.  If the application "thinks" it is online but the connection fails, as in the error message that provoked but 189696, it should go offline silently.

And 6.  Mozilla should abandon the practice of popping up unwanted modal dialogs for any purpose, ever.  Or at least provide users with a global option to turn them off.  But I think that's out of scope for this bug.
Comment 9 Jo Hermans 2009-02-09 04:44:57 PST
*** Bug 477580 has been marked as a duplicate of this bug. ***
Comment 10 Jo Hermans 2009-05-21 11:44:37 PDT
*** Bug 494173 has been marked as a duplicate of this bug. ***
Comment 11 Henrik Skupin (:whimboo) 2010-01-13 09:51:11 PST
*** Bug 539459 has been marked as a duplicate of this bug. ***
Comment 12 Yesudeep Mangalapilly 2010-07-24 08:28:49 PDT
In addition to this, with the latest Firefox 4.0 beta 1, there's a usability issue.  The default layout of the browser does not show the menu bar and only a single menu button labeled "Firefox".  So the message "Uncheck 'Work Offline' in the File menu." does not make sense at all.  

I had to enable the menu bar and then click File > Work Offline.

This message page should contain a "Go online and try again." button and a similar button should be added to the "Firefox" menu.  "Try again" makes absolutely no sense.

This should also be taken as a reminder to check all references to MENUS in message boxes and pages, because most people won't see a "File" or "Edit" menu if Firefox 4.0 ships with a single "Firefox" application menu.

Cheers,
Yesudeep.
Comment 13 Steffen Wilberg 2010-10-05 13:37:42 PDT
Created attachment 481027 [details] [diff] [review]
hacky approach using BrowserOnClick, Firefox only

This is the hacky approach, which reuses browser.js' existing BrowserOnClick handler to watch for clicks on the "Try Again" button.
It switches Firefox online and reloads the page.

The reload actually happens twice, once from neterror.xhtml and once from browser.js...
But it's a short and simple patch and works just fine.
Comment 14 Steffen Wilberg 2010-10-05 13:43:24 PDT
Created attachment 481030 [details] [diff] [review]
real patch

This is the more involved approach suggested by roc in comment 6.

Adds a goOnline() method to nsDOMWindowUtils, which can be called from unprivileged script, but throws a security error if the caller is not about:neterror.

In the neterror is netOffline case, it calls goOnline() before reloading the page.

Please check the C++ code carefully, I'm a novice in this area.
Comment 15 Steffen Wilberg 2010-10-05 14:02:35 PDT
The page only needs one button, which goes online and reloads the page.
(I don't consider reloading in offline mode a relevant use-case, that's just the frustrating experience of today, where clicking "Try Again" doesn't yield any result. So we don't need a button which stays offline and reloads.)

But maybe we can find a better button label than "Try Again".
"Go Online and Reload" is more precise, but a bit long.
Comment 16 Boris Zbarsky [:bz] 2010-10-05 21:34:42 PDT
Comment on attachment 481030 [details] [diff] [review]
real patch

The check for about:neterror should use StringBeginsWith, and probably shouldn't match "about:neterroring", say.

Other than that and the fact that I'd totally expect this to break if we stop allowing Components.interfaces in untrusted scripts (which is worth a test that will start failing and point to this code!), I guess this is ok.
Comment 17 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-10-07 22:26:21 PDT
I don't think the text should be "Go Online And Reload". I think it should probably just be "Try Again". Clicking that button will not actually get the user online if they currently aren't.
Comment 18 Steffen Wilberg 2010-10-09 11:59:44 PDT
Created attachment 482065 [details] [diff] [review]
with test

Uses StringBeginsWith, checks for "about:neterror?", and adds a test.

The test switches the app to offline mode, disables disk and memory caches, and tries to load http://example.com. It checks that the app is in offline mode, it has loaded the offline mode neterror page, and that has the Try Again button.
It clicks that button and checks whether the app switched to online mode. The error message includes a note that Components.interfaces.nsIDOMWindowUtils needs to be available in untrusted content, and this bug.

The DOMContentLoaded event doesn't work correctly when opening the error page
in a new tab; I get "about:blank" as its documentURI. So I simply use the current tab and set content.location to "about:blank" in the cleanup function.
Comment 20 Boris Zbarsky [:bz] 2010-10-12 08:29:12 PDT
Comment on attachment 482065 [details] [diff] [review]
with test

s/disable the disk cache/disable the cache/, right?
Comment 21 Steffen Wilberg 2010-10-12 08:30:43 PDT
Of course, and thanks for the quick review.
Comment 22 Steffen Wilberg 2010-10-12 08:43:11 PDT
Comment on attachment 482065 [details] [diff] [review]
with test

Requesting additional review for the strings in browser/.
Please see comment 15 and comment 17 on that.
This is a much better solution than bug 593125, which basically says: "Uncheck the Work Offline menu item, but you need to find it yourself, I won't tell you"...
Comment 23 Steffen Wilberg 2010-10-12 08:56:28 PDT
Oh, and we could even undo the changes from bug 598310, which added a "Work Offline" menu item to the Firefox appmenu button to recover from Offline mode.
Comment 24 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-10-12 11:22:35 PDT
If you're going to change netOffline.longDesc, I don't think there's any point in keeping netOffline.overrideLongDesc2, given that it's only purpose was for fallback for netError.dtd overriders that didn't update their overrides. With this changes we'll need to update all of the known overrides anyways (Thunderbird, Fennec at the very least).
Comment 25 Axel Hecht [:Pike] 2010-10-12 11:45:49 PDT
Comment on attachment 482065 [details] [diff] [review]
with test

I guess gavin's right here on the l10n part, feedback- from that.
Comment 26 Steffen Wilberg 2010-10-12 14:21:03 PDT
Created attachment 482670 [details] [diff] [review]
dropped overrideLongDesc2

Addressed comments 20 and 24.

http://hg.mozilla.org/comm-central/annotate/3fb3186c8172/mail/locales/en-US/chrome/overrides/netError.dtd#l55 says:
  "Place &brandShortName; in online mode, then try again."
That needs an update, which I'll do in a follow-up bug.

http://mxr.mozilla.org/mobile-browser/source/locales/en-US/chrome/overrides/netError.dtd#56 says:
  "Try again. &brandShortName; will attempt to open a connection and reload the page."
That already matches the behavior of my patch. That's because Fennec has a click handler which we should remove in a follow-up bug, which I'll file:
http://hg.mozilla.org/mobile-browser/annotate/18c77a6b48bf/chrome/content/browser.js#l784
Comment 27 Axel Hecht [:Pike] 2010-10-12 15:17:46 PDT
Comment on attachment 482670 [details] [diff] [review]
dropped overrideLongDesc2

I'd welcome a more descriptive change to the entity name than netOffline.longDesc2, not that I have one ad-hoc.

This looks OK from an l10n point of view otherwise, though.
Comment 28 :Gavin Sharp [email: gavin@gavinsharp.com] 2010-10-13 00:42:16 PDT
Does using a new tab for the test work if you ignore the first/about:blank DOMContentLoaded event?
Comment 29 Steffen Wilberg 2010-10-13 11:30:18 PDT
Created attachment 482920 [details] [diff] [review]
ignore the first "about:blank" DOMContentLoaded event

Cool, that works!
Comment 30 Steffen Wilberg 2010-10-21 13:52:41 PDT
Note to self: Don't forget to rev the uuid in nsIDOMWindowUtils.idl.
Comment 31 Steffen Wilberg 2010-12-03 15:32:40 PST
Created attachment 495148 [details] [diff] [review]
unbitrotted

If this still has a chance to land for Firefox 4, I'd use nsIDOMWindowUtils_MOZILLA_2_0_BRANCH instead.
Comment 32 Steffen Wilberg 2010-12-03 17:00:11 PST
Created attachment 495178 [details] [diff] [review]
also set the browser.offline pref

Ugh, the previous patches had a bug:
When you select "Work Offline" from the menu to test this, then hit the neterror page and click the "Try Again" button to go online, it works.
But after restarting Firefox, it is in offline mode again until you de-select "Work Offline" from the menu.

This is because setting the menu item also sets the pref "browser.offline", and this pref is read at profile startup (in BG__onProfileStartup()) to set the offline mode: http://mxr.mozilla.org/mozilla-central/source/browser/components/nsBrowserGlue.js#345
At least in the (!Services.io.manageOfflineStatus) case - why would that be false on startup?

Anyway, this patch also sets the "browser.offline" pref to false when going online. The alternative would be to rip out that pref altogether...
Comment 33 :Ms2ger (⌚ UTC+1/+2) 2011-01-12 06:08:40 PST
*** Bug 325129 has been marked as a duplicate of this bug. ***
Comment 34 Steffen Wilberg 2011-04-25 04:52:37 PDT
Created attachment 528082 [details] [diff] [review]
also includes changes for mobile

This is the unbitrotted patch from December 3 (!) plus changes for mobile, now that mobile-browser has been merged into mozilla-central:
- Removed the call to Util.forceOnline() if about:neterror?e=netOffline is displayed, since the neterror page now sets Services.io.offline = false itself. Can't remove Util.forceOnline() altogether since that's used elsewhere.
- Updated the name of the netOffline.longDesc2 string.

Alex, please comment on the suggested text for Firefox:
  Press "Try Again" to switch to online mode and reload the page.
Mobile already uses this:
  Try again. &brandShortName; will attempt to open a connection and reload the page.
Comment 35 Alex Faaborg [:faaborg] (Firefox UX) 2011-06-01 15:45:47 PDT
>Press "Try Again" to switch to online mode and reload the page.

That's fine, or alternatively we could just keep the string the same (it's probably assumed that reloading would take you into online mode, even if we didn't do that in the past).
Comment 36 Alex Faaborg [:faaborg] (Firefox UX) 2011-06-01 15:46:19 PDT
Comment on attachment 528082 [details] [diff] [review]
also includes changes for mobile

the mobile string of "opening a connection" is a little jargony, but otherwise this is fine.
Comment 37 :Gavin Sharp [email: gavin@gavinsharp.com] 2011-06-02 17:27:37 PDT
Comment on attachment 528082 [details] [diff] [review]
also includes changes for mobile

>diff --git a/docshell/test/browser/browser_bug435325.js b/docshell/test/browser/browser_bug435325.js

This should probably have one of those PD license headers (https://www.mozilla.org/MPL/boilerplate-1.1/pd-c)

>diff --git a/dom/base/nsDOMWindowUtils.cpp b/dom/base/nsDOMWindowUtils.cpp

>+nsDOMWindowUtils::GoOnline()

>+    nsIPrefBranch2 *prefBranch = nsContentUtils::GetPrefBranch();
>+    if (prefBranch)
>+      prefBranch->SetBoolPref("browser.offline", PR_FALSE);

I don't think core code should be touching this pref. It's Firefox-specific AFAICT. Our use of browser.offline is bogus and I think we should get rid of it, but that should happen in a followup bug (I don't think the issue in comment 32 needs to block this landing).

http://mxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/overrides/netError.dtd also needs updating here (file a followup at least).

r=me with those addressed; I apologize for the unreasonable delay in getting to this.
Comment 38 Steffen Wilberg 2011-06-11 02:37:23 PDT
Created attachment 538673 [details] [diff] [review]
final patch

(In reply to comment #37)
> >diff --git a/docshell/test/browser/browser_bug435325.js b/docshell/test/browser/browser_bug435325.js
> 
> This should probably have one of those PD license headers
> (https://www.mozilla.org/MPL/boilerplate-1.1/pd-c)
done

> >diff --git a/dom/base/nsDOMWindowUtils.cpp b/dom/base/nsDOMWindowUtils.cpp
> 
> >+nsDOMWindowUtils::GoOnline()
> 
> >+    nsIPrefBranch2 *prefBranch = nsContentUtils::GetPrefBranch();
> >+    if (prefBranch)
> >+      prefBranch->SetBoolPref("browser.offline", PR_FALSE);
> 
> I don't think core code should be touching this pref. It's Firefox-specific
> AFAICT. Our use of browser.offline is bogus and I think we should get rid of
> it, but that should happen in a followup bug (I don't think the issue in
> comment 32 needs to block this landing).
Removed that part and filed bug 663253.
I also needed to adjust for a new compile error. Instead of adding
#include "nsNetCID.h", I'm now adding #include "nsIIOService.h", which makes more sense anyway.

> http://mxr.mozilla.org/comm-central/source/mail/locales/en-US/chrome/
> overrides/netError.dtd also needs updating here (file a followup at least).
Filed bug 663608 and added a patch.
Comment 40 Simona B [:simonab ] -PTO- back Sept 5th 2011-09-06 07:16:03 PDT
Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0

Verified on Win XP, Win 7, Ubuntu 11.04 and Mac OS X 10.6 - clicking "Try again" in the Offline-mode error page switches you to the online  mode.

Setting resolution to VERIFIED FIXED.

Note You need to log in before you can comment on or make changes to this bug.