Navigating the in content preferences feels a bit clunky always having to use the back arrow to get back to the main list. If a side list like what is used for add-ons is adopted I think this would make the prefs in content feel more natural.

In the original mockups, there's something akin to the URL bar used. Could that not be used, using drop downs at the breadcrumbs as Windows 7 does with its Control Panel (Explorer)?

I think we would want to keep a consistent UI/UX with our various in-content panels 1) so people understand what to do without relearning 2) so they feel like things are consistent 3) so people trying to spoof have a harder time

In fact it seems like the right even if the whole thing feels a bit outdated visually speaking (Chrome provides for example smooth animations between categories). A bit of visual work could be good : http://people.mozilla.com/~shorlander/files/australis-design-specs/images/Australis-i01-DesignSpec-InContentUI-%28Addons-Manager%29.jpg

This really needs some UX thought and input before any work is started on this.

Jon, Christian has put together a rough first-pass patch for this bug so reassigning to him. If you've already got a patch that you're working on, please upload it here so that we can get the best of both patches.

"I think we would want to keep a consistent UI/UX with our various in-content panels 1) so people understand what to do without relearning 2) so they feel like things are consistent 3) so people trying" Plus its better that way

(In reply to magnumarchonbasileus from comment #6) > Created attachment 627110 [details] > Should look like this The general idea of this bug is to make the different in-content panels look alike, (and like the current add-on manager) - not to mimic the old panels look in-content - precisely to make the in-content panels look more consistent throughout the product. The content of the individual preference panes is being redesigned in a different bug.

some quick comments on latest try-build found here: https://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/colmeiro@gmail.com-1b333d5b2441/ some pop-up dialog boxes disappear on open but clicking in the body brings it back with a warning-bell: Options->Advanced-> Update: Show updates pop-up hides, there was another on the Security tab: Saved Passwords some pop-up dialog boxes open in upper left-corner, others in middle of screen Security tab->Exceptions button - opens upper left Saved Passwords - opens Middle but hides until clicking again on the button Security tab: Warn when sites... Exceptions open upper left Privacy tab: Show Cookies - opens middle page Privacy tab: Accept cookies from sites.. Exceptions open upper left Advanced tab: Update tab -> Show Update History - Hides until clicked again Advanced tab: Encryption tab: pop-ups open in various places, mid-left side, upper left and middle of page. I have not taken time to test every function but quick testing shows similar problems with dialog boxes as noted above.

This patch is a work in progress. It's not rebased to current tip, and we still need to move the style and xml that's common to preferences and add-ons to inContentUI.{css,xml} files.

I talked with Christian and Hernan and they said it would be fine if I took this bug. This patch is based on the previous WIP patch but I've made some changes to it. I cleaned up some miscellaneous parts of it, removed the search field, and the extra images that were previously added. I'll attach a second part that removes redundancy with inContentUI.css. Blair, can you review this in the meantime?

Comment on attachment 655281 [details] [diff] [review] Patch drive-by review comment: >--- a/browser/components/preferences/in-content/preferences.xul >+++ b/browser/components/preferences/in-content/preferences.xul >+<?xml-stylesheet href="chrome://mozapps/content/extensions/extensions.css"?> >+<?xml-stylesheet href="chrome://mozapps/content/extensions.css"?> >+<?xml-stylesheet href="chrome://mozapps/skin/extensions/extensions.css"?> Code unrelated to about:addons shouldn't depend on these stylesheets.

Comment on attachment 655281 [details] [diff] [review] Patch Review of attachment 655281 [details] [diff] [review]: ----------------------------------------------------------------- * There's a bunch of changes here that are unrelated fixups, that should be done in separate bugs - handlers.css, main.js, preferences/main.xul. * Mix of name/aName for names of argument variables (I much prefer the later). * Using a mix of terminology to refer to the same thing - page, pane, category. I think category fits best, both in meaning, and how its used in the Add-ons Manager code which we're now wanting to share. * Missing gnomestripe & pinstripe changes? ::: browser/components/preferences/in-content/preferences.js @@ +17,5 @@ > + prefPanes: null, > + forwardButton: null, > + backButton: null, > + > + initialize: function gPreferences__initialize() { Nit: I can haz whitespace in this function? @@ +20,5 @@ > + > + initialize: function gPreferences__initialize() { > + document.documentElement.instantApply = true; > + this.categories = document.getElementById("categories"); > + this.prefPanes = document.getElementById("mainPrefPane").children; "prefPanes" is a misleading name. @@ +44,2 @@ > > + gotoPref: function gPreferences__gotoPref(page) { May as well rename this to be showPane/showCategory/something while you're at it. @@ +67,3 @@ > > + onStatePopped: function gPreferences__onStatePopped(aEvent) { > + var id = "category-"+aEvent.state.slice(4).toLowerCase(); This line should contain less magic. @@ +73,5 @@ > + gPreferences.categories.selectedItem = item; > + gPreferences.categories.suppressOnSelect = false; > + } > + gPreferences.search(aEvent.state, "data-category"); > + gPreferences.currentPage = aEvent.state; There's a disconnect here - .search() gives as a kind of free-form filtering, but then using .currentPage means we can only ever use categories. FWIW, the Add-ons Manager solves this by using view IDs - strings that uniquely identify a state, and can be parsed to regenerate that state. That's what is stored to identify the current view, reconstruct history, etc. eg, addons://search/whatever, prefs://category/tabs, etc. IMO, it works well for the Add-ons Manager, with the downside of not being easy to navigate to a view via entering a URL in the URL bar (been thinking of fixing that by implementing a proper firefox:// protocol). I originally thought that was overkill for Preferences, but maybe not - given the need to handle things like search in history. Might make sense to wait til we do search to implement something like that - in which case, I think .search() should be fixed up to be something like .filterCategory() so we're not in this half-way state. ::: browser/components/preferences/in-content/preferences.xul @@ +5,5 @@ > > +<?xml-stylesheet href="chrome://global/skin/" type="text/css"?> > +<?xml-stylesheet href="chrome://mozapps/content/extensions/extensions.css"?> > +<?xml-stylesheet href="chrome://mozapps/content/extensions.css"?> > +<?xml-stylesheet href="chrome://mozapps/skin/extensions/extensions.css"?> What Dao said - these shouldn't be here, even if they're only temporary. @@ +18,5 @@ > > <!DOCTYPE page [ > +<!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd" > > +%brandDTD; > +<!ENTITY % extensionsDTD SYSTEM "chrome://mozapps/locale/extensions/extensions.dtd"> Ditto. @@ +146,5 @@ > + > + <box id="view-port-container" class="main-content" flex="1"> > + > + <!-- view port --> > + <deck id="view-port" flex="1" selectedIndex="0"> Doesn't make sense to have this be a <deck> when there's only one child element. @@ +172,5 @@ > + > + <stringbundleset id="appManagerBundleset"> > + <stringbundle id="appManagerBundle" > + src="chrome://browser/locale/preferences/applicationManager.properties"/> > + </stringbundleset> All these string bundles should be at the top of the file, before the visible content. ::: browser/themes/winstripe/preferences/in-content/preferences.css @@ +47,4 @@ > } > > +#category-sync { > + list-style-image: url("chrome://browser/skin/preferences/Options-sync.png"); This should depend on MOZ_SERVICES_SYNC.

(In reply to Christian Sonne [:cers] from comment #7) > Created attachment 627147 [details] > Screenshot of current patch > Does anyone else find the tab icon in this screenshot (and the "tabs on the bottom of the window" behaviour it implies) ominous?

(In reply to Stephan Sokolow from comment #13) > (In reply to Christian Sonne [:cers] from comment #7) > > Created attachment 627147 [details] > > Screenshot of current patch > > > > Does anyone else find the tab icon in this screenshot (and the "tabs on the > bottom of the window" behaviour it implies) ominous? I'm not quite sure what you mean by any of those questions. If you're talking about the look and placement of tabs, then that's how Firefox currently looks in OS X. Only the content of the tab (and currently which icon the tab uses) is relevant to this bug.

> I'm not quite sure what you mean by any of those questions. There was just one question. :-) What he means is that the icon of the "Tabs" panel is the other way round. Sebastian

This bug is just about making the in-content preferences more usable by mimicking the add-on manager. There will be further visual improvements later (like new icons) as part of the Australis redesign.

(In reply to Sebastian Zartner from comment #15) > > I'm not quite sure what you mean by any of those questions. > There was just one question. :-) > What he means is that the icon of the "Tabs" panel is the other way round. > > Sebastian Ah! Thanks for clarifying that - and good eye there Stephan. But yes, as Guillaume states, I doubt this is the correct bug to start changing icons. Bug 738796 might be more relevant.

Personally I find the layout and appearance very appealing. However I found I was not able to edit the Save Files to: downloads location. Also using the Browse button it would append the directory to the directory I wanted to use. I want this: d:\downloads I get this: d:\downloads\downloads It also updates in the about:config to the incorrect directory.

Any news on this ?

I'll be working on this again soon, as part of my internship - but it might be a few weeks before I can really dedicate time to it...

Fixed bug that were causing XBL bindings to go away (or not attach), and moved styling to shared theme. Only tested in OSX though

Some news on design on the preferences etherpad : https://etherpad.mozilla.org/Preferences >In-content Prefs (not blocking Australis but related) >shippable in-content prefs (ship in-content prefs with minimal redesign of the >current preferences content) https://etherpad.mozilla.org/Preferences >Some questions: > >Make the panel less spread out, add padding on the right of the page > >Move Tabs into General panel (https://bugzilla.mozilla.org/show_bug.cgi?id=767313) > >Do we want URLs for Preferences? (not for now) > >Back/Forward is not really necessary if we don't have URL ( and if we open a >new tab when going to about:preferences) > >New visual redesign for Australis (Add-ons & Preferences)? > shorlander will have something

(In reply to Christian Sonne [:cers] from comment #21) > Created attachment 731311 [details] [diff] [review] > Patch > > Fixed bug that were causing XBL bindings to go away (or not attach), and > moved styling to shared theme. > > Only tested in OSX though Thanks Christian. However it looks like you forgot to "hg add". The shared preferences.inc.css does not exist before/after your patch is applied. If you don't have time to work on this right now, could you please upload the preferences.inc.css file here and I'll work on this for a little bit. Thanks.

Comment on attachment 750856 [details] [diff] [review] Patch Review of attachment 750856 [details] [diff] [review]: ----------------------------------------------------------------- Don't need the back/forward buttons now, since we've gone ahead with bug 752434 (so the back/forward buttons in the navbar are usable now). On Windows, at least, the captions in groupboxes have a different background colour - will need to override the default, so it's transparent. ::: browser/components/preferences/in-content/preferences.xul @@ +6,5 @@ > + > +<?xml-stylesheet href="chrome://global/skin/global.css" type="text/css"?> > +<?xml-stylesheet href="chrome://mozapps/content/extensions/extensions.css"?> > +<?xml-stylesheet href="chrome://mozapps/content/extensions.css"?> > +<?xml-stylesheet href="chrome://mozapps/skin/extensions/extensions.css"?> Need to either copy the relevant code into files in browser/components/preferences/in-content/, or put it somewhere so it can be properly shared. Probably easiest to do the former now, and leave the later to a followup bug. Depending on Add-ons Manager code like this is just asking for trouble. @@ +20,5 @@ > <!DOCTYPE page [ > +<!ENTITY % brandDTD SYSTEM "chrome://branding/locale/brand.dtd" > > +%brandDTD; > +<!ENTITY % extensionsDTD SYSTEM "chrome://mozapps/locale/extensions/extensions.dtd"> > +%extensionsDTD; Strings are one thing we absolutely cannot re-use like this. @@ +104,5 @@ > + <richlistitem id="category-general" > + class="category" > + name="&paneGeneral.title;" > + tooltiptext="&paneGeneral.title;" > + onclick="gPreferences.gotoPref('paneGeneral')"/> These need to use the select event, so they can be keyboard navigable. @@ +147,5 @@ > + > + <box id="view-port-container" class="main-content" flex="1"> > + > + <!-- view port --> > + <deck id="view-port" flex="1" selectedIndex="0"> Why is a <deck> needed here? @@ +167,3 @@ > </hbox> > + > + <stringbundle id="bundleBrand" Usually these are placed at the top of the file, with <script> etc. @@ +170,5 @@ > + src="chrome://branding/locale/brand.properties"/> > + <stringbundle id="bundlePreferences" > + src="chrome://browser/locale/preferences/preferences.properties"/> > + > + <stringbundleset id="appManagerBundleset"> Does any code use this ID? If not, merge all stringbundles into one stringbundleset. ::: browser/themes/shared/preferences/in-content/preferences.inc.css @@ +6,5 @@ > + > +@namespace html "http://www.w3.org/1999/xhtml"; > + > +[visible="false"] { > + visibility: collapse; This should be in a content CSS file (browser/components/preferences/in-content/preferences.css), as basic functionality of the page depends on it. ::: browser/themes/windows/preferences/in-content/preferences.css @@ +1,5 @@ > +/* - This Source Code Form is subject to the terms of the Mozilla Public > + - License, v. 2.0. If a copy of the MPL was not distributed with this file, > + - You can obtain one at http://mozilla.org/MPL/2.0/. */ > +%include ../../../shared/preferences/in-content/preferences.inc.css > +#handlersView { Y U NO LIKE linebreaks?! (Blank line between each rule.) Ditto for the same file in the linux theme.

I want to assist/work on this bug. I have adequate CSS and DOM knowledge. How shall I proceed?

Christian had been working on this - don't think he's been able to put much time into it recently, but IIRC he did have a more recent patch that's attached to the bug. Christian?

I recently stumbled over our in-content preferences that we ship but hide. This bugged me and I felt like someone should spend some time on it :) This patch makes the in-content preference navigation resemble the one from about:addons. The styles are copied and not shared, it doesn't feel like there's much value in creating a big abstraction here yet before we rip out the old preference dialog. I modified utilityOverlay.js so that when flipping 'browser.preferences.inContent' the whole UI just points to our new in-content preferences, complete with support for pointing to specific panes. Session history works as well, when an about:preferences tab is restored we show the same pane as last time. Navigating back/fwd works as well - I think it did before, too. [Sorry for stealing this, this seemed like a great weekend project :]

Forgot to handle the current pref dialog URL in nsBrowserContentHandler with browser.preferences.inContent=true.

(In reply to Tim Taubert [:ttaubert] from comment #28) > Created attachment 819610 [details] [diff] [review] > Make in-content prefs navigation look like about:addons > [Sorry for stealing this, this seemed like a great weekend project :] I'm glad you did! (I actually spent some of the weekend unbitrotting my old patch) - but now I've started working on bug 782599 instead :-) A quick comment on the latest patch you posted, it yields the error: Error in parsing value for 'border-radius'. Declaration dropped. preferences.css:160 That line reads: border-radius: @toolbarbuttonCornerRadius@; As far as I can tell, the file is pre-processed, so I'm not sure why that would still be there...

(In reply to Christian Sonne [:cers] from comment #30) > I'm glad you did! (I actually spent some of the weekend unbitrotting my old > patch) - but now I've started working on bug 782599 instead :-) Yay, search. Thanks for picking that up and not hating me! > A quick comment on the latest patch you posted, it yields the error: > Error in parsing value for 'border-radius'. Declaration dropped. > preferences.css:160 Hmm, yeah I have no idea where that variable is defined but I'll investigate that, thanks!

Fixed the "Error in parsing value for 'border-radius'" pointed out by cers.

Comment on attachment 821515 [details] [diff] [review] Make in-content prefs navigation look like about:addons, v3 Review of attachment 821515 [details] [diff] [review]: ----------------------------------------------------------------- Clearing review based on the lack of the similar changes for applications.css. ::: browser/base/content/utilityOverlay.js @@ +498,5 @@ > + > + if (newLoad) { > + browser.addEventListener("load", function onload() { > + browser.removeEventListener("load", onload, true); > + browser.contentWindow.setTimeout(switchToPane); Can you add a comment that explains why this setTimeout is necessary? ::: browser/themes/osx/preferences/applications.css @@ +13,5 @@ > margin-top: 0; > margin-bottom: -1px; > } > > +#handlersView > richlistitem label { I don't see a similar change for {windows, linux}/preferences/applications.css. ::: browser/themes/windows/preferences/in-content/preferences.css @@ +10,5 @@ > margin-bottom: 18px; > } > > +caption { > + font-size: 20px; Can you switch this to 1.667rem? @@ +66,4 @@ > } > > +.category-name { > + font-size: 150%; 1.5rem? @@ +107,4 @@ > } > > +#category-advanced > .category-icon { > + -moz-image-region: rect(0px, 224px, 32px, 192px) s/0px/0/g

Comment on attachment 823896 [details] [diff] [review] Make in-content prefs navigation look like about:addons, v4 Review of attachment 823896 [details] [diff] [review]: ----------------------------------------------------------------- r=me, just looked at the interdiff this time since there were no other issues in the previous review.

Backed out for test failures, sorry :/ https://hg.mozilla.org/integration/fx-team/rev/6f04b612353d

For posterity, these were the test failures: TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug735471.js | Test timed out TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug743421.js | uncaught exception - ReferenceError: is is not defined at chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug735471.js:26 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug763468_perwindowpb.js | uncaught exception - ReferenceError: is is not defined at chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug735471.js:45 TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug783614.js | location bar value after cutting 's' from https - Got https://example.com/, expected http://example.com/ TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug832435.js | Test timed out TEST-UNEXPECTED-FAIL | chrome://mochitests/content/browser/browser/base/content/test/general/browser_canonizeURL.js | uncaught exception - ReferenceError: ok is not defined at chrome://mochitests/content/browser/browser/base/content/test/general/browser_bug832435.js:12

Re-landed with a small fix and cleanup for browser_bug735471.js. Try agrees. https://hg.mozilla.org/integration/fx-team/rev/a3f771caf25d

Just updated to the latest nightly build and it doesn't use the full width. Is the patch already fully implemented?

(In reply to sjw from comment #41) > Just updated to the latest nightly build and it doesn't use the full width. > Is the patch already fully implemented? Yes, this is intentional. The design will be changing again soon, but this current iteration is sufficient for the time being.