1044401 - Update site settings dialog to be less confusing

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Settings and Preferences, defect)

Description

[Praveen Sridhar]

Attachment: Screenshot_2014-07-26-12-45-29.png

User Agent: Mozilla/5.0 (X11; Ubuntu; Linux i686; rv:31.0) Gecko/20100101 Firefox/31.0 (Beta/Release)
Build ID: 20140715214335

Steps to reproduce:

Select  Menu (upper right corner of the screen) --> Edit site settings


Actual results:

The site settings page shows 'There are no settings to clear'


Expected results:

The page should have shown the settings that are available to edit. The present string suggests that there are no settings to be edited and that settings can only be cleared.

Comment 1

[Praveen Sridhar]

Attachment: Screenshot for the Menu.

Comment 2

[Aaron Train [:aaronmt]]

This sounds like bug 711774 in that if the menu item serves no purpose for the current context, then the item should be disabled/removed.

Comment 3

[:Margaret Leibovic]

The "Edit site settings" menu item was introduced as a last-minute quick fix for the Fennec native re-write and has hung around ever since. Since that time this menu item as also moved locations.

At the very least, I think we should rename this item to "Site settings", or maybe just "Site permissions". But maybe we should rethink how we display site permissions in general.

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: screenshot of contradictory-looking "share location" setting in this dialog

One other thing that should be cleaned up, if/when this dialog is rewritten: it looks confusing/contradictory if I've *denied* a site permission.

e.g. right now, if I deny a location request, and then go to "Edit Site Settings", the UI says:
  
>  Share Location              [x]
>   Don't Share

At first glance, this seems to contradict itself about whether or not I'm sharing my location. On the one hand, it says "Don't share" -- BUT it also has a ticked checkbox next to "Share Location", which gives the opposite impression.

(I think the checkbox is _really_ to enable clearing of the setting, if I were to hit the "clear" button. The up-front UI experience, though, makes it looks like it's to enable/disable "Share Location")

(I was going to file a bug on this, but given comment 3, it sounds like it may belong as part of this bug.)

Comment 5

[:Margaret Leibovic]

Yeah, this dialog definitely is pretty confusing.

Robin, do you want to take a look at this to see if there's anything we can do to make this more straightforward?

We want to improve our settings UI in the Firefox 35 cycle (bug 965377), and although this isn't exactly part of that, it could be good to group these fixes together.

Comment 6

[Robin Andersen [:tecgirl]]

OK, yes, two things here.

1) Yes, Aaron is right, if no action/data is available, disable the menu item.

2) Having a check denotes ON/OFF, YES/NO. Using a check and then having subtext that says the choice is negative is confusing. The check should mean "Yes, do this thing" (share location). So we should reverse what's happening here. Remove the subtext and make the checkbox select share location true.

Comment 7

[:Margaret Leibovic]

I'm going to make this part of the settings effort we're tracking in bug 1063114.

Comment 8

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

NI-ing Margaret

It seems like this is about improving the dialog for "Edit site settings".

Do you think this should still be a part of the Settings re-org? It feels like it should be something separate, perhaps related to Control center or permissions control for something like notifications.

Comment 9

[:Margaret Leibovic]

(In reply to Anthony Lam (:antlam) from comment #8)
> NI-ing Margaret
> 
> It seems like this is about improving the dialog for "Edit site settings".
> 
> Do you think this should still be a part of the Settings re-org? It feels
> like it should be something separate, perhaps related to Control center or
> permissions control for something like notifications.

Yeah, I think is makes more sense to break this into its own item. Past Margaret was wrong!

Comment 10

[Chenxia Liu [:liuche]]

Hey Anthony, what should the UX for this be? e.g., change "Clear" to "Save" and reset (clear) the state of unchecked items.

Comment 12

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Looking at the guidelines from google around these types of components, I think we need to address two things:

1) The language is confusing, there's something even more confusing about seeing a check-box with a negative action like "clearing". This seems like a harder fix than a simple language change since it's inherently confusing.

2) Using a dialog doesn't seem to be the best decision UX here

Perhaps we should go with a full page "dialog" [1]. It fits with the design guidelines, and it opens up the possibility for us to do more (our current dialog is not great if we want to add more Control Center functionality). It also has the added benefit of being more obvious that these changes are not saved in real time.

Since it doesn't look like this is being tracked as a part of a larger initiative, I'm going to NI Barbara here to possibly add an Aha card for this to get prioritized/tracked. 

I do think that this is valuable especially in the "Control center" topic. But I'll need to spend some time thinking about this new full-page dialog to see what else makes sense to include there. 

Chrome does a decent job with theirs by including things like permissions and usage stats.

[1] https://www.google.com/design/spec/components/dialogs.html#dialogs-full-screen-dialogs

Comment 13

[:Margaret Leibovic]

