Enable notifications by default again for using offline storage and update Offline Web Applications preference pane

RESOLVED FIXED in seamonkey2.30

Status

defect
--
major
RESOLVED FIXED
5 years ago
5 years ago

People

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

Tracking

({privacy, regression})

Dependency tree / graph

SeaMonkey Tracking Flags

(seamonkey2.26 fixed, seamonkey2.27 wontfix, seamonkey2.28 fixed, seamonkey2.29 fixed)

Details

()

Attachments

(3 attachments, 7 obsolete attachments)

This seems to have regressed again - the oldest build I have exhibiting the issue is SeaMonkey/2.23a1 20130904, the 2.22a2 build around the same time still shows the notification bar. Reproducible in current 2.29a1 nightly.


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

When loading http://html5demos.com/offlineapp in Firefox, I get a notification where I can allow offline app caching, but I don't get that in SeaMonkey.

It just sets a permission for "offline-apps" in the end, and if that one is set, the demo works, but the notification is missing in SeaMonkey.
Definitely a privacy issue given that offline data from any site is silently accepted by default.
Keywords: privacy
And even worse, SeaMonkey even accepts offline data if "Notify me when a website wants to store data for offline use" is unchecked and thus the feature supposedly disabled. :-\
Severity: normal → major
Summary: Missing notification for allowing offline app cache → Missing notification for allowing websites to use offline web application cache, all requests are quietly accepted
That was "broken by design" in bug 892488 - setting offline-apps.allow_by_default to false fixes it. Thus, the default should be flipped so that we see the warnings initially again.
Assignee: nobody → rsx11m.pub
Status: NEW → ASSIGNED
xref Firefox bug 933993.
(Quoting Honza Bambas (:mayhemer) from bug 933993 comment #1)
> If you want to re-enable the prompt, just switch the pref
> offline-apps.allow_by_default to false in about:config.

Ok, so it should be safe to simply flip that preference to revert the change in default
(and might be the only thing suitable for the current branches due to l10n restrictions).

It is my understanding that the backend issues mentioned in the Firefox bug (like explicit blocking for a site not working) will be handled on the Core site, thus we don't have to care about those here.

> The UI is bound to browser.offline-apps.notify pref.
> This has not been changed by bug 892488.

Thus, the main problem is that with the current UI, the checked box suggests that the user will be asked when in fact he or she isn't because offline-apps.allow_by_default is true.

> We never had a UI for offline-apps.allow_by_default.
> We have to add it.

I'm not entirely sure if exposing the option to unconditionally accept all requests in the preferences UI is needed, but the current design is definitely misleading when the preference to allow by default is set.

There are a couple of possible variations coming to my mind, where the somewhat twisted logic of the underlying prefs isn't helping to make this both nice and easy.


Version A - Direct implementation of the preferences:

[x] Notify me when a website wants to store data for offline use
    [ ] Accept requests without asking me

where the first box remains unchanged and the second is bound to offline-apps.allow_by_default but would have to be explicitly unchecked and disabled if the user unchecks the "Notify" box.


Version B - Somewhat more intuitive using two checkboxes:

[x] Allow websites to store data for offline use
    [x] Ask me before accepting requests

Underlying logic:

[x] [x]   allow_by_default=false, notify=true
[x] [ ]   allow_by_default=true,  notify=any
[ ] any   allow_by_default=false, notify=false


Version C - Tristate radiogroup:

When a website wants to store data for offline use: 
(*) Ask me what to do
( ) Accept all requests
( ) Block all requests

Underlying logic:
Ask:     allow_by_default=false, notify=true
Accept:  allow_by_default=true,  notify=any
Block:   allow_by_default=false, notify=false


Personally I'd tend to Variant B given that C needs four lines of vertical space.

Opinions?
There are actually some more labels that need work in this prefpane:

> Your application cache is currently using [__] of disk space.

This is the only occasion where the word "application cache" is used, which may be confused with the general disk cache. Everywhere else we say "offline storage" thus I'd change that here.

> The following websites are allowed to store data for offline use:

This is misleading, /permissions/ are inspected in the Page Info or the Data Manager; this listbox only provides information about /usage/ of the offline storage by each website/domain.
Component: UI Design → Preferences
No longer blocks: 630092
Summary: Missing notification for allowing websites to use offline web application cache, all requests are quietly accepted → Enable notifications by default again for using offline storage and update Offline Web Applications preference pane
Posted patch Proposed patch (obsolete) — Splinter Review
This still looks like the best solution:

> Version B - Somewhat more intuitive using two checkboxes:
> 
> [x] Allow websites to store data for offline use
>     [x] Ask me before accepting requests
> 
> Underlying logic:
> 
> [x] [x]   allow_by_default=false, notify=true
> [x] [ ]   allow_by_default=true,  notify=any
> [ ] any   allow_by_default=false, notify=false

The patch implements the respective UI and logic, which isn't actually overly complex. It also takes care of the label corrections identified in comment #6, where I've also shortened the new label to make them fit into a single line rather than having just a word or too wrap into a second line.

>+    this.allowBox.checked = this.allowPref.value || this.askPref.value;
>+    this.askBox.checked   = !this.allowPref.value;

This initialization may need some explanation. If either the "allow_by_default" or the "notify" preference is true, accepting request isn't disabled and hence the upper box checked. For the lower box, there are two scenarios where it is checked: The first is that "notify" is enabled, thus we have allowBox already set (i.e., both boxes are enabled and checked), but the "allow_by_default" preference is not set in this case (otherwise the upper box would be checked but the lower must not). The second case is where requests are always blocked, thus allowBox is unchecked and askBox a don't-care parameter (and both prefs are false). In this case I want to check askBox, thus if the user checks the allowBox to enable the feature, askBox is checked by default already to make accepting requests without notification a conscious process.

>+    if (this.allowPref.locked || this.askPref.locked) {
>+      this.allowBox.disabled = true;
>+      this.askBox.disabled   = true;
>+    }

The logic seemed to be getting rather tricky when I tried to consider one of the preferences being locked and the other one still open to change, given that they don't match a checkbox each. Thus, if either of them is locked, I'm just disabling both boxes and hence freeze the current status after initialization.

>-function ReadOfflineNotify(aChecked)
>-{
>-  document.getElementById("offlineNotifyPermissions").disabled = aChecked;
>-}

I removed this as it's wrong. The "Manage Permissions" button just opens the Data Manager in the permissions tab, this should work at any time regardless of the permissions here. Also, setting both prefs to false doesn't disable the feature itself. If a website has been explicitly allowed to use the offline storage, it can still do so in this case.

I've also updated/corrected the Help content to match the new layout and the label changes.
Attachment #8438787 - Flags: ui-review?(neil)
Attachment #8438787 - Flags: review?(iann_bugzilla)
Posted image Screenshot (obsolete) —
Note that I've added a <separator class="thin"> between the checkboxes and the label for the listbox, otherwise it looked a bit too tight. I've aligned the "Manage Permissions" button with the "Ask" checkbox as it is logically closer (you set the site-specific permissions with the notification bar).
(Quoting rsx11m from comment #5)
> (Quoting Honza Bambas (:mayhemer) from bug 933993 comment #1)
> > If you want to re-enable the prompt, just switch the pref
> > offline-apps.allow_by_default to false in about:config.
> 
> Ok, so it should be safe to simply flip that preference to revert the change in default
> (and might be the only thing suitable for the current branches due to l10n restrictions).

Just for the remote case that this is still wanted for 2.26.1, here the branch patch against comm-release (should apply to all branches given the reduced context).
Attachment #8438830 - Flags: review?(neil)
Comment on attachment 8438830 [details] [diff] [review]
Part 1: Flip default [checked in; comment #14, comment #26]

(Hmm, I get offered "Never for this site" in a private window... oops?)
Attachment #8438830 - Flags: review?(neil) → review+
Comment on attachment 8438830 [details] [diff] [review]
Part 1: Flip default [checked in; comment #14, comment #26]

[Approval Request Comment]
Regression caused by (bug #): bug 892488
User impact if declined: requests from web sites to use the offline storage will be quietly accepted
Testing completed (on m-c, etc.): c-c and c-a builds
Risk to taking this patch (and alternatives if risky): minimum
String changes made by this patch: removed from this patch
Attachment #8438830 - Flags: approval-comm-release?
Attachment #8438830 - Flags: approval-comm-beta?
Attachment #8438830 - Flags: approval-comm-aurora?
(In reply to neil@parkwaycc.co.uk from comment #10)
> Comment on attachment 8438830 [details] [diff] [review]
> Branch patch
> 
> (Hmm, I get offered "Never for this site" in a private window... oops?)

Can't be me, this is just flipping the preference. I'll look if there is a bug pending already, otherwise see if it's easy to add as a ride-along to the patch with the UI change.
Hmm, so this may be just a tick too late to land on the 2.26.1 release branch. Callek?
Blocks: 1018792
Flags: needinfo?(bugspam.Callek)
Attachment #8438830 - Flags: approval-comm-release?
Attachment #8438830 - Flags: approval-comm-release+
Attachment #8438830 - Flags: approval-comm-beta?
Attachment #8438830 - Flags: approval-comm-beta+
Attachment #8438830 - Flags: approval-comm-aurora?
Attachment #8438830 - Flags: approval-comm-aurora+
Flags: needinfo?(bugspam.Callek)
So, I take it the annoying thing from our point of view is that the allow_by_default preference takes precedence over the notify preference. (Unlike, say, geolocation, it can't wait for a callback, so we can't emulate the preference ourself.)

Strangely enough setting this preference still sets the permissions in the permission manager, although deleting them seems to have little effect because they'll just get recreated all the time, so I'm not sure whether we should enable the Manage Permissions button.

One approach might be to just have one checkbox:

  [X] Ask me before allowing websites to store data for offline use

which is the reverse of the allow_by_default pref.

Or maybe we should have a couple of radio buttons and a checkbox:
  ( ) Allow websites to store data for offline use
  (*) Customise permissions for each website [Manage Permissions]
    [X] Notify me when websites want to store data for offline use
(In reply to neil@parkwaycc.co.uk from comment #15)
> Strangely enough setting this preference still sets the permissions in the
> permission manager, although deleting them seems to have little effect
> because they'll just get recreated all the time, so I'm not sure whether we
> should enable the Manage Permissions button.

Neil, I don't think this behavior is intended (Firefox bug 933993 discusses a couple of bugs with that feature which /should/ be fixed on the Core side somewhere - however, not much recent action).

> [X] Ask me before allowing websites to store data for offline use

This is kind of what we have right now, but the label would still be insufficient as unchecking the "Ask me" box also implies "block all requests unless a site-specific permission says otherwise".
(In reply to rsx11m from comment #12)
> (In reply to comment #10)
> > Comment on attachment 8438830 [details] [diff] [review]
> > Branch patch
> > 
> > (Hmm, I get offered "Never for this site" in a private window... oops?)
> 
> Can't be me, this is just flipping the preference. I'll look if there is a
> bug pending already, otherwise see if it's easy to add as a ride-along to
> the patch with the UI change.

Actually I'm not sure what correct behaviour is for offline apps in private windows.

(In reply to rsx11m from comment #16)
> (In reply to comment #15)
> > [X] Ask me before allowing websites to store data for offline use
> 
> This is kind of what we have right now, but the label would still be
> insufficient as unchecking the "Ask me" box also implies "block all requests
> unless a site-specific permission says otherwise".

"before" was /supposed/ to imply "instead of blindly going ahead and" ;-)
(In reply to neil@parkwaycc.co.uk from comment #15)
> Or maybe we should have a couple of radio buttons and a checkbox:
>   ( ) Allow websites to store data for offline use
>   (*) Customise permissions for each website [Manage Permissions]
>     [X] Notify me when websites want to store data for offline use

If you don't like the proposed 2-checkbox solution, the 2-radiobutton+checkbox design would certainly be a good alternative and may be better to explain what the options are doing (once we know that for sure - and, it's closer to what I have right now).

Anyway, let's get the preference override landed on all channels first to be the same everywhere, then we can go on with the UI.

Push of attachment 8438830 [details] [diff] [review] for comm-central, comm-aurora, and comm-beta, please.
Keywords: checkin-needed
Whiteboard: [c-n: comm-central/aurora/beta][leave open]
(In reply to neil@parkwaycc.co.uk from comment #15)
> Strangely enough setting this preference still sets the permissions in the
> permission manager, although deleting them seems to have little effect
> because they'll just get recreated all the time,

I can't exactly reproduce this on Firefox 33.0a1, it stays at "Use default" even when the offline storage is used based on the "allow_by_default" setting, though its permissions.sqlite seems to have an entry for that site created as well. I've filed bug 1024832 on this yesterday.

(In reply to neil@parkwaycc.co.uk from 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.

(In reply to rsx11m from comment #16)
> Neil, I don't think this behavior is intended (Firefox bug 933993 discusses
> a couple of bugs with that feature which /should/ be fixed on the Core side
> somewhere - however, not much recent action).

That's actually referring to yet another issue where having set the permissions to "Block" is ignored when "allow_by_default" is in effect (bug 934457). As for disabling the "Manage Permissions" button, I'd suggest to keep that code alive until that bug and bug 1024832 are fixed. Only then it makes sense to actually modify the permissions with "allow_by_default" set.
(In reply to rsx11m from comment #19)
> (In reply to neil@parkwaycc.co.uk from 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).

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.
Posted patch Proposed UI patch (v2) (obsolete) — Splinter Review
This implements the 2nd variant of Neil's suggestion in comment #15, with two radio buttons and a checkbox for the lower one. These are more closely following the underlying preferences, consequently the JavaScript is now much less complex.

I've modified the labels a bit from your suggestion. The upper one ("allow_by_default" set) has an "all" added to make it unambiguous that it applies to all websites; the lower one ("allow_by_default" cleared) reflects more obviously that the default here is to /not/ accept the request unless the site is explicitly allowed to use the offline store. The other label changes correspond to the first patch.

I've spun off the issue of the notification bar offering "for this site" options to bug 1025569 as it implies other string changes to clarify the buttons and thus may warrant further discussion.

As a drive-by fix, I've added the two variables to the sync preferences. Firefox has at least browser.offline-apps.notify synced, thus it seems like a good idea to do the same for SeaMonkey (and it's a setting you'd likely want to be synced). However, Firefox apparently forgot about adding a sync entry for offline-apps.allow_by_default in bug 892488. Those two are clearly intertwined, thus I've added it as well.
Attachment #8438787 - Attachment is obsolete: true
Attachment #8438791 - Attachment is obsolete: true
Attachment #8438787 - Flags: ui-review?(neil)
Attachment #8438787 - Flags: review?(iann_bugzilla)
Attachment #8440357 - Flags: ui-review?(neil)
Attachment #8440357 - Flags: review?(iann_bugzilla)
Comment on attachment 8440357 [details] [diff] [review]
Proposed UI patch (v2)

>+  document.getElementById("offlineNotifyAsk").disabled = aValue;
This needs to use EnableElement because the element might have been locked.
Posted patch Part 2: Preferences UI (v3) (obsolete) — Splinter Review
(In reply to neil@parkwaycc.co.uk from comment #23)
> This needs to use EnableElement because the element might have been locked.

I figured that something looked too easy here... corrected.
Attachment #8440357 - Attachment is obsolete: true
Attachment #8440357 - Flags: ui-review?(neil)
Attachment #8440357 - Flags: review?(iann_bugzilla)
Attachment #8440383 - Flags: ui-review?(neil)
Attachment #8440383 - Flags: review?(iann_bugzilla)
Attachment #8438830 - Attachment description: Branch patch → Part 1: Flip default
Attachment #8440358 - Attachment description: Screenshot (v2) → Part 2: Screenshot (v2)
Comment on attachment 8440383 [details] [diff] [review]
Part 2: Preferences UI (v3)

>+++ b/suite/locales/en-US/chrome/common/pref/pref-offlineapps.dtd
> <!ENTITY offlineAppsListRemove.label        "Clear data…">
> <!ENTITY offlineAppsListRemove.accesskey    "d">

This is a button and as such should be capitalized "Clear Data…" instead.
I'll change that with the next patch unless someone objects.
Attachment #8438830 - Attachment description: Part 1: Flip default → Part 1: Flip default [checked in; comment #14, comment #26]
(In reply to rsx11m from comment #21)
> However, Firefox apparently forgot about adding a sync
> entry for offline-apps.allow_by_default in bug 892488.

(Quoting Dão Gottwald [:dao] from bug 1025581 comment #2)
> We usually only sync prefs that are exposed in some preferences UI
> (about:config doesn't count).

So, Firefox doesn't sync it on purpose - but, this patch adds the preference to the UI, thus it should also add it to the synced prefs by that logic.
Comment on attachment 8440383 [details] [diff] [review]
Part 2: Preferences UI (v3)

>+function DisableNotifyBox(aValue)
Nit: Other pref panes go for UpdateNotifiyBox or SetNotifyBoxEnabled.

>-      <hbox align="center">
>+      <hbox align="end">
Rather than putting the radiogroup and the button in the hbox, can you put the second radio and the button in an hbox, in a similar way to the checkbox and button is currently?

>+<!ENTITY offlineAppsUsage.label             "The following websites are using the offline storage:">
Nit: Remove the "the".

>-appCacheSizeInfo=Your application cache is currently using %1$S %2$S of disk space.
>+offlineSizeInfo=Your offline storage currently uses %1$S %2$S of disk space.
Nit: Property name looks odd, can you use offlineAppSizeInfo instead?
(In reply to neil@parkwaycc.co.uk from comment #28)
> >-      <hbox align="center">
> >+      <hbox align="end">
> Rather than putting the radiogroup and the button in the hbox, can you put
> the second radio and the button in an hbox, in a similar way to the checkbox
> and button is currently?

Actually, I did this on purpose. Depending on the desktop theme (if it matters, it seems to do so on Windows per attachment 8440358 [details]), this allows the button to extend up into the space of the first radio, thus avoiding that there is a gap between the two radios caused by any button decorations extending its height. Thus, I'd rather like to keep it as proposed in the patch.
Posted patch Part 2: Preferences UI (v4) (obsolete) — Splinter Review
All changes but the one mentioned in comment #29 addressed.
Attachment #8440383 - Attachment is obsolete: true
Attachment #8440383 - Flags: ui-review?(neil)
Attachment #8440383 - Flags: review?(iann_bugzilla)
Attachment #8444941 - Flags: ui-review?(neil)
Attachment #8444941 - Flags: review?(iann_bugzilla)
(In reply to rsx11m from comment #29)
> Actually, I did this on purpose. Depending on the desktop theme (if it
> matters, it seems to do so on Windows per attachment 8440358 [details]),
> this allows the button to extend up into the space of the first radio, thus
> avoiding that there is a gap between the two radios caused by any button
> decorations extending its height. Thus, I'd rather like to keep it as
> proposed in the patch.

This isn't very l10n-friendly, and we don't do anything similar in other panels (e.g. the Private Data pane).
Posted patch Part 2: Preferences UI (v5) (obsolete) — Splinter Review
Ok, easy change.
Attachment #8444941 - Attachment is obsolete: true
Attachment #8444941 - Flags: ui-review?(neil)
Attachment #8444941 - Flags: review?(iann_bugzilla)
Attachment #8446204 - Flags: ui-review?(neil)
Attachment #8446204 - Flags: review?(iann_bugzilla)
Comment on attachment 8446204 [details] [diff] [review]
Part 2: Preferences UI (v5)

>+      <radiogroup id="offlineDefault"
>+                  preference="offline-apps.allow_by_default"
>+                  flex="1">
This flex is wrong...

>+          <radio id="offlineExplicit"
>+                 value="false"
>+                 label="&offlineExplicit.label;"
>+                 accesskey="&offlineExplicit.accesskey;"/>
... and this is missing flex. r=me with those fixed.
Attachment #8446204 - Flags: ui-review?(neil) → ui-review+
Posted patch Part 2: Preferences UI (v6) (obsolete) — Splinter Review
Eh, missed that - fixed.
Attachment #8446204 - Attachment is obsolete: true
Attachment #8446204 - Flags: review?(iann_bugzilla)
Attachment #8446239 - Flags: ui-review+
Attachment #8446239 - Flags: review?(iann_bugzilla)
Noticing that the flex comes right after the id attribute in all other instances, let's make it consistent.
Attachment #8446239 - Attachment is obsolete: true
Attachment #8446239 - Flags: review?(iann_bugzilla)
Attachment #8446252 - Flags: ui-review+
Attachment #8446252 - Flags: review?(iann_bugzilla)
Attachment #8446252 - Flags: review?(iann_bugzilla) → review+
Thanks Ian, push for comm-central please.
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/de00ac249aea
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → seamonkey2.30
You need to log in before you can comment on or make changes to this bug.