706876 - no "site options" in native ui menu

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Brion Vibber]

User Agent: Mozilla/5.0 (Android; Linux armv7l; rv:11.0a1) Gecko/20111201 Firefox/11.0a1 Fennec/11.0a1
Build ID: 20111201040252

Steps to reproduce:

attempting to reset location prefs for nextbus.com to test; no "site options" menu item is available.


Actual results:

no site options - nowhere to reset perms


Expected results:

should be a "site options" item in the menu which brings up the callout to reset location & data prefs

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Madhava, Ian: Thoughts on where this should go? We used Larry in XUL Fennec, but I don't think we should do that now. A menu would be fine, but we don't show/hide menus. We enable/disable so the order is not messed up. Some action bars will flow menuitems onto the actionbar.

For text, I like "Clear Site Settings" instead of "Clear Site Prefs".

Comment 2

[Mark Finkle (:mfinkle) (use needinfo?)]

Building on Brion's idea, we could have a "Site Settings" menu that displayed a dialog with a list of settings and allowed the user to clear the ones that are set:

-------------------------
Popup blocker     [Clear]
Location          not set
Password         [Forget]
...
-------------------------

Comment 3

[Brad Lassey [:blassey] (use needinfo?)]

Madhava, we need UX on this

Comment 4

[Madhava Enros [:madhava]]

Attachment: sketch of UI flow for Clear Site Settings

I agree we don't want this in Larry any more.

The bug is really about allowing clearing of site settings. Please see the attached UI flow.

Comment 5

[Brion Vibber]

I would be very happy with what's proposed in that pdf.

Comment 6

[:Margaret Leibovic]

I'll implement this.

Comment 7

[:Margaret Leibovic]

Madhava, did you mean to assign yourself to this?

Comment 8

[Madhava Enros [:madhava]]

Accidentally - I will gladly give it back.

Comment 9

[:Margaret Leibovic]

Attachment: WIP

I got this to work with the single-line UI provided by setMultiChoiceItems so that I could develop the gecko side of things. The code I've been working on to try to get the two-line UI is in there, but I commented it out in showSiteSettingsDialog. I'm not sure how much we want to block on this custom UI, so I wanted to get this patch out for some feedback.

I got to the point where I have the UI I want showing without crashing, but when I click on the "Clear" button, the listView.getCheckedItemPositions() isn't finding any checked items. I was hoping implementing that CheckableLinerLayout method would help with this, but the methods in there don't even seem to be called (before I was just using a regular LinearLayout there).

I also need to make a custom title to get a two-line UI for that, but hopefully that won't be as bad because there's a method for that. I just need to make sure I get the right theming on the custom XML I use (Sriram, I bet you could help me here).

I also still need to disable the menuitem if there are no permissions set on the site. I'll have to change around where I pass my gecko/java messages for this, since I need to know about the permissions when the menu is shown, not when the item is clicked.

Comment 10

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 582144 [details] [diff] [review]
WIP


