The default bug view has changed. See this FAQ.

no "site options" in native ui menu

VERIFIED FIXED

Status

()

Firefox for Android
General
P1
normal
VERIFIED FIXED
5 years ago
8 months ago

People

(Reporter: Brion Vibber, Assigned: Margaret)

Tracking

unspecified
ARM
Android
Points:
---
Dependency tree / graph
Bug Flags:
in-litmus +

Firefox Tracking Flags

(firefox11 fixed, fennec11+)

Details

(Whiteboard: [MTD], [QA!])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

5 years ago
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

Updated

5 years ago
Whiteboard: [MTD]
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".
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]
...
-------------------------
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Linux → Android
Hardware: Other → ARM
Madhava, we need UX on this
Assignee: nobody → madhava
Keywords: uiwanted
Priority: -- → P1
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.
(Reporter)

Comment 5

5 years ago
I would be very happy with what's proposed in that pdf.
(Assignee)

Comment 6

5 years ago
I'll implement this.
Assignee: madhava → margaret.leibovic
Assignee: margaret.leibovic → madhava
Keywords: uiwanted
(Assignee)

Comment 7

5 years ago
Madhava, did you mean to assign yourself to this?
Accidentally - I will gladly give it back.
Assignee: madhava → margaret.leibovic
(Assignee)

Comment 9

5 years ago
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.
Attachment #582144 - Flags: feedback?(sriram)
Attachment #582144 - Flags: feedback?(mark.finkle)
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?)
Attachment #582144 - Flags: feedback?(mark.finkle) → feedback+
(Assignee)

Comment 11

5 years ago
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.
Attachment #582144 - Attachment is obsolete: true
Attachment #582144 - Flags: feedback?(sriram)
(Assignee)

Updated

5 years ago
Blocks: 711772
(Assignee)

Updated

5 years ago
Blocks: 711773
(Assignee)

Updated

5 years ago
Blocks: 711774
(Assignee)

Comment 12

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

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

Updated

5 years ago
Flags: in-litmus?(carla.nadastean)
Whiteboard: [MTD] → [MTD], [QA+]
https://hg.mozilla.org/mozilla-central/rev/f4c27f53131d
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
(Reporter)

Comment 14

5 years ago
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"
(Assignee)

Comment 15

5 years ago
(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.
(Assignee)

Updated

5 years ago
Blocks: 711993
(Reporter)

Comment 16

5 years ago
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

5 years ago
For Clearing password: testcase: https://litmus.mozilla.org/show_test.cgi?id=33831 was updated.
(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

5 years ago
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
Flags: in-litmus?(carla.nadastean) → in-litmus+

Comment 20

5 years ago
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Comment 22

5 years ago
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.
Status: RESOLVED → VERIFIED
Whiteboard: [MTD], [QA+] → [MTD], [QA!]
Attachment #581431 - Attachment mime type: application/msdownload → application/pdf
Depends on: 712588
tracking-fennec: --- → 11+
status-firefox11: --- → fixed
You need to log in before you can comment on or make changes to this bug.