(In reply to Anthony Lam (:antlam) from comment #12)

> Since it doesn't look like this is being tracked as a part of a larger
> initiative, I'm going to NI Barbara here to possibly add an Aha card for
> this to get prioritized/tracked. 

We do have an Aha card for this:
https://mozilla.aha.io/features/FENN-76

Comment 14

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to :Margaret Leibovic from comment #13)
> (In reply to Anthony Lam (:antlam) from comment #12)
> 
> > Since it doesn't look like this is being tracked as a part of a larger
> > initiative, I'm going to NI Barbara here to possibly add an Aha card for
> > this to get prioritized/tracked. 
> 
> We do have an Aha card for this:
> https://mozilla.aha.io/features/FENN-76

My mistake! It looks like I even commented on that too!

(leaving NI on for context anyways)

I also just talked to Chenxia about this. We both do think that the long term solution would be a full page dialog but in the interim (and for this bug) we can probably still do something quick. So, this is what we're proposing.

+----------------------------------+
|                                  |
|  Edit Site Settings              |
|                                  |
+----------------------------------+
|                                  |
|  Location                   +-+  |
|  Don't share                +-+  |
|                                  |
|                                  |
|  Camera                     +-+  |
|  Share                      +-+  |
|                                  |
|                                  |
+----------------------------------+
|                                  |
|                Cancel     Clear  |
|                                  |
+----------------------------------+

Comment 15

[:Margaret Leibovic]

Isn't the choice between "Cancel" and "Clear" one of the main points of confusion? I feel like we should change this around to be "Cancel" and "Save", and update the checkbox states accordingly.

Comment 16

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to :Margaret Leibovic from comment #15)
> Isn't the choice between "Cancel" and "Clear" one of the main points of
> confusion? I feel like we should change this around to be "Cancel" and
> "Save", and update the checkbox states accordingly.

It is, and our second option was "Reset". What do you think about that?

Chenxia and myself took a look at Chrome's UI where they use "Clear & Reset" as the action.

Comment 17

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1044401 - Update site settings dialog to be less confusing. r=sebastian

Bug 1044401 - Update site settings dialog to be less confusing. r=sebastian

Comment 18

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1044401 - Remove title from dialog. r=sebastian

Bug 1044401 - Remove title from dialog. r=sebastian

Comment 19

[Chenxia Liu [:liuche]]

Attachment: MozReview Request: Bug 1044401 - Turn permissions into nouns. r=sebastian

Bug 1044401 - Turn permissions into nouns. r=sebastian

Comment 20

[Chenxia Liu [:liuche]]

Attachment: Screenshot: Patch changes to Site Settings dialog

This implements the changes from antlam:
1) Uncheck everything by default, and only enable the "Clear" button if an item has been selected
2) Update the permissions strings to be nouns, rather than verbs (where checking the item could cause confusion as if checking the box was enabling the action)
3) Remove the url of the page, because it's redundant

Comment 21

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8688760 [details]
MozReview Request: Bug 1044401 - Update site settings dialog to be less confusing. r=sebastian

https://reviewboard.mozilla.org/r/25405/#review22967

::: mobile/android/base/GeckoApp.java:835
(Diff revision 1)
> -                        listView.setItemChecked(i, true);
> +                        listView.setItemChecked(i, false);

If we previously set everything to be checked=true as default - do we need to set it to false? Isn't this the default already?

Comment 22

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Comment on attachment 8688765 [details]
Screenshot: Patch changes to Site Settings dialog

Nice! This is a lot more obvious!

Comment 23

[Chenxia Liu [:liuche]]

https://reviewboard.mozilla.org/r/25405/#review22967

> If we previously set everything to be checked=true as default - do we need to set it to false? Isn't this the default already?

Good call! I wasn't even thinking about that, but the default is in fact "false" so I removed these two lines.

I wanted to check about the other two patches in this series - if you haven't got to them that's fine! but I also wanted to make sure that you hadn't reviewed them but just not posted them due to reviewboard or something.

Comment 24

[Chenxia Liu [:liuche]]

NI - I'm still seeing r? on the other parts of the review request. Just checking that you didn't review all of them and just forget/got misled by reviewboard about publishing.

Comment 25

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8688761 [details]
MozReview Request: Bug 1044401 - Remove title from dialog. r=sebastian

https://reviewboard.mozilla.org/r/25407/#review23205

Comment 26

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8688762 [details]
MozReview Request: Bug 1044401 - Turn permissions into nouns. r=sebastian

https://reviewboard.mozilla.org/r/25409/#review23207

Comment 27

[Sebastian Kaspari (:sebastian; :pocmo)]

Done :)

Comment 28

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/8781a96ad0e2
https://hg.mozilla.org/integration/fx-team/rev/49476fcf5279
https://hg.mozilla.org/integration/fx-team/rev/6fa0eebc7019

Comment 29

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8781a96ad0e2
https://hg.mozilla.org/mozilla-central/rev/49476fcf5279
https://hg.mozilla.org/mozilla-central/rev/6fa0eebc7019

Comment 30

[Francesco Lodolo [:flod]]

http://hg.mozilla.org/mozilla-central/rev/6fa0eebc7019#l2.37

You updated the string ID everywhere (thanks) but not in the .properties file.

-geolocation.shareLocation=Share Location
+geolocation.shareLocation=Location

I'd expect Fennec to be quite upset or at least get an empty value

Comment 31

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Francesco Lodolo [:flod] from comment #30)
> http://hg.mozilla.org/mozilla-central/rev/6fa0eebc7019#l2.37
> 
> You updated the string ID everywhere (thanks) but not in the .properties
> file.
> 
> -geolocation.shareLocation=Share Location
> +geolocation.shareLocation=Location
> 
> I'd expect Fennec to be quite upset or at least get an empty value

This needinfo should go to Chenxia, I guess.

Comment 32

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5f6ea7bb1641

Comment 33

[Chenxia Liu [:liuche]]

Pushed a change, thanks for the heads up flod!

Comment 34

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5f6ea7bb1641