Closed Bug 1025569 Opened 10 years ago Closed 10 years ago

Notifications for requests from Offline Web Applications offer beyond-session options in Private Browsing mode

Categories

(SeaMonkey :: UI Design, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
seamonkey2.30

People

(Reporter: rsx11m.pub, Assigned: rsx11m.pub)

References

(Blocks 1 open bug, )

Details

(Keywords: privacy)

Attachments

(2 files, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #1019583 +++

I'm spinning this off the bug for modifying the the default preferences and adapting the preferences UI as result of the changes from Core bug 892488. This issue relates more to the notification itself and to the Private Browsing mode in general.

(Quoting neil@parkwaycc.co.uk from bug 1019583 comment #10)
> (Hmm, I get offered "Never for this site" in a private window... oops?)

(Quoting rsx11m from bug 1019583 comment #19)
> (In reply to neil@parkwaycc.co.uk from bug 1019583 comment #17)
> > Actually I'm not sure what correct behaviour is for offline apps in private windows.
> 
> It behaves as if "Not Now" was clicked in Firefox, regardless of the
> preference settings (i.e., the test site simply shows no action). This makes
> sense, given that Private Browsing isn't supposed to leave any traces, and
> the offline store is always located on the disk (but then, how is PB done
> with cookies, which aren't allowed to persist either?). Thus, either the
> notification shouldn't be shown in a private window at all even with the
> notify pref set (that's what Firefox does); or maybe even better, the
> notification should just state that offline storage isn't available in a
> private window and only offer to close the notification bar.

(Quoting to rsx11m from bug 1019583 comment #20)
> > It behaves as if "Not Now" was clicked in Firefox, regardless of the
> > preference settings (i.e., the test site simply shows no action).
> 
> Err, it doesn't. I've retested that with a new profile and
> "allow_by_default" disabled, and Firefox trunk behaves the same, offering to
> allow the site to use offline storage. It's just that with the
> "allow_by_default" setting it apparently won't accept it.

As another observation, the "Allow" label should actually be "Always Allow" given that it's not just allowing the offline-storage usage for the current session but for future visits to that site as well (except when in private browsing mode, in which case the site-specific "allow" setting is ignored but the notification bar not re-shown either).

Thus, all arguments point to not showing the buttons on the notification bar at all and just posting an information message that the user is in Private Browsing mode and hence the request was blocked to avoid storing data on the local disk.
Attached image Proposed design
This is where I am. In a private window, the notification bar states at the beginning that you are in a private window. The second part of the string is similar to the usual text, but explicitly states that the request was blocked. The little "Close" button works as expected, no other option is offered.

I'm not sure about the "Always Allow" string. In the doorhanger notifications, the proposed labels are "Always for This Site" vs. "Never for This Site" which would add significant width here, though. Given that "This site (...)" is part of the label, it should be obvious what "Always" relates to, and we it again in the "Never for ..." label. Thus, I'd assume that it's ok to use the short label.
This is confusing, there are actually three instances where "offlineApps.allow" (and its counterpart "offlineApps.deny", which actually should be named ".always" and ".never" to correctly express what they are doing) is used:

> http://mxr.mozilla.org/comm-central/search?string=offlineApps.allow&find=suite/

- First instance is in <method name="offlineAppRequested"> (that's the one I've modified);
- then there is <method name="promptIndexedDB"> as part of "browserNotificationBindings";
- with a doorhanger <method name="promptIndexedDB"> in the "popup-notification" binding.

So, how do I get to "promptIndexedDB" in order to test this, assuming that it's behaving identical to the "offlineAppRequested" method which I get when just visiting the web site?
Flags: needinfo?(neil)
Attached patch Proposed patch (obsolete) — Splinter Review
This introduces the private-browsing notification bar with a different label and no buttons to make "[x] Close this message" the only option. It also changes the label of the "Allow" button to "Always Allow" with change of the underlying properties from ".allow"/".deny" to ".always"/".never" to reflect that it's a permanent entry and to be consistent with the doorhanger properties (where ".allow" means "this instance only").

Grammar question: The screenshot shows "This website () is not allowed ..." which I've changed to "has not been allowed" in the patch to be less general and better convey that this specific request has been ignored (which is why "has been denied" would be wrong as no response is actually given to the server). Please rephrase if that's not the intended meaning.

I have made the corresponding changes for the promptIndexedDB() methods, unfortunately without being able to test those. The examples I tried from https://developer.mozilla.org/en-US/demos/tag/tech:indexeddb all seem to invoke offlineAppRequested() instead. Maybe one of you has a working example to verify that it works. According to the documentation for the popup notification, passing null as mainAction parameter shouldn't show the button at all while ignoring any secondaryActions specified, which is what's intended.

Line wrapping in the two "var message = ..." statements is tricky, but maybe acceptable in this way. As a drive-by fix, I have corrected PRIOITY_INFO_HIGH (note the typo) to PRIORITY_INFO_LOW, thus it corresponds now to the level in the offlineAppRequested() version.

I have also updated the Help content, which means that this patch has to be applied on top of bug 1019583 attachment 8440383 [details] [diff] [review] (due to the reduced context, it can be applied after bug 1019986 attachment 8436327 [details] [diff] [review] without being bitrotted). I've double-checked that the "Browsing in a Private Window" section makes the correct claims in the "Cached and Offline Content" description.
Attachment #8441752 - Flags: ui-review?(neil)
Attachment #8441752 - Flags: review?(iann_bugzilla)
(In reply to rsx11m from comment #2)
> So, how do I get to "promptIndexedDB" in order to test this, assuming that
> it's behaving identical to the "offlineAppRequested" method which I get when
> just visiting the web site?

Hmm, looks like the default is to allow indexedDB unless you "allow" it in Data Manager, at which point it starts prompting (and then when you allow it, it turns to "prompt" in Data Manager; yes, it really is back-to-front like that) except in private windows, where it's silently denied.

http://mxr.mozilla.org/comm-central/source/mozilla/dom/indexedDB/CheckPermissionsHelper.cpp#30
Flags: needinfo?(neil)
Wow, this is an ugly hack.  :-\

So, bug 758357 changed the semantics of those boxes and turned them upside down based on the expectation that "bug 742822 will make indexeddb less persistent" (which hasn't seen any action during the last two years, not to mention any patch), and that's hard-wired in the Core code's C++ code... I'm amazed.

Not much can be done here other than accept that this is how it is, so in general the changes made should be the right ones. The question is if that requires changes in the suite/ implementation of the Page Info and Data Manager dialogs to reflect the flipped logic in the user interface (Firefox has received such custom changes in attachment 638105 [details] [diff] [review]). If yes, that's yet another spin-off bug.
Attached patch Proposed patch (v2) (obsolete) — Splinter Review
Ok, the site in comment #3 now shows the prompt after forcing an explicit permission entry for "Store Local Databases" with value "Allow" for that site. Absurdly, clicking "Always Allow" removes that entry in the Data Manager, but it seems to do what it's supposed to do.

Both doorhanger notification and the notification bar if disabled appear and show the expected labels in a normal window when the demo is entered. There was an error in the old patch when determining the message to show (precedence of operands), which is resolved in this updated patch. Also, "Not Now" and "Never for This Site" were reversed compared to the Offline Apps notification bar, thus I've switched those two buttons to make it consistent.

Interestingly, neither the doorhanger nor the notification bar are showing in a private window, with or without the patch applied. Thus, apparently this case is caught upfront before invoking the method. I'd think that playing it safe and keeping the code distinguishing between normal and private window as proposed can't hurt in case some extension (or a future change) calls that method in a private context anyway, thus I left it with the correction of the equations.
Attachment #8441752 - Attachment is obsolete: true
Attachment #8441752 - Flags: ui-review?(neil)
Attachment #8441752 - Flags: review?(iann_bugzilla)
Attachment #8443106 - Flags: ui-review?(neil)
Attachment #8443106 - Flags: review?(iann_bugzilla)
(In reply to rsx11m from comment #7)
> Interestingly, neither the doorhanger nor the notification bar are showing
> in a private window, with or without the patch applied. Thus, apparently
> this case is caught upfront before invoking the method. I'd think that
> playing it safe and keeping the code distinguishing between normal and
> private window as proposed can't hurt in case some extension (or a future
> change) calls that method in a private context anyway, thus I left it with
> the correction of the equations.

[Apparently localisers don't like to localise unused messages...]
It's actually the same set of strings that is also used for the Offline Apps notification bar and doorhanger, so that doesn't apply and all strings are used (just not all in the IndexedDB context).
(In reply to rsx11m from comment #9)
> It's actually the same set of strings that is also used for the Offline Apps
> notification bar and doorhanger, so that doesn't apply and all strings are
> used (just not all in the IndexedDB context).

This one too?
>+offlineApps.private=You are in a private window. This website (%S) has not been allowed to store data on your computer for offline use.
Also, doesn't that make the help text inaccurate?
Comment on attachment 8443106 [details] [diff] [review]
Proposed patch (v2)

(In reply to neil@parkwaycc.co.uk from comment #10)
> This one too?
> >+offlineApps.private=You are in a private window. This website (%S) has not been allowed to store data on your computer for offline use.

Yes, here (for the "traditional" offline apps, not the IndexedDB types):

>-              var messageString = this._stringBundle.formatStringFromName("offlineApps.permissions", [host], 1);
>+              var messageString = this._stringBundle.formatStringFromName(this.usePrivateBrowsing ?
>+                "offlineApps.private" : "offlineApps.permissions", [host], 1);

> Also, doesn't that make the help text inaccurate?

Not quite, given that it's still accurate for the main offline apps use case:

>+    <p><strong>Note</strong>: The offline storage is <em>not</em> available in
>+      a <a href="using_priv_help.xhtml#browsing_in_a_private_window">private
>+      window</a>. You will receive a notification if this option is checked,
>+      but all requests will be blocked.</p>

Other than possibly using "may" rather than "will receive" to fuzzify this a bit whether or not a notification is received in private browsing mode.
[Considering that the user /will/ get a notification for traditional offline web applications but only /may/ get one for IndexedDB with or without PB mode = only if the minimum quota is exceeded, and will get a different message indicating how much is allocated /if/ that quota is reached, but only when the website uses IndexedDB rather than traditional offline apps, I find it hard to express this variety of cases in easy-to-grasp words for the Help text, so that was the most generic I could come up with. ;-) ]
Comment on attachment 8443106 [details] [diff] [review]
Proposed patch (v2)

Is there not a door hanger version of this notification?
Attachment #8443106 - Flags: review?(iann_bugzilla) → review+
Apparently the doorhanger was implemented only for the IndexedDB notification. The traditional offline applications version has the notification bar only and ignores the browser.doorhanger.enabled setting.
Filed bug 1035785 on comment #13/14.
Comment on attachment 8443106 [details] [diff] [review]
Proposed patch (v2)

>+              var messageString = this._stringBundle.formatStringFromName("offlineApps.private", [host], 1);
>               var priority = this.PRIORITY_INFO_LOW;
>               notification = this.appendNotification(messageString, notificationName,
>-                                                     null, priority, buttons);
>+                                                     null, priority, null);
[Given that this is all you need to do in the private case, I'm not convinced it's worth sharing the code.]

>+            var message = this._stringBundle.formatStringFromName("offlineApps."
>+                              + (this.usePrivateBrowsing ? "private" : aTopic),
Nit: operator at end of line, not start of next line. Maybe it would better if you used this.usePrivateBrowsing ? "offlineApps.private" : "offlineApps." + aTopic (if you can work out how to wrap it, maybe use a variable for the message string name), as that avoids concatenating strings in the private browsing case.

>-            var box = this.appendNotification(message, "indexedDB-" + aTopic + "-prompt", null,
>-                                              this.PRIOITY_INFO_HIGH, buttons);
>+            var box = this.appendNotification(message,
>+                                              "indexedDB-" + aTopic + "-prompt",
>+                                              null, this.PRIORITY_INFO_LOW,
[Hmm, I should double-check to find out what the original priorities were.]

>+                                        this.usePrivateBrowsing ? null : mainAction,
>+                                        secondaryActions, options);
So what happens here in a private window?
(In reply to neil@parkwaycc.co.uk from comment #16)
> >+              var messageString = this._stringBundle.formatStringFromName("offlineApps.private", [host], 1);
> >               var priority = this.PRIORITY_INFO_LOW;
> >               notification = this.appendNotification(messageString, notificationName,
> >-                                                     null, priority, buttons);
> >+                                                     null, priority, null);
> [Given that this is all you need to do in the private case, I'm not
> convinced it's worth sharing the code.]

So, you mean something like

  if (this.usePrivateBrowsing) {
    // the code above
  } else {
    // current code
  }

which would check this.usePrivateBrowsing only once but replicates the code rather than handling the differences in the "?:" constructs?

> >+                                        this.usePrivateBrowsing ? null : mainAction,
> >+                                        secondaryActions, options);
> So what happens here in a private window?

That's defined in PopupNotifications.show (toolkit/modules/PopupNotifications.jsm):

> 206    * @param mainAction
> 215    *        If null, the notification will not have a button, and
> 216    *        secondaryActions will be ignored.

Thus, it would be doing as desired and just show the notification which can be dismissed.
Comment on attachment 8443106 [details] [diff] [review]
Proposed patch (v2)

Don't worry about that, we do a similar thing in other notifications (fetching all the strings, then throwing away the non-private versions in private windows).
Attachment #8443106 - Flags: ui-review?(neil) → ui-review+
(In reply to neil@parkwaycc.co.uk from comment #16)
> Nit: operator at end of line, not start of next line. Maybe it would better
> if you used this.usePrivateBrowsing ? "offlineApps.private" : "offlineApps."
> + aTopic (if you can work out how to wrap it, maybe use a variable for the
> message string name), as that avoids concatenating strings in the private
> browsing case.

Sounds reasonable, I'll split this up into messageName (with the "?:" clause) and messageString.

(In reply to neil@parkwaycc.co.uk from comment #18)
> Don't worry about that, we do a similar thing in other notifications
> (fetching all the strings, then throwing away the non-private versions in
> private windows).

Ok, so I'll leave that part as it is.

I'm waiting for the bunch of patches on notifications to be pushed, thus I can create a new properly unbitrotted patch for check-in.
(In reply to neil@parkwaycc.co.uk from comment #16)
> Nit: operator at end of line, not start of next line. Maybe it would better
> if you used this.usePrivateBrowsing ? "offlineApps.private" : "offlineApps."
> + aTopic (if you can work out how to wrap it, maybe use a variable for the
> message string name), as that avoids concatenating strings in the private
> browsing case.

Well, the wrapping here is a tough exercise. The best I can come up with is:

>-            var message = this._stringBundle.formatStringFromName("offlineApps." + aTopic, [host, aData], 2);
>+            var property = this.usePrivateBrowsing ? "offlineApps.private" :
>+                                                     "offlineApps." + aTopic;
>+            var message = this._stringBundle.formatStringFromName(property,
>+                                                                  [host, aData], 2);

(In reply to rsx11m from comment #17)
> > >+                                        this.usePrivateBrowsing ? null : mainAction,
> > >+                                        secondaryActions, options);
> > So what happens here in a private window?
> 
> That's defined in PopupNotifications.show
> (toolkit/modules/PopupNotifications.jsm):
> 
> > 206    * @param mainAction
> > 215    *        If null, the notification will not have a button, and
> > 216    *        secondaryActions will be ignored.
> 
> Thus, it would be doing as desired and just show the notification which can be dismissed.

I've double-checked this by negating the logic, and the doorhanger indeed shows the described behavior.
Locally unbitrotted patch as applied after
  bug 998787  attachment 8452045 [details] [diff] [review],
  bug 1019986 attachment 8436327 [details] [diff] [review], and
  bug 1019583 attachment 8446252 [details] [diff] [review].
Attachment #8443106 - Attachment is obsolete: true
Attachment #8454972 - Flags: ui-review+
Attachment #8454972 - Flags: review+
Keywords: checkin-needed
Whiteboard: [c-n: needs to land after 1019986 and 1019583]
https://hg.mozilla.org/comm-central/rev/534051b7ae7d
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [c-n: needs to land after 1019986 and 1019583]
Target Milestone: --- → seamonkey2.30
You need to log in before you can comment on or make changes to this bug.