Closed
Bug 435325
Opened 17 years ago
Closed 13 years ago
Offline-mode error page should switch to online mode when clicking 'Try Again'
Categories
(Core :: DOM: Navigation, enhancement)
Core
DOM: Navigation
Tracking
()
VERIFIED
FIXED
mozilla7
People
(Reporter: avarma, Assigned: steffen.wilberg)
References
Details
Attachments
(1 file, 8 obsolete files)
(deleted),
patch
|
steffen.wilberg
:
review+
steffen.wilberg
:
ui-review+
|
Details | Diff | Splinter Review |
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•17 years ago
|
||
(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.
Updated•17 years ago
|
Assignee: avarma → nobody
Component: General → Embedding: Docshell
Product: Firefox → Core
QA Contact: general → docshell
Version: 3.0 Branch → Trunk
Comment 2•17 years ago
|
||
Aren't the error pages unprivileged? That is, doesn't this need UI support?
Reporter | ||
Comment 3•17 years ago
|
||
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•16 years ago
|
||
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."
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 8•16 years ago
|
||
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.
Updated•15 years ago
|
Severity: normal → enhancement
Comment 12•14 years ago
|
||
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.
Assignee | ||
Updated•14 years ago
|
Assignee: nobody → steffen.wilberg
Status: NEW → ASSIGNED
Assignee | ||
Comment 13•14 years ago
|
||
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.
Assignee | ||
Comment 14•14 years ago
|
||
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.
Attachment #481030 -
Flags: review?(bzbarsky)
Assignee | ||
Comment 15•14 years ago
|
||
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.
Summary: "Offline Mode" error page should have a "Work Online" button if applicable → Offline-mode error page should switch to online mode when clicking 'Try Again'
Comment 16•14 years ago
|
||
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.
Attachment #481030 -
Flags: review?(bzbarsky) → review-
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.
Assignee | ||
Comment 18•14 years ago
|
||
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.
Attachment #481030 -
Attachment is obsolete: true
Attachment #482065 -
Flags: review?(bzbarsky)
Assignee | ||
Updated•14 years ago
|
Attachment #481027 -
Attachment is obsolete: true
Assignee | ||
Comment 19•14 years ago
|
||
Comment 20•14 years ago
|
||
Comment on attachment 482065 [details] [diff] [review]
with test
s/disable the disk cache/disable the cache/, right?
Attachment #482065 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 21•14 years ago
|
||
Of course, and thanks for the quick review.
Assignee | ||
Comment 22•14 years ago
|
||
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"...
Attachment #482065 -
Flags: review?(gavin.sharp)
Attachment #482065 -
Flags: feedback?(l10n)
Assignee | ||
Comment 23•14 years ago
|
||
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•14 years ago
|
||
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•14 years ago
|
||
Comment on attachment 482065 [details] [diff] [review]
with test
I guess gavin's right here on the l10n part, feedback- from that.
Attachment #482065 -
Flags: feedback?(l10n) → feedback-
Assignee | ||
Comment 26•14 years ago
|
||
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
Attachment #482065 -
Attachment is obsolete: true
Attachment #482670 -
Flags: review?(gavin.sharp)
Attachment #482670 -
Flags: feedback?(l10n)
Attachment #482065 -
Flags: review?(gavin.sharp)
Comment 27•14 years ago
|
||
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.
Attachment #482670 -
Flags: feedback?(l10n) → feedback+
Comment 28•14 years ago
|
||
Does using a new tab for the test work if you ignore the first/about:blank DOMContentLoaded event?
Assignee | ||
Comment 29•14 years ago
|
||
Cool, that works!
Attachment #482670 -
Attachment is obsolete: true
Attachment #482920 -
Flags: review?(gavin.sharp)
Attachment #482670 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 30•14 years ago
|
||
Note to self: Don't forget to rev the uuid in nsIDOMWindowUtils.idl.
Assignee | ||
Comment 31•14 years ago
|
||
If this still has a chance to land for Firefox 4, I'd use nsIDOMWindowUtils_MOZILLA_2_0_BRANCH instead.
Attachment #482920 -
Attachment is obsolete: true
Attachment #495148 -
Flags: review?(gavin.sharp)
Attachment #482920 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 32•14 years ago
|
||
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...
Attachment #495148 -
Attachment is obsolete: true
Attachment #495178 -
Flags: review?(gavin.sharp)
Attachment #495148 -
Flags: review?(gavin.sharp)
Assignee | ||
Comment 34•14 years ago
|
||
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.
Attachment #495178 -
Attachment is obsolete: true
Attachment #528082 -
Flags: ui-review?(faaborg)
Attachment #528082 -
Flags: review?(gavin.sharp)
Attachment #495178 -
Flags: review?(gavin.sharp)
Comment 35•13 years ago
|
||
>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•13 years ago
|
||
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.
Attachment #528082 -
Flags: ui-review?(faaborg) → ui-review+
Comment 37•13 years ago
|
||
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.
Attachment #528082 -
Flags: review?(gavin.sharp) → review+
Assignee | ||
Comment 38•13 years ago
|
||
(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.
Attachment #528082 -
Attachment is obsolete: true
Attachment #538673 -
Flags: ui-review+
Attachment #538673 -
Flags: review+
Assignee | ||
Comment 39•13 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla7
Comment 40•13 years ago
|
||
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.
Status: RESOLVED → VERIFIED
You need to log in
before you can comment on or make changes to this bug.
Description
•