>+    private void showSiteSettingsDialog(String aHost, JSONArray aPermissions) {
>+        final AlertDialog.Builder builder = new AlertDialog.Builder(this);
>+
>+        //XXX use setCustomTitle to get two-line title with domain name
>+        builder.setTitle("Clear Site Settings: " + aHost);;

* two ;;
* l10n for the string

>+        //XXX this isn't working right now because listView.getCheckedItemPositions
>+        // isn't returning the checked items
>+        /*ArrayList <HashMap<String, String>> itemList = new ArrayList <HashMap<String, String>>();
>+        for (int i = 0; i < aPermissions.length(); i++) {
>+            try {
>+                JSONObject permObj = aPermissions.getJSONObject(i);
>+                HashMap<String, String> map = new HashMap<String, String>();
>+                map.put("permission_label", permObj.getString("label"));
>+                map.put("permission_value", permObj.getString("value"));
>+                itemList.add(map);
>+            } catch (JSONException e) {
>+                Log.i(LOGTAG, "JSONException: " + e);
>+            }
>+        }
>+        builder.setAdapter(new SimpleAdapter(
>+            GeckoApp.this,
>+            itemList,
>+            R.layout.site_setting_item,
>+            new String[] { "permission_label", "permission_value" },
>+            new int[] { R.id.permission_label, R.id.permission_value }
>+        ), new DialogInterface.OnClickListener() {
>+            public void onClick(DialogInterface dialog, int id) {
>+                // Do nothing
>+            }
>+        });*/

This is the preferred approach, rihgt? Once the getCheckedItemPositions issues is resolved.

>+                String label = permObj.getString("label");
>+                String value = permObj.getString("value");
>+                //XXX This probably isn't l10n friendly
>+                items[i] = label + ": " + value;

I'm OK with this is we intend to get the above approach working

>+        builder.setCancelable(true);
>+        builder.setNegativeButton("Cancel", new DialogInterface.OnClickListener(){

l10n

>+        builder.setPositiveButton("Clear", new DialogInterface.OnClickListener() {

l10n

>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>+var PermissionsHelper = {
>+
>+  _permissonTypes: ["password", "geo", "popup", "indexedDB", "offline-app"],

I think we need "notification" in here too, for showing web notifications

>+  _permissionStrings: {
>+    "password": {
>+      label: "password.rememberPassword",
>+      allow: "password.remember",
>+      deny: "password.never"
>+    },


the "allow" and "deny" names here are not for actually setting the preferences, right? They are only for picking the right string to use in the UI dialog?

If so, it might be better to use "allowed" and "denied" so it won't be as easy to confuse the intent. These values are for showing what the current setting is, not for changing it. (did I get that right?)

Comment 11

[:Margaret Leibovic]

Attachment: patch

Bugzilla was down when I tried to upload this earlier, but I emailed it to mfinkle and here are his review comments, which I'm going to work on addressing now.

+          permissions.push({
+            type: type,
+            settingString: settingString
+          });
+        }

Let's just call the JSON name "setting" for now. When we switch to two separate strings we can use "setting" and "value"

+        // Keep track of permissions, so we know which ones to clear
+        this._currentPermissions = permissions;

When we switch to the two-line UI we might be able to use the Java setTag()/getTag() to store the string version of the "type" with the row and then we wouldn't need to save the currentPermissions or be tied to an indexed-based way of associating the permissions. Come to think of it, we might be able to do that with the single-line UI, but let's do this in a followup in any case.

+    /**
+     * @param aPermissions
+     *           Array of JSON objects to represent site permissions.
+     *           Example: { type: "offline-app", settingString: "Store Offline Data: Allow" }
+     */
+    private void showSiteSettingsDialog(String aHost, JSONArray aPermissions) {
+        final AlertDialog.Builder builder = new AlertDialog.Builder(this);
+
+        View customTitleView = getLayoutInflater().inflate(R.layout.site_setting_title, null);
+        ((TextView) customTitleView.findViewById(R.id.title)).setText(R.string.site_settings_title);
+        ((TextView) customTitleView.findViewById(R.id.host)).setText(aHost);       
+        builder.setCustomTitle(customTitleView);
+
+        // If there are no permissions to clear, show the user a message about that.
+        // In the future, we want to disable the menu item if there are no permissions to clear.
+        if (aPermissions.length() == 0) {
+            builder.setMessage(R.string.site_settings_no_settings);
+        } else {
+            // Eventually we should use a list adapter and custom checkable list items
+            // to make a two-line UI to match the mock-ups
+            CharSequence[] items = new CharSequence[aPermissions.length()];
+            boolean[] states = new boolean[aPermissions.length()];
+            for (int i = 0; i < aPermissions.length(); i++) {
+                try {
+                    items[i] = aPermissions.getJSONObject(i).
+                                            getString("settingString");
+                    // Make all the items checked by default
+                    states[i] = true;
+                } catch (JSONException e) {
+                    Log.i(LOGTAG, "JSONException: " + e);
+                }
+            }
+            builder.setMultiChoiceItems(items, states, new DialogInterface.OnMultiChoiceClickListener(){
+                public void onClick(DialogInterface dialog, int item, boolean state) {
+                    // Do nothing
+                }
+            });
+            builder.setPositiveButton(R.string.site_settings_clear, new DialogInterface.OnClickListener() {
+                public void onClick(DialogInterface dialog, int id) {
+                    ListView listView = ((AlertDialog) dialog).getListView();
+                    SparseBooleanArray checkedItemPositions = listView.getCheckedItemPositions();
+   
+                    // An array of the indices of the permissions we want to clear
+                    JSONArray permissionsToClear = new JSONArray();
+                    for (int i = 0; i < checkedItemPositions.size(); i++) {
+                        boolean checked = checkedItemPositions.get(i);
+                        if (checked)
+                            permissionsToClear.put(i);
+                    }
+                    GeckoAppShell.sendEventToGecko(new GeckoEvent("Permissions:Clear", permissionsToClear.toString()));
+                }
+            });
+        }
+
+        builder.setNegativeButton(R.string.site_settings_cancel, new DialogInterface.OnClickListener(){
+            public void onClick(DialogInterface dialog, int id) {
+                dialog.cancel();
+            }           
+        });
+
+        mMainHandler.post(new Runnable() {
+            public void run() {
+                builder.create().show();
+            }
+        });
+    }
+

This code looks fine, but it has a lot of TABs in the indents. Just make sure to clean those up before pushing.

Comment 12

[:Margaret Leibovic]

Pushed to inbound (with email r+ from mfinkle):

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4c27f53131d

Comment 13

[Matt Brubeck (:mbrubeck)]

https://hg.mozilla.org/mozilla-central/rev/f4c27f53131d

Comment 14

[Brion Vibber]

Testing 20111219 nightly on Galaxy Nexus (Android 4.0.2) I see the dialog anf it seeems to work for appcache, but not for location sharing permission.

* go to http://html5demos.com/geo
* allow location sharing
* menu / clear site settings
* dialog says "there are no settings to clear"

Comment 15

[:Margaret Leibovic]

(In reply to Brion Vibber from comment #14)
> Testing 20111219 nightly on Galaxy Nexus (Android 4.0.2) I see the dialog
> anf it seeems to work for appcache, but not for location sharing permission.
> 
> * go to http://html5demos.com/geo
> * allow location sharing
> * menu / clear site settings
> * dialog says "there are no settings to clear"

This is expected - for geolocation we aren't setting persistent permissions (there is no "Always Allow" or "Never Allow" option). I think we may set an "Always Allow" if you allow it 3 times or something like that, but I could be wrong about that.

If you want a site to test, you could use popuptest.com, or try saving your password on a login form.

Comment 16

[Brion Vibber]

Can confirm that the appcache ('store offline data') prompt & its clear prompt work on <http://www.nextbus.com/>. If we're not saving a permanent exception for location then no surprise it's missing after all. :D

Comment 17

[Camelia Urian]

For Clearing password: testcase: https://litmus.mozilla.org/show_test.cgi?id=33831 was updated.

Comment 18

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Brion Vibber from comment #16)
> Can confirm that the appcache ('store offline data') prompt & its clear
> prompt work on <http://www.nextbus.com/>. If we're not saving a permanent
> exception for location then no surprise it's missing after all. :D

We save a permanent permission for location if you make the same choice 5 times for a domain. For example, if you pick "Show" 5 times for foo.com, we save a permanent permission and won't prompt for foo.com again, unless you clear it.

The same is true for web notifications.

Comment 19

[Camelia Urian]

Test for clearing pop-ups updated: https://litmus.mozilla.org/show_test.cgi?id=33864
Test for clearing offline data storage: https://litmus.mozilla.org/show_test.cgi?id=33671

Also created test for clear site settings disabled: https://litmus.mozilla.org/show_test.cgi?id=40495

Comment 20

[Camelia Urian]

Margret, Mark there is no option to clear location from the Clear Site Settings Option from Menu->More

 Steps to reproduce:
1. Go to html5demos.com/geo
2. On the door hanger select "Share".
3. Repeat steps 1 and 2: 5 times. ->After the 5th time door hanger is not displayed, permanent permission was saved.
4. Go to Menu->More->Clear Site Settings
Results: Prompt says "There are no settings to clear".

Since a permanent permission was saved for this site, should we have to option to clear from Clear Site Settings?

Passwords, data storage and popups can be cleared from the Clear Site Settings option.

I will  mark the bug as REOPENED for location issue. If this is a different issue, I will log a new bug and close this one.

Comment 21

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to Camelia Urian from comment #20)

> Passwords, data storage and popups can be cleared from the Clear Site
> Settings option.
> 
> I will  mark the bug as REOPENED for location issue. If this is a different
> issue, I will log a new bug and close this one.

Let's file a new bug for location only. Make sure you "Share" your location more than 5 times on a website so the "Do you want to share your location" prompt is no longer shown.

Comment 22

[Camelia Urian]

Build ID: Mozilla/5.0 (Android; Linux armv7l; rv:11.0a1) Gecko/20111220 Firefox/11.0a1 Fennec/11.0a1
Device: Samsung Nexus S
OS: Android 2.3.6

Bug #712588 was logged for location issue from comment 20.

Marking this bug as Verified.