Last Comment Bug 706876 - no "site options" in native ui menu
: no "site options" in native ui menu
Status: VERIFIED FIXED
[MTD], [QA!]
:
Product: Firefox for Android
Classification: Client Software
Component: General (show other bugs)
: unspecified
: ARM Android
: P1 normal (vote)
: ---
Assigned To: :Margaret Leibovic
:
Mentors:
Depends on: 712588
Blocks: 711772 711773 711774 711993
  Show dependency treegraph
 
Reported: 2011-12-01 10:01 PST by Brion Vibber
Modified: 2012-01-09 11:53 PST (History)
12 users (show)
camelia.urian: in‑litmus+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
11+


Attachments
sketch of UI flow for Clear Site Settings (87.18 KB, application/pdf)
2011-12-13 14:19 PST, Madhava Enros [:madhava]
no flags Details
WIP (30.01 KB, patch)
2011-12-15 16:52 PST, :Margaret Leibovic
mark.finkle: feedback+
Details | Diff | Review
patch (23.83 KB, patch)
2011-12-17 13:34 PST, :Margaret Leibovic
no flags Details | Diff | Review

Description Brion Vibber 2011-12-01 10:01:23 PST
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?) 2011-12-01 15:22:01 PST
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?) 2011-12-01 15:25:01 PST
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?) 2011-12-08 09:59:20 PST
Madhava, we need UX on this
Comment 4 Madhava Enros [:madhava] 2011-12-13 14:19:03 PST
Created attachment 581431 [details]
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 2011-12-13 14:22:48 PST
I would be very happy with what's proposed in that pdf.
Comment 6 :Margaret Leibovic 2011-12-13 14:24:25 PST
I'll implement this.
Comment 7 :Margaret Leibovic 2011-12-13 15:57:50 PST
Madhava, did you mean to assign yourself to this?
Comment 8 Madhava Enros [:madhava] 2011-12-14 09:15:43 PST
Accidentally - I will gladly give it back.
Comment 9 :Margaret Leibovic 2011-12-15 16:52:18 PST
Created attachment 582144 [details] [diff] [review]
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?) 2011-12-16 09:00:51 PST
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 2011-12-17 13:34:22 PST
Created attachment 582589 [details] [diff] [review]
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 2011-12-17 14:01:17 PST
Pushed to inbound (with email r+ from mfinkle):

https://hg.mozilla.org/integration/mozilla-inbound/rev/f4c27f53131d
Comment 13 Matt Brubeck (:mbrubeck) 2011-12-18 15:33:38 PST
https://hg.mozilla.org/mozilla-central/rev/f4c27f53131d
Comment 14 Brion Vibber 2011-12-19 10:17:04 PST
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 2011-12-19 10:28:47 PST
(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 2011-12-19 14:20:55 PST
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 2011-12-20 03:17:17 PST
For Clearing password: testcase: https://litmus.mozilla.org/show_test.cgi?id=33831 was updated.
Comment 18 Mark Finkle (:mfinkle) (use needinfo?) 2011-12-20 05:35:38 PST
(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 2011-12-20 06:40:46 PST
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 2011-12-20 07:38:09 PST
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?) 2011-12-20 07:43:41 PST
(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 2011-12-21 02:47:53 PST
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.

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