938177 - [User Story] Browser Settings Moved to System Settings

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Settings, defect)

Description

[Peter Dolanjski [:pdol]]

User Story:
As a user I want my Browser Settings moved into the System Settings since the current Browser settings generally apply to the OS, not just the Browser app. 

Acceptance Criteria:
1. The Browser app no longer displays an icon to access settings.
2. The System Settings now displays the items that were formerly within the Browser settings.

Assumptions:
UX to define how System Settings will look.

Comment 1

[Ben Francis [:benfrancis]]

Over to the Settings team to figure out where these will live.

Let's talk about how we communicate between the settings app and system app though, because clearing browser history (stored in a DataStore in the system app) may be tricky.

Comment 3

[Stephany Wilkes]

Francis Djabri and I spoke about this quickly. It was our impression (in line with this bug) that System Browser Settings would not be their own, but would move into generic Settings. So +1 to that, for the record.

"Clear browsing history" and "Clear cookies and stored data" can fit under Privacy.

Please re-flag us if any additional UX is needed here. We're happy to do these as quickly as possible.

Comment 4

[Bruce Huang [:bhuang] (inactive)]

(In reply to Stephany Wilkes from comment #3)
> "Clear browsing history" and "Clear cookies and stored data" can fit under
> Privacy.

Will it go in the "Privacy & Security" division in settings, under "Do Not Track"?  I think we'll need an icon and appropriate wording since the item will navigate to a page with the 2 buttons that either clear history or clear cookies.

Comment 5

[Stephany Wilkes]

Flagging Francis to ensure this ends up in the correct spot. Francis, please flag Peter if an icon is needed. Thanks!

Comment 6

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

https://moztrap.mozilla.org/manage/caseversion/105635/ created

Comment 7

[Arthur Chen [:arthurcc][inactive after 6/30]]

It seems we only have two options for communicating between the system app and settings app: mozSettings and IAC. Ben, have the related API been moved to the system app already?

Comment 8

[Ben Francis [:benfrancis]]

There is currently no code in the system app to clear private data or browser history.

To clear private data we need to call clearBrowserData() on the app object of the system app. This will effect browser windows and everything.me apps. I'm not sure if it will have an effect on other apps which have the mozapp attribute set and I'm not sure what permissions you need to call this, and therefore whether the settings app could do it or if the system app has to call it itself.

To clear browser history we will need to clear visits, places and icons from the places DataStore. We may need to be careful not to delete place and icon data for bookmarked places if they are stored centrally in the places DataStore, but as far as I know they're not yet.

It's possible we want some code in the browser app to do both of these, and we should listen for a change in a setting via the Settings API to trigger this code, so that it can be triggered from the Settings app.

Dale, how do you think we should approach this?

Comment 9

[Dale Harvey (:daleharvey)]

Yeh taking a look at the system app theres a whole bunch of things triggered via settings listeners, the system app will always be running so I dont think its a problem

Comment 10

[Omega Feng [:Omega] [:馮於懋]]

Attachment: [1.5 Settings] Browsing Settings v1.0.pdf

Here is the UX proposal.

Comment 11

[Arthur Chen [:arthurcc][inactive after 6/30]]

Hi Peter, I was wondering is the system frontend team able to handle this item as it is actually about moving the logic from browser to system?

Comment 12

[Francis Djabri [:djabber]]

For 2.1, the UX should follow the proposal set out by Omega, but I'd suggest that we change the top-level heading in the Settings app to "Browsing" and not "Browsing History", as not all the settings are related to the history. 

@Jenny - could you confirm that you're ok with this proposal, or if there are any other 2.1 changes that would impact it?

@Eric, could you provide an icon for the top level header?

Comment 13

[Eric Pang [:epang] UX]

(In reply to Francis Djabri [:djabber] from comment #12)
> For 2.1, the UX should follow the proposal set out by Omega, but I'd suggest
> that we change the top-level heading in the Settings app to "Browsing" and
> not "Browsing History", as not all the settings are related to the history. 
> 
> @Jenny - could you confirm that you're ok with this proposal, or if there
> are any other 2.1 changes that would impact it?
> 
> @Eric, could you provide an icon for the top level header?

Hi Carol, can you help out with creating the browsing icon mentioned above for settings?
Once ready Peter should be able to review.  Please let me know, thanks!

Comment 14

[Carol Huang [:Carol]]

hey Eric,
Helen is taking care of Settings right now.

Hi Helen,
Could you help on this browsing icon in Settings? Thank you!

Comment 15

[Helen Huang [:HHuang]]

Sure. I will create a browsing icon and upload it once I done.

Comment 16

[Jenny Lee]

(In reply to Francis Djabri [:djabber] from comment #12)

> @Jenny - could you confirm that you're ok with this proposal, or if there
> are any other 2.1 changes that would impact it?

Yes let's make the change!

Comment 17

[Francis Djabri [:djabber]]

(In reply to Jenny Lee from comment #16)
> (In reply to Francis Djabri [:djabber] from comment #12)
> 
> > @Jenny - could you confirm that you're ok with this proposal, or if there
> > are any other 2.1 changes that would impact it?
> 
> Yes let's make the change!

@Jenny, great, would you mind updating the spec and re-posting it here? Thanks!

@Dale, in the current "App permissions" setting, there's a "Clear bookmarks data" button at the end of the list, which seems to clear private data from sites added to home screen. It seems like we could merge this with the "Clear cookies and stored data" setting, as this has now been taken out of the browser and into the system settings. What do you think?

Comment 18

[James Hobin [:hobinjk]]

I have a few questions about this before I start actually moving things around. The focus of this bug seems to be moving the Privacy & Security options (Clear history and Clear data). However, the browser also has the default search engine preference. While I am sure that the more polemic of us would argue that choosing Google over Duck Duck Go is a Privacy & Security preference, there is probably a more user-friendly area for this to be placed.

tl;dr: Where does the default search engine preference go?

Comment 19

[Dale Harvey (:daleharvey)]

> in the current "App permissions" setting, there's a "Clear bookmarks data" button at the end of the list, which seems to clear private data from sites added to home screen. It seems like we could merge this with the "Clear cookies and stored data" setting, as this has now been taken out of the browser and into the system settings. What do you think?

Yup, I actually didnt realise that existed and certainly makes more sense merged with the new system settings

> tl;dr: Where does the default search engine preference go?

They are currently under Homescreens, not sure if thats the final location according to spec

Comment 20

[Gregor Wagner [:gwagner]]

Francis should have an answer here.

Comment 21

[Francis Djabri [:djabber]]

> 
> > tl;dr: Where does the default search engine preference go?
> 
> They are currently under Homescreens, not sure if thats the final location
> according to spec

It doesn't really make sense to have it under Homescreen anymore since search is accessible from anywhere. I'd recommend that we create a new top-level category called "Search", which can be found under the "Personalization" sub-heading. 

Please see the latest Search app spec at https://mozilla.box.com/s/lbw2wzw3p4jvxs24k4sg to see what should be contained in these settings. 

@Jenny - could you let James know where the search heading should appear in the list? 

Flagging @Helen to provide a new icon for the top level heading.

Comment 22

[Jenny Lee]

Attachment: [2.1 Settings] Browsing Settings v1.1.pdf

Hi all, please take a look at the latest Browsing spec. Thank you

Comment 23

[Jenny Lee]

(In reply to Francis Djabri [:djabber] from comment #21) 
> @Jenny - could you let James know where the search heading should appear in
> the list? 

"Search" should be under Personalization section, in the order:
Sound
Display
Homescreen
>>>Search<<<
Notifications
Navigation
Date & Time
Language
Keyboards

Thanks!

Comment 24

[jhobin]

Attachment: Gaia pull request

I still need to strip out the modifications to the Browser app, even though AFAIK the Browser app is about to go away.

Otherwise, this patch implements and tests System Browser settings for clearing data while moving the search engine preference.

Comment 25

[jhobin]

(In reply to Helen Huang from comment #15)
> Sure. I will create a browsing icon and upload it once I done.

What is the status of icons for the Search and Browser Privacy settings menus?

Comment 26

[Gregor Wagner [:gwagner]]

Eric, do we have assets for this somewhere?

Comment 27

[Kevin Grandon :kgrandon]

Comment on attachment 8472022 [details] [review]
Gaia pull request

Hey James. This is starting to look great! I left one comment about the bootstrap.js listener design - I think we should move that out into browser_setttings before taking another pass.

I'm still not entirely sure why we need the changes to browser, let's have a chat this week to figure out if that's really necessary.

We'll need reviews from a few system/settings peers probably. If you flag me for review again after making some changes I'll forward it to the right people. Thanks!

Comment 28

[Eric Pang [:epang] UX]

(In reply to Gregor Wagner [:gwagner] from comment #26)
> Eric, do we have assets for this somewhere?

Hi Gregor, Helen is working on the icon and will attached to the bug once ready.
Helen can you give us an update on the progress? thx!

Comment 29

[Arthur Chen [:arthurcc][inactive after 6/30]]

Hi James, thanks for the work! Note that in settings app we are now using AMD to define modules, so newly added scripts should be wrapped as AMD modules. Details please refer to https://github.com/mozilla-b2g/gaia/tree/master/apps/settings.

Comment 30

[Helen Huang [:HHuang]]

Attachment: Browsing.svg

The Browsing icon is done, I've attached the SVG file here.

As I know, we are using icon font in Settings now, so I didn't update Settings sprite. Please refer to http://gaia-components.github.io/gaia-icons/

Let me know if there is any question, thank you!

Comment 31

[jhobin]

(In reply to Arthur Chen [:arthurcc] from comment #29)
> Hi James, thanks for the work! Note that in settings app we are now using
> AMD to define modules, so newly added scripts should be wrapped as AMD
> modules. Details please refer to
> https://github.com/mozilla-b2g/gaia/tree/master/apps/settings.

I've refactored the PR to use AMD modules, thanks for the feedback! The last thing that remains is to incorporate the icons.

Comment 32

[jhobin]

(In reply to Helen Huang from comment #30)
> Created attachment 8472888 [details]
> Browsing.svg
> 
> The Browsing icon is done, I've attached the SVG file here.
> 
> As I know, we are using icon font in Settings now, so I didn't update
> Settings sprite. Please refer to http://gaia-components.github.io/gaia-icons/
> 
> Let me know if there is any question, thank you!

I think I also need an icon for the Search category, unless there is one that can be reused somewhere.

I'm also not sure how to generate the 'icons.css' file in the shared style directory. gaia-components/gaia-icons generates a very different file. (icons.css has an item for each icon, whereas gaia-icons uses "content: attr(data-icon)")

Comment 33

[jhobin]

Attachment: Add a Browsing Icon to gaia-icons

In theory, this is the beginning of placing browsing.svg within the Gaia icons font where it can then be placed into icons.css and the Settings app in general. I don't actually know what I'm doing, any feedback on how to actually do this would be greatly appreciated.

Comment 34

[Kevin Grandon :kgrandon]

Comment on attachment 8473905 [details] [review]
Add a Browsing Icon to gaia-icons

Wilson - can you help us here? We are lost without your guidance.

Comment 35

[Wilson Page [:wilsonpage]]

Comment on attachment 8473905 [details] [review]
Add a Browsing Icon to gaia-icons

LANDED (gaia-icons:master)

https://github.com/gaia-components/gaia-icons/commit/1b356c37a71af77ce4e5b3755cc15bf9b60ad86a#diff-d41d8cd98f00b204e9800998ecf8427e

STAMPTED v0.5.1

https://github.com/gaia-components/gaia-icons/releases/tag/v0.5.1

Comment 36

[Wilson Page [:wilsonpage]]

Attachment: pull-request (master) - gaia-icons

Comment 37

[Helen Huang [:HHuang]]

(In reply to jhobin from comment #32)

> I think I also need an icon for the Search category, unless there is one
> that can be reused somewhere.

Following the UX spec, I didn't see anything relates the Search category icon, could you provide me any document that I can know what it's about?

Thanks!

Comment 38

[Wilson Page [:wilsonpage]]

I'm getting a lot of interest around gaia-icons, so I've just updated the README [1] to included Contribution instructions. I also wrote 'Version controlled packages in Gaia' [2] to explain installation options to Gaia Hackers.

[1] https://github.com/gaia-components/gaia-icons
[2] https://gist.github.com/wilsonpage/3d7f636a78db66f8f1d7

Comment 39

[Wilson Page [:wilsonpage]]

(In reply to Helen Huang from comment #37)
> (In reply to jhobin from comment #32)
> 
> > I think I also need an icon for the Search category, unless there is one
> > that can be reused somewhere.
> 
> Following the UX spec, I didn't see anything relates the Search category
> icon, could you provide me any document that I can know what it's about?
> 
> Thanks!

FYI current search icon: http://gaia-components.github.io/gaia-icons/#search

Comment 40

[Kevin Grandon :kgrandon]

Comment on attachment 8474459 [details] [review]
pull-request (master) - gaia-icons

Probably fine, but seems weird to be accessing the document like that to me.

Comment 41

[jhobin]

(In reply to Helen Huang from comment #37)
> (In reply to jhobin from comment #32)
> 
> > I think I also need an icon for the Search category, unless there is one
> > that can be reused somewhere.
> 
> Following the UX spec, I didn't see anything relates the Search category
> icon, could you provide me any document that I can know what it's about?
> 
> Thanks!

It looks like we do have a preexisting search icon.

Comment 42

[jhobin]

(In reply to Wilson Page [:wilsonpage] from comment #36)
> Created attachment 8474459 [details] [review]
> pull-request (master) - gaia-icons

This doesn't modify `style/icons.css`, the icons source used in the Settings app. Is there a way to regenerate that file for the new icons?

Comment 43

[Wilson Page [:wilsonpage]]

(In reply to jhobin from comment #42)
> (In reply to Wilson Page [:wilsonpage] from comment #36)
> > Created attachment 8474459 [details] [review]
> > pull-request (master) - gaia-icons
> 
> This doesn't modify `style/icons.css`, the icons source used in the Settings
> app. Is there a way to regenerate that file for the new icons?

Ah, the icons.css used in Settings currently is deprecated. Once bug 1015297 lands, Settings app will be running with the new gaia-icons icon-font, found at `shared/elements/gaia-icons/`. We're waiting for an r+. 

Until then, you could rebase your patch on top of bug 1015297 so that you can get your work done.

Comment 44

[Wilson Page [:wilsonpage]]

Comment on attachment 8474459 [details] [review]
pull-request (master) - gaia-icons

Gaia-icons has already been updated in shared as part of another bug.

Comment 45

[Kevin Grandon :kgrandon]

Comment on attachment 8472888 [details]
Browsing.svg

Arnau - we want to include this in the existing icons.css file in shared/. Do you have any documentation of the workflow to do this? Alternatively could you get this included for us? Thanks!

Comment 46

[Wilson Page [:wilsonpage]]

Attachment: pull-request (master) - shared/icons.css

I've updated this for you guys. But want to clarify that shared/icons.css is deprecated and will be dropped from settings when bug 1015297 lands.

Comment 47

[Kevin Grandon :kgrandon]

Comment on attachment 8472888 [details]
Browsing.svg

Sounds good, thanks!

Comment 48

[Kevin Grandon :kgrandon]

Comment on attachment 8475355 [details] [review]
pull-request (master) - shared/icons.css

I will throw my R+ on here and get this landed to get James unblocked.

Comment 49

[Kevin Grandon :kgrandon]

Icon part landed here: https://github.com/mozilla-b2g/gaia/commit/d3824cb8e6eaef0144b0fd8bcca24d31bef21213

Comment 50

[jhobin]

Comment on attachment 8472022 [details] [review]
Gaia pull request

This should be properly set up for the current icons

Comment 51

[Kevin Grandon :kgrandon]

Comment on attachment 8472022 [details] [review]
Gaia pull request

Thanks James. I'm pretty happy with this, but I think we need peers from the respective components to put an official R+ from this.

I'm flagging Tim and Alive for the System app, and Evelyn and Arthur for the settings app. I think you only need one review for each app, but I'm not sure who has cycles to review.

I believe James's last day might be on Friday, so if you guys could promptly review this tonight it would be greatly appreciated.

Comment 52

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Comment on attachment 8472022 [details] [review]
Gaia pull request

r+ system app part.

Comment 53

[Evelyn Hung]

Comment on attachment 8472022 [details] [review]
Gaia pull request

Looks good! r+ with nit addressed. Thanks.

Comment 54

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8472022 [details] [review]
Gaia pull request

Thanks for the patch! Please check my comments in github.

Comment 55

[jhobin]

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) (please ni?) from comment #52)
> Comment on attachment 8472022 [details] [review]
> Gaia pull request
> 
> r+ system app part.

(In reply to Evelyn Hung [:evelyn] from comment #53)
> Comment on attachment 8472022 [details] [review]
> Gaia pull request
> 
> Looks good! r+ with nit addressed. Thanks.

(In reply to Arthur Chen [:arthurcc] from comment #54)
> Comment on attachment 8472022 [details] [review]
> Gaia pull request
> 
> Thanks for the patch! Please check my comments in github.

Thank you all very much for your reviews! I think I've addressed all of the comments made on the pull request.

Comment 56

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8472022 [details] [review]
Gaia pull request

The ids in the script should also be replaced with class names. r=me with the issue addressed, thanks!

Comment 57

[jhobin]

Passing try build: https://tbpl.mozilla.org/?rev=d42239dc0a713dda2a69a46c0bd0ce14e739f637&tree=Gaia-Try

(the latest push is only to fix a merge conflict)

Comment 58

[Ryan VanderMeulen [:RyanVM]]

Can you please mark obsolete PRs as such? Makes it hard to know what to check in otherwise.

Comment 59

[jhobin]

Comment on attachment 8475355 [details] [review]
pull-request (master) - shared/icons.css

>https://github.com/mozilla-b2g/gaia/pull/23045

Comment 60

[jhobin]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #58)
> Can you please mark obsolete PRs as such? Makes it hard to know what to
> check in otherwise.

Sorry about that! I forgot that the other PRs were still listed.

Comment 61

[Ryan VanderMeulen [:RyanVM]]

Thanks!

Master: https://github.com/mozilla-b2g/gaia/commit/3579256554985d6ad49bee4615651a255962f22f

Comment 62

[Francesco Lodolo [:flod]]

Filed follow-up bug 1057696, the number of l10n issues is disappointing. Not sure how else to ask, but can we please starting to test patches with pseudo-locales?

Comment 63

[Shally Li（leave from Mozilla）]

Hi all,
    This issue has been successfully verified on Flame 2.1.
    See attachment: verified_v2.1.mp4
    Reproducing rate: 0/5

Flame 2.1 build:
Gaia-Rev        1bdd49770e2cb7a7321e6202c9bf036ab5d8f200
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/db893274d9a6
Build-ID        20141125001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141125.040617
FW-Date         Tue Nov 25 04:06:28 EST 2014
Bootloader      L1TC00011880

Comment 64

[Shally Li（leave from Mozilla）]

Attachment: verified_v2.1.mp4