Closed Bug 938177 Opened 8 years ago Closed 7 years ago

[User Story] Browser Settings Moved to System Settings

Categories

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

x86
macOS
defect
Not set
normal

Tracking

(feature-b2g:2.1, tracking-b2g:backlog, b2g-v1.4 affected, b2g-v2.0 affected, b2g-v2.1 verified)

RESOLVED FIXED
2.1 S3 (29aug)
feature-b2g 2.1
tracking-b2g backlog
Tracking Status
b2g-v1.4 --- affected
b2g-v2.0 --- affected
b2g-v2.1 --- verified

People

(Reporter: pdol, Assigned: jhobin)

References

Details

(Keywords: feature, Whiteboard: [ucid:Browser106, ft:systemsfe], permafail)

Attachments

(4 files, 4 obsolete files)

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.
Keywords: feature
Flags: in-moztrap?(nhirata.bugzilla)
No longer blocks: 1.3-systems-fe
No longer blocks: browser-chrome-mvp
Component: Gaia::Browser → Gaia::System::Browser Chrome
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.
Component: Gaia::System::Browser Chrome → Gaia::Settings
Duplicate of this bug: 941255
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.
(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.
Flags: needinfo?(swilkes)
Flagging Francis to ensure this ends up in the correct spot. Francis, please flag Peter if an icon is needed. Thanks!
Flags: needinfo?(swilkes) → needinfo?(fdjabri)
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?
Flags: needinfo?(bfrancis)
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?
Flags: needinfo?(bfrancis) → needinfo?(dale)
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
Flags: needinfo?(dale)
Depends on: 972314
Depends on: 972315
Flags: needinfo?(fdjabri) → needinfo?(ofeng)
Here is the UX proposal.
Flags: needinfo?(ofeng)
No longer blocks: 1.4-systems-fe
Whiteboard: [ucid:Browser106, 1.3:P2, ft:systems-fe] → [ucid:Browser106, 1.3:P2, ft:systems-fe], burirun1.4-1, burirun1.4-2
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?
Flags: needinfo?(pdolanjski)
Whiteboard: [ucid:Browser106, 1.3:P2, ft:systems-fe], burirun1.4-1, burirun1.4-2 → [ucid:Browser106, 1.3:P2, ft:systems-fe], permafail
blocking-b2g: --- → backlog
feature-b2g: --- → 2.1
Flags: needinfo?(pdolanjski)
Whiteboard: [ucid:Browser106, 1.3:P2, ft:systems-fe], permafail → [ucid:Browser106, ft:systemsfe], permafail
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?
Flags: needinfo?(jelee)
Flags: needinfo?(epang)
(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!
Flags: needinfo?(epang) → needinfo?(chuang)
Flags: needinfo?(chuang)
hey Eric,
Helen is taking care of Settings right now.

Hi Helen,
Could you help on this browsing icon in Settings? Thank you!
Flags: needinfo?(hhuang)
Sure. I will create a browsing icon and upload it once I done.
Flags: needinfo?(hhuang)
(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!
Flags: needinfo?(jelee)
(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?
Flags: needinfo?(jelee)
Flags: needinfo?(dale)
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?
> 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
Flags: needinfo?(dale)
Francis should have an answer here.
Flags: needinfo?(fdjabri)
> 
> > 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.
Flags: needinfo?(fdjabri) → needinfo?(hhuang)
Hi all, please take a look at the latest Browsing spec. Thank you
Attachment #8378818 - Attachment is obsolete: true
Flags: needinfo?(jelee)
(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!
Assignee: nobody → hobinjk
Target Milestone: --- → 2.1 S2 (15aug)
Attached file 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.
Assignee: hobinjk → jhobin
Status: NEW → ASSIGNED
Attachment #8472022 - Flags: review?(kgrandon)
(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?
Eric, do we have assets for this somewhere?
Flags: needinfo?(epang)
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!
Attachment #8472022 - Flags: review?(kgrandon)
(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!
Flags: needinfo?(epang)
Depends on: 941253
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.
Flags: needinfo?(jhobin)
Attached image 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!
Flags: needinfo?(hhuang)
(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.
Flags: needinfo?(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)")
Flags: needinfo?(hhuang)
Attached file Add a Browsing Icon to gaia-icons (obsolete) —
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 on attachment 8473905 [details] [review]
Add a Browsing Icon to gaia-icons

Wilson - can you help us here? We are lost without your guidance.
Attachment #8473905 - Flags: feedback?(wilsonpage)
Attached file pull-request (master) - gaia-icons (obsolete) —
Attachment #8474459 - Flags: review?(kgrandon)
(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!
Flags: needinfo?(hhuang) → needinfo?(jhobin)
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
(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 on attachment 8474459 [details] [review]
pull-request (master) - gaia-icons

Probably fine, but seems weird to be accessing the document like that to me.
Attachment #8474459 - Flags: review?(kgrandon) → review+
Blocks: rocketbar-mvp
No longer blocks: browser-chrome-mvp
(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.
Flags: needinfo?(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?
Flags: needinfo?(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.
Flags: needinfo?(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.
Attachment #8474459 - Attachment is obsolete: true
Depends on: 1015297
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!
Attachment #8472888 - Flags: feedback?(rnowmrch)
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 on attachment 8472888 [details]
Browsing.svg

Sounds good, thanks!
Attachment #8472888 - Flags: feedback?(rnowmrch)
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.
Attachment #8475355 - Flags: review+
Comment on attachment 8472022 [details] [review]
Gaia pull request

This should be properly set up for the current icons
Attachment #8472022 - Flags: review?(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.
Attachment #8472022 - Flags: review?(timdream)
Attachment #8472022 - Flags: review?(kgrandon)
Attachment #8472022 - Flags: review?(ehung)
Attachment #8472022 - Flags: review?(arthur.chen)
Attachment #8472022 - Flags: review?(alive)
Attachment #8472022 - Flags: feedback+
Comment on attachment 8472022 [details] [review]
Gaia pull request

r+ system app part.
Attachment #8472022 - Flags: review?(timdream)
Attachment #8472022 - Flags: review?(alive)
Attachment #8472022 - Flags: review+
Comment on attachment 8472022 [details] [review]
Gaia pull request

Looks good! r+ with nit addressed. Thanks.
Attachment #8472022 - Flags: review?(ehung)
Attachment #8472022 - Flags: review?(arthur.chen)
Attachment #8472022 - Flags: review+
Comment on attachment 8472022 [details] [review]
Gaia pull request

Thanks for the patch! Please check my comments in github.
Attachment #8472022 - Flags: review-
(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.
Attachment #8472022 - Flags: review- → review?(arthur.chen)
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!
Attachment #8472022 - Flags: review?(arthur.chen) → review+
No longer depends on: 1015297
Target Milestone: 2.1 S2 (15aug) → 2.1 S3 (29aug)
Passing try build: https://tbpl.mozilla.org/?rev=d42239dc0a713dda2a69a46c0bd0ce14e739f637&tree=Gaia-Try

(the latest push is only to fix a merge conflict)
Keywords: checkin-needed
Can you please mark obsolete PRs as such? Makes it hard to know what to check in otherwise.
Keywords: checkin-needed
Attachment #8473905 - Attachment is obsolete: true
Comment on attachment 8475355 [details] [review]
pull-request (master) - shared/icons.css

>https://github.com/mozilla-b2g/gaia/pull/23045
Attachment #8475355 - Attachment is obsolete: true
(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.
Thanks!

Master: https://github.com/mozilla-b2g/gaia/commit/3579256554985d6ad49bee4615651a255962f22f
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Depends on: 1057696
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?
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
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.