With the new theme hiding the MenuBar we need a place to expose the contents of the Bookmarks menu. It is also a good time to streamline the bookmarks interface: * Hide the Bookmarks Bar by default for new profiles and profiles that have never modified the default contents * A Bookmarks Widget that replicates the Bookmarks Menu will be placed to the right of the Search Bar by default or if the Bookmarks Bar is hidden * If the Bookmarks Bar is shown place the Bookmarks Widget on the left side of the Bookmarks Bar * Replicate the contents and functionality of the bookmarks menu minus "Bookmark This Page" and "Subscribe to This Page" because they are exposed in the interface See attachment for mockup.

Why would you completely move the widget if the bookmarks bar is showing? Have it in one place so it feels natural. A bookmark is a bookmark and is on the bookmarks toolbar for it's frequent use. If it's in the bookmarks folder, then I definitely don't need it's urgency pushed by displacing the rest of my bookmarks. Stick it on the far right and keep it there at all times.

For me it is pretty obvious that bookmarks button is in bookmarks bar.

Why? Especially if it's not there by default. It means change and option for additional usability and your break familiarity. Don't get me wrong. I currently don't have any icons on my navigation bar and have moved the Ubiquity button to the right side of the bookmarks bar. You apparently can't do this with the bookmarks button which is a shame. But after months of using the Mockup (http://spewboy.deviantart.com/art/Strata40-v0-6-132191373 by SpewBoy as you well know), I have to say that it still things wrong and out of place. I don't have nor want a bookmark to my bookmarks, I want to be able to access them, but not as the cost of pushing my bookmark toolbar bookmarks over and messing with the amount of room I have to spare for them, which I've already increased thanks to Smart Bookmarks (https://addons.mozilla.org/en-US/firefox/addon/4072) which I consider usability that should be stock.

Also, having two such huge drop downs in the same place really isn't a good idea, as The application button and the bookmarks toolbar button share the same space.

Which application button?

The big orange Firefox Menu Replacement one.

How the hell can app menu and bookmarks buuton share the same place?!

Please don't make me open a graphics program just to show you that if you activate the booksmarks toolbar button you can't see the Firefox button.

I'd like to see that.

Yeah, me too.

In my opinion, the Firefox menu should remain visible at all times, except for when in full screen mode. The bookmarks menu clearly overlaps the button in this attachment. Large menus such as these shown should have their own independent space,, so as that there's never a miss-click that takes three clicks to correct.

The "Firefox" app menu will move to the title bar, see the first attachment in this bug.

(In reply to comment #11) > Created an attachment (id=436206) [details] > Image showing Firefox App menu being hidden by bookmarks > > In my opinion, the Firefox menu should remain visible at all times, except for > when in full screen mode. The bookmarks menu clearly overlaps the button in > this attachment. > > Large menus such as these shown should have their own independent space,, so as > that there's never a miss-click that takes three clicks to correct. Please don't post more in this bug if you have issues with the Strata theme. It is not maintained by Mozilla.

(In reply to comment #11) > Created an attachment (id=436206) [details] > Image showing Firefox App menu being hidden by bookmarks > > In my opinion, the Firefox menu should remain visible at all times, except for > when in full screen mode. The bookmarks menu clearly overlaps the button in > this attachment. > > Large menus such as these shown should have their own independent space,, so as > that there's never a miss-click that takes three clicks to correct. I hate to say it, but, FAIL! Don't talk about something you clearly don't understand.

(In reply to comment #13) > Please don't post more in this bug if you have issues with the Strata theme. It > is not maintained by Mozilla. Actually, the Strata theme is maintained by Mozilla :) (but not strata40)

=(In reply to comment #12) > The "Firefox" app menu will move to the title bar, see the first attachment in > this bug. You're right, I forgot, my apologies. I still maintain that they should have their own space though at separate ends of the window.

(In reply to comment #13) > (In reply to comment #11) > > Created an attachment (id=436206) [details] [details] > > Image showing Firefox App menu being hidden by bookmarks > > > > In my opinion, the Firefox menu should remain visible at all times, except for > > when in full screen mode. The bookmarks menu clearly overlaps the button in > > this attachment. > > > > Large menus such as these shown should have their own independent space,, so as > > that there's never a miss-click that takes three clicks to correct. > > Please don't post more in this bug if you have issues with the Strata theme. It > is not maintained by Mozilla. Please note that while giving an example, it's hard to create a demonstration on a mockup (even an interactive one) that doesn't open these menu's. The best I could achieve was the Strata40 theme.

(In reply to comment #14) > I hate to say it, but, FAIL! Don't talk about something you clearly don't > understand. Cut it out; you are violating bugzilla etiquette: https://bugzilla.mozilla.org/page.cgi?id=etiquette.html This whole dicussion that just happened belongs in the newsgroups (mozilla.dev.usability comes to mind; http://groups.google.com/group/mozilla.dev.usability/topics?lnk=srg), not in a bug.

(In reply to comment #8) > Please don't make me open a graphics program just to show you that if you > activate the booksmarks toolbar button you can't see the Firefox button. Ha ha. It is nice to see that someone here has a sense of humor.

(In reply to comment #16) > You're right, I forgot, my apologies. I still maintain that they should have > their own space though at separate ends of the window. I think having functions at different ends of the window is a good idea. The right side seems kind of empty when the bookmark bar is shown.

> * A Bookmarks Widget that replicates the Bookmarks Menu will be placed to the > right of the Search Bar by default or if the Bookmarks Bar is hidden > * If the Bookmarks Bar is shown place the Bookmarks Widget on the left side of > the Bookmarks Bar > * Replicate the contents and functionality of the bookmarks menu minus > "Bookmark This Page" and "Subscribe to This Page" because they are exposed in > the interface I think a bookmark widget should remain there even if the bookmark bar is shown. I think that hiding and showing that which it might be a little bit inconsistent. I know it seems highly intelligent to move the widget to the bookmark bar when it is shown and not have it in multiple places, but hiding it intelligently like that will confuse users. It reminds me of Microsoft office and intelliMenu we're the menu changes.

While I agree, keep in mind that this will apply only when menu bar is showed permanently.

tentatively assigning to me to give it a kick next week.

This is a first kick at it. Things to implemented yet: - hide bookmarks toolbar by default or if the user never changed it. The second part could be slow to detect though. Still have to find a good way to do it, could be followup. - needs icon, label and tooltip, for now I've used the existing bookmarks sidebar button ones, but that makes them not recognizeable, also because they have same icon for now (what we will do of that button?) Notes: - I tried to use broadcasters for 2 reasons: 1. I recall somebody said we should avoid DOMAttrModified in browser window, because it's a perf hog 2. I guess in future others could want to react to an hidden menubar, these should help observing that event. Alternatives exist, but this was looking like the shorter way to go, the only drawback is that observing the broadcaster means inheriting its attributes (thus the button must be explicitly uncollapsed). I'd like feedback from Gavin or Dao (or some other browser code guru) on this. - the menu contents are mostly a repeat of the bookmarks menu, I have made them separate to be able to split they roads in future, since for now there is no final decision on that (I also noticed some bad behavior when using the same popup from both menus due to the way we use parentNode, so this is for sure less bogus)

Comment on attachment 449929 [details] [diff] [review] patch 1.0 Dao, could you please take a look at the approach and give me a hint if I should investigate other ways? Even if implementation could change, this patch should be fine to test the behavior. It probably also fixes a couple Places menus D&D bugs.

Comment on attachment 449929 [details] [diff] [review] patch 1.0 "bookmarks-widget" seems ambiguous, how about "bookmarks-menu-button"? Instead of the broadcasters, can onViewToolbarCommand just call some function that shows/hides the button? Moving the button probably affects toolbar customization, which is bad. Maybe there should be two buttons, one as an item on the nav bar and one as a child of personal-bookmarks?

Also, I'm not sure the OS X ifdefs make sense.

(In reply to comment #26) > (From update of attachment 449929 [details] [diff] [review]) > "bookmarks-widget" seems ambiguous, how about "bookmarks-menu-button"? sure, i used widget because I miss the final design, from what I got from Steven this is going to become a global browser bookmarks entry point, "menu-button" could be a bit small-scoped. > Instead of the broadcasters, can onViewToolbarCommand just call some function > that shows/hides the button? Yes, but with the broadcaster the status is automatically updated on start and for new profiles... using onViewToolbarCommand I should add an init() method to detect toolbar status and apply it manually. Plus any new thing (like the global menu for example) that will want to check that will have to add its own method to onViewToolbarCommand that will bloat. Do you think broadcasters are bad? > Moving the button probably affects toolbar customization, which is bad. Maybe > there should be two buttons, one as an item on the nav bar and one as a child > of personal-bookmarks? Actually till the user does not hide/show a toolbar the button stays in place. When he changes a toolbar view the button (unfortunately) moves to the default position (right). I did not touch that too much because times when user changes toolbars visiblity are pretty rare (while they will more often change position of the button). Is there a method to persist the position in a toolbar and restore it? we could save an attribute for each toolbar and on button move add it in the right position. Having one button per toolbar would not solve the problem for users moving the button to a third toolbar that is not bookmarks nor nav-bar and would complicate things imho (plus adding more views in the game, bookmarks views are not exactly "free" since they are live-updated). It's a complicated problem. > Also, I'm not sure the OS X ifdefs make sense. I did not know what we wanted to do on OS X, just clarified with Steven and I'll enable everything I can on OS X too.

(In reply to comment #28) > > Instead of the broadcasters, can onViewToolbarCommand just call some function > > that shows/hides the button? > > Yes, but with the broadcaster the status is automatically updated on start and > for new profiles... using onViewToolbarCommand I should add an init() method to > detect toolbar status and apply it manually. Plus any new thing (like the > global menu for example) that will want to check that will have to add its own > method to onViewToolbarCommand that will bloat. > Do you think broadcasters are bad? They seem to be overkill. The same function that onViewToolbarCommand would call can just be called from BrowserStartup as well. Also, the menu bar status can probably be handled entirely with CSS, e.g.: #toolbar-menubar:not([autohide="true"]) ~ #nav-bar > #bookmarks-menu-button { display: none; } > > Moving the button probably affects toolbar customization, which is bad. Maybe > > there should be two buttons, one as an item on the nav bar and one as a child > > of personal-bookmarks? > > Actually till the user does not hide/show a toolbar the button stays in place. > When he changes a toolbar view the button (unfortunately) moves to the default > position (right). I did not touch that too much because times when user changes > toolbars visiblity are pretty rare (while they will more often change position > of the button). > Is there a method to persist the position in a toolbar and restore it? we could > save an attribute for each toolbar and on button move add it in the right > position. > Having one button per toolbar would not solve the problem for users moving the > button to a third toolbar that is not bookmarks nor nav-bar and would > complicate things imho (plus adding more views in the game, bookmarks views are > not exactly "free" since they are live-updated). > It's a complicated problem. The button should be stuck in the nav-bar, it shouldn't be movable.

(In reply to comment #29) Fine for the toggle function, actually I forgot I already have an init()! > The button should be stuck in the nav-bar, it shouldn't be movable. Why? It's a common button, if I want it on the left side, or before the locationbar, I should be able to do that, provided we can respect user's will.

Because of the dependency on the menu bar and the bookmarks bar. Toolbar customization doesn't play nicely with this. You could move the button to the menu bar or to the bookmarks bar, and then what? Let's not make this more complicated than it needs to be.

>You could move the button to the >menu bar or to the bookmarks bar, and then what? In those cases wouldn't the user have to drag the bookmarks widget from the customization pallet, since it is no longer in chrome? It seems redundant to allow the user to have both, but that seems ok to me if they really want it. I've also been meaning to file bugs on getting similar buttons to expose menus (for instance for the developer menu in the Firefox button so it is easier for people to get to those commands). While those controls are just for user customization, it seems odd for some controls to be customizable and others not.

(In reply to comment #32) > >You could move the button to the > >menu bar or to the bookmarks bar, and then what? > > In those cases wouldn't the user have to drag the bookmarks widget from the > customization pallet, since it is no longer in chrome? No, the bookmarks strip would likely be in the bookmarks toolbar. (But it could be anywhere else, too, which is another reason to put the second button into the strip.)

If the user moves the button to another position that is not what we expect, the current patch does not touch it (just hide/unhide based on autohide of the menubar). As I said if possible I'd not want to duplicate the button, because I don't want to spawn another Places view. Btw, will start implementing some of the comments I got so far, the customization can be fixed at any time.

Is there something wrong with the places view system, then? The buttons would never be displayed simultaneously, so ideally there shouldn't be a perf impact. Or are you saying you'd want to avoid it merely because it's extra code? I agree that customization can be fixed at any time, if that means that it will be disabled until then.

- Addressed dao's comment: don't use broadcasters, rename button, use pure css for the menu status (actually moved the rule to /content/browser.css since it's related to the implementation and not to the theme) - Customization is handled through persisting the old index before moving the button. Before saying it could be bad please try it. It works quite well for me and should satisfy most of the users without sacrifices and without having to dupe a bunch of elements and code. Should satisfy ux requests as far as I can tell. - Addressed latest Steven's requests. - hide Personal Toolbar for new profiles - hide toolbar for profiles that have never changed default bookmarks. Doing this with a full compare would be extremely slow, I'm using a lazy compare of item types, could fail sometimes but just in edge rare cases. In such rare cases user can still use the (now even more reachable) View Bookmarks Toolbar options to bring it back. - updated to latest trunk TODO: - test on Linux and Mac (on the latter the button should always be visible) - needs icons (to distinguish from sidebar button) and final locale strings (labels & tooltip)

(In reply to comment #36) > - needs icons (to distinguish from sidebar button) I've been wondering about this.. if we were replacing widget and removing the sidebar vs adding new functionality only. Thanks for mentioning it.

Addressed last requests from Steven: - added icons to toolbar/unsorted special folders - on OSes that don't have "autohide" the button must be always visible and in the palette, but on OSX it should be in the defaultset. - tested on all tier platforms I think this is ready for review and would be really really cool to have it in the beta and gather feedback from users on how we can improve it. TODO: - change icon? Steven says the current one is perfect and it's hard to find a good replacement, could be done later. - localized strings: both this button and old sidebar button are called bookmarks, we could change old buttons to be "Bookmarks Sidebar" and "History Sidebar", but those buttons are inheriting the label from the broadcaster that is also giving the name to View > Sidebar > Bookmarks/History. So they could need an onbroadcast="override_title" (inheriting just the other attributes through <observes> would be pretty verbose). Also this one could be a second time refinement if we can't find a viable solution really soon.

Steven, if you want to sign off a ui-review feel free to.

minor change, use this.parentNode

(In reply to comment #38) > - change icon? Steven says the current one is perfect and it's hard to find a > good replacement, could be done later. Are we talking about icon that is on mockups?

(In reply to comment #41) > Are we talking about icon that is on mockups? not sure what you mean, the problem is that icon of this button is the same as icon of Bookmarks sidebar button, the only difference is that this is a type="menu" button.

Ah, you mean Bookmarks Button in Navigation Bar. Well, it has drop arrow ;)

Dao, is beta1 going to show the Firefox button by default on Vista and 7? If so we need this sooner than later.

Comment on attachment 451249 [details] [diff] [review] patch v1.3 This fails to place the button in an already-customized nav bar. There's an easy way around this: don't make it removable. I suggest you use this structure: <toolbar id="nav-bar"> <toolbaritem id="bookmarks-menu-button-container"> <toolbarbutton id="bookmarks-menu-button"/> </toolbaritem> </toolbar> <toolbar id="PersonalToolbar"> <toolbaritem id="personal-bookmarks"/> </toolbar> ... and move the button back and forth as needed between bookmarks-menu-button-container and personal-bookmarks. >-#ifdef WINCE > collapsed="true" >-#endif >+ _runOnceDefaultToolbarCollapse: function BMB_runOnceDefaultToolbarcollapse() { The first part (browser.xul) collapses the bookmarks bar for existing profiles if the user never collapsed and uncollapsed it, which seems ok. On top of that, the second part (browser-places.js) collapses the bookmarks toolbar for existing profiles if the user didn't add items but the toolbar has been uncollapsed explicitly, which seems wrong.

(In reply to comment #47) > (From update of attachment 451249 [details] [diff] [review]) > This fails to place the button in an already-customized nav bar. There's an > easy way around this: don't make it removable. Would not that make it not movable to a personalized toolbar? Also Linux wants the button in the palette but not in toolbars, and user can add it at will (until we implement autohide menu). Mac wants the button in toolbars by default, but easily removable. > The first part (browser.xul) collapses the bookmarks bar for existing profiles > if the user never collapsed and uncollapsed it, which seems ok. > > On top of that, the second part (browser-places.js) collapses the bookmarks > toolbar for existing profiles if the user didn't add items but the toolbar has > been uncollapsed explicitly, which seems wrong. Right, we care that if the user never changed toolbar contents, toolbar should be hidden regardless the fact they could have collapsed/uncollapsed the toolbar explicitly (indeed many users toggle those to learn customize functionality), so preserving uncollapsed status for users with explicit uncollapse but only default bookmarks sounds wrong as hiding toolbar for users that never customized it but have personalized it. New profiles are profiles where clearly user did not change contents too, so this catches both cases. I'll not collapse by default, on the one-time-check if the toolbar is already collapsed (by the user) won't do anything. if it's uncollapsed I'll collapse it if it has not been customized and bookmarks are expected ones. The only drawback is that on first start the toolbar will appear for a brief moment and then disappear, one-time glitch. Actually bookmarks check won't work for non-mozilla builds since they usually add their own bookmarks. Linux users are expert enough to hide the bar if they don't need it though, so i'd not care much that case. Doing the opposite would mean collapse by default, then uncollapse if the user personalized bar contents, this is doable but would require a bit more code (an explicitly uncollapsed toolbar could be collapsed if not personalized as well as a collapsed toolbar could be uncollapsed if personalized).

This should solve both of issues you pointed out, without sacrificing customize, it's even smaller than previous patch. On the run-once check it checks if PersonalToolbar is customized, if so checks if the menu-button should be added, and adds it in case. This runs just once as the toolbar hiding code. PersonalToolbar is collapsed if it's not customized and bookmarks are defaults. I have done some tests and it looks working fine, will do others tomorrow, reasking review to move on work.

(In reply to comment #48) > (In reply to comment #47) > > (From update of attachment 451249 [details] [diff] [review] [details]) > > This fails to place the button in an already-customized nav bar. There's an > > easy way around this: don't make it removable. > > Would not that make it not movable to a personalized toolbar? I don't understand your question. > Also Linux wants the button in the palette but not in toolbars, and user can > add it at will (until we implement autohide menu). Mac wants the button in > toolbars by default, but easily removable. Differentiating between Linux and OS X doesn't seem to make sense. > > > The first part (browser.xul) collapses the bookmarks bar for existing profiles > > if the user never collapsed and uncollapsed it, which seems ok. > > > > On top of that, the second part (browser-places.js) collapses the bookmarks > > toolbar for existing profiles if the user didn't add items but the toolbar has > > been uncollapsed explicitly, which seems wrong. > > Right, we care that if the user never changed toolbar contents, toolbar should > be hidden regardless the fact they could have collapsed/uncollapsed the toolbar > explicitly (indeed many users toggle those to learn customize functionality), Is that parenthesized claim based on actual data? If a user discovers this, why would he re-show the toolbar even though he doesn't use it? > so preserving uncollapsed status for users with explicit uncollapse but only > default bookmarks sounds wrong as hiding toolbar for users that never > customized it but have personalized it. Note that the lack of additional items doesn't mean the toolbar is unused either. I used the live bookmark myself and I know others who use the smart bookmark. I do think we should migrate these users, but users who've added items as well. > New profiles are profiles where clearly user did not change contents too, so > this catches both cases. I'll not collapse by default, on the one-time-check if > the toolbar is already collapsed (by the user) won't do anything. if it's > uncollapsed I'll collapse it if it has not been customized and bookmarks are > expected ones. The only drawback is that on first start the toolbar will appear > for a brief moment and then disappear, one-time glitch. > Actually bookmarks check won't work for non-mozilla builds since they usually > add their own bookmarks. Linux users are expert enough to hide the bar if they > don't need it though, so i'd not care much that case. This all seems bogus, please set the default attribute on the toolbar as this *is* our new default.

Comment on attachment 453277 [details] [diff] [review] patch v1.4 Does this handle the bookmarks toolbar item being moved to a different toolbar? I suppose it doesn't. See my suggestion from comment 47 for how I think the structure should look like. There's no fiddling with the defaultsets and currentsets needed for that.

(In reply to comment #50) > > Would not that make it not movable to a personalized toolbar? > > I don't understand your question. if the user creates his own toolbar and wants to add the button there, I don't see why it should not work. > Differentiating between Linux and OS X doesn't seem to make sense. It is the request I got, and I agree with it because OS X has menu detached from the window, while common Linux managers don't (even if IIRC somebody is moving to a common menu) should > > be hidden regardless the fact they could have collapsed/uncollapsed the toolbar > > explicitly (indeed many users toggle those to learn customize functionality), > > Is that parenthesized claim based on actual data? If a user discovers this, why > would he re-show the toolbar even though he doesn't use it? Regardless how many times it has been collapsed/uncollapsed, if the bar is not customized and bookmarks are defaults I don't see the point of not hiding it. My example was just something I've seen in years, I don't have numbers. But having an explicitly uncollapsed bar with default bookmarks is an edge-case, the correct operation imo is to collapse, if we do wrong for some user the solution is at a click distance. Both ways we could take wrong decision for some user, in this case we could take wrong decision for a user that knows how to uncollapse the bar since he already uncollapsed it. > Note that the lack of additional items doesn't mean the toolbar is unused > either. I used the live bookmark myself and I know others who use the smart > bookmark. I do think we should migrate these users, but users who've added > items as well. The request is to hide toolbar by default, and for user with default bookmarks, there is no way to differentiate between users who like Latest Headlines and user who don't, plus there is also a request to replace Latest Headlines with a different feed (See newsgroup), and to remove all default entries from the bookmarks toolbar (There is a post from Alex about this). Still, we are talking about something that can be unhidden in seconds with a click on the "View Bookmarks Toolbar" from the new button, it is easily discoverable. > This all seems bogus, please set the default attribute on the toolbar as this > *is* our new default. if I do that I have to add a bunch of additional checks in the code, but if the initial collapsing is unwanted I'll do (In reply to comment #51) > (From update of attachment 453277 [details] [diff] [review]) > Does this handle the bookmarks toolbar item being moved to a different toolbar? Yes, if the button is moved to a toolbar that is not the expected one I don't touch it. Actually when an update request is done, if the bookmarks toolbar has been collapsed and button was on it, it is moved to navbar. If bookmarks toolbar has been uncollapsed and button is in navbar it is moved to bookmarks toolbar. If none of this situations happen, nothing is changed.

This addresses the collapsed by default comment. I cover user choice checking the value persisted in localstore.rdf. I overwrite user choice only if everything is untouched (no customized and default bookmarks). I clarified comments to make it easier to follow. TODO: - check existing tests, some of them could rely on a visible personal toolbar. update it. - make a new b-c test for the button.

Would it be possible to address https://bugzilla.mozilla.org/show_bug.cgi?id=501044 and https://bugzilla.mozilla.org/show_bug.cgi?id=501046 when fixing this bug (if appropriate?)? ~B

(In reply to comment #54) > Would it be possible to address > https://bugzilla.mozilla.org/show_bug.cgi?id=501044 and > https://bugzilla.mozilla.org/show_bug.cgi?id=501046 when fixing this bug nope, that involves a bunch of other code, so it's just too risky, sorry.

fixed 2 b-c tests, I've run all remaining b-c, m-p, m-c tests and have not seen any other failure.

(In reply to comment #52) > > Differentiating between Linux and OS X doesn't seem to make sense. > > It is the request I got, and I agree with it because OS X has menu detached > from the window, while common Linux managers don't Why is this relevant, given that most users browse in maximized windows? > (even if IIRC somebody is moving to a common menu) It's in the Gnome 3 feature list and planned for the next Ubuntu release, I think. > should > > > be hidden regardless the fact they could have collapsed/uncollapsed the toolbar > > > explicitly (indeed many users toggle those to learn customize functionality), > > > > Is that parenthesized claim based on actual data? If a user discovers this, why > > would he re-show the toolbar even though he doesn't use it? > > Regardless how many times it has been collapsed/uncollapsed, if the bar is not > customized and bookmarks are defaults I don't see the point of not hiding it. Because the user may be using it. I don't see why we should try to be smarter than the user when he discovered that he can hide toolbars but explicitly uncollapsed the personal toolbar. > > Does this handle the bookmarks toolbar item being moved to a different toolbar? > > Yes, if the button is moved to a toolbar that is not the expected one I don't > touch it. I was referring to the bookmarks toolbar item, not the button.

(In reply to comment #57) > Why is this relevant, given that most users browse in maximized windows? Is that true? I know I sure don't, and I've seen lots of other people who don't too. Where are you getting this data?

(In reply to comment #58) > (In reply to comment #57) > > Why is this relevant, given that most users browse in maximized windows? > Is that true? I know I sure don't, and I've seen lots of other people who > don't too. Where are you getting this data? Watching random people. It very likely depends on the screen size.

(In reply to comment #58) > (In reply to comment #57) > > Why is this relevant, given that most users browse in maximized windows? > Is that true? I know I sure don't, and I've seen lots of other people who > don't too. Where are you getting this data? Considering that tabs will be in title bar when windows is maximized, I think you will see more people whit mnaximized Firefox.

Ok, let's stop bringing experience as facts, I'm staying adherent to UX requests, we can discuss with them and they can change at any time, but getting feedback from beta1 on things UX already discussed a lot sounds like more important than trying to demonstrate who is right :) (In reply to comment #57) > Because the user may be using it. I don't see why we should try to be smarter > than the user when he discovered that he can hide toolbars but explicitly > uncollapsed the personal toolbar. This change is pretty easy in current patch (a one-line change). But the question is, why should they collapse the toolbar, uncollapse it and then never edit it? just for using a feed that is going to be changed/removed soon? I've seen a lot of persons trying to open close toolbars just to see what was that "Bookmarks toolbar" menuitem, and they usually restore it to what it was before because they don't know what it is for. As I said, I don't want to bring my experience as a fact, so I'll ask Stephen what they think is better. > > > Does this handle the bookmarks toolbar item being moved to a different toolbar? > I was referring to the bookmarks toolbar item, not the button. Oops, well I use document.getElementById("PlacesToolbarItems") to check children, that is not dependent on its position. And if the bar is customized I add the button to the end. Do you see other possible problems apart those regarding position of bookmark-items?

(In reply to comment #61) > Ok, let's stop bringing experience as facts, I'm staying adherent to UX > requests, we can discuss with them and they can change at any time, but getting > feedback from beta1 on things UX already discussed a lot sounds like more > important than trying to demonstrate who is right :) > > (In reply to comment #57) > > Because the user may be using it. I don't see why we should try to be smarter > > than the user when he discovered that he can hide toolbars but explicitly > > uncollapsed the personal toolbar. > > This change is pretty easy in current patch (a one-line change). But the > question is, why should they collapse the toolbar, uncollapse it and then never > edit it? just for using a feed that is going to be changed/removed soon? > I've seen a lot of persons trying to open close toolbars just to see what was > that "Bookmarks toolbar" menuitem, and they usually restore it to what it was > before because they don't know what it is for. > As I said, I don't want to bring my experience as a fact, so I'll ask Stephen > what they think is better. If they have collapsed/uncollapsed it before then it could have been accidental. If it wasn't accidental then they know how to turn it back on and can do so. We should go forward with the idea that if they haven't modified it then they likely aren't using it and it is wasting space and adding complexity. Feedback can be gathered on that approach.

(In reply to comment #61) > > Because the user may be using it. I don't see why we should try to be smarter > > than the user when he discovered that he can hide toolbars but explicitly > > uncollapsed the personal toolbar. > > This change is pretty easy in current patch (a one-line change). I'm actually flirting with getting rid off all of _checkOnceCustomizedToolbars. > But the > question is, why should they collapse the toolbar, uncollapse it and then never > edit it? just for using a feed that is going to be changed/removed soon? Because the live bookmark or the smart bookmark. I would assume the smart bookmark to be more popular. > I've seen a lot of persons trying to open close toolbars just to see what was > that "Bookmarks toolbar" menuitem, and they usually restore it to what it was > before because they don't know what it is for. Right, but then they know what it's for, and keep the toolbar. Seems like an informed decision to me. > > > > Does this handle the bookmarks toolbar item being moved to a different toolbar? > > I was referring to the bookmarks toolbar item, not the button. > > Oops, well I use document.getElementById("PlacesToolbarItems") to check > children, that is not dependent on its position. That's not what I meant. I'm pretty sure that if the menubar is hidden and the bookmarks bar item is visible /anywhere/, we want the button to appear exactly there, hence my suggestion from comment 47.

(In reply to comment #62) > If they have collapsed/uncollapsed it before then it could have been > accidental. If it wasn't accidental then they know how to turn it back on and > can do so. > > We should go forward with the idea that if they haven't modified it then they > likely aren't using it and it is wasting space and adding complexity. Feedback > can be gathered on that approach. This case doesn't sound like it will play a big role among beta testers.

I'd expect _checkOnceCustomizedToolbars to screw up with multiple windows as well...

(In reply to comment #63) > I'm actually flirting with getting rid off all of _checkOnceCustomizedToolbars. even if I'd be happy about that it's hard without doing half of the needed work > That's not what I meant. I'm pretty sure that if the menubar is hidden and the > bookmarks bar item is visible /anywhere/, we want the button to appear exactly > there, hence my suggestion from comment 47. Toolbars are customized in such a case, and since personal toolbar is not visible I expect button to be in the navigation bar. this button is not a bookmark, it's a personal widget (thus it belongs to PersonalToolbar imo). Your suggestion hits against design there. (In reply to comment #65) > I'd expect _checkOnceCustomizedToolbars to screw up with multiple windows as > well... This could happen only if sessionstore restores multiple windows, right? I just tested that and it works pretty fine, it runs before next windows are open.

(In reply to comment #66) > > I'm actually flirting with getting rid off all of _checkOnceCustomizedToolbars. > > even if I'd be happy about that it's hard without doing half of the needed work Well, except that I don't think collapsing an explicitly uncollapsed toolbar is exactly "needed work". > > That's not what I meant. I'm pretty sure that if the menubar is hidden and the > > bookmarks bar item is visible /anywhere/, we want the button to appear exactly > > there, hence my suggestion from comment 47. > > Toolbars are customized in such a case, and since personal toolbar is not > visible I expect button to be in the navigation bar. this button is not a > bookmark, it's a personal widget (thus it belongs to PersonalToolbar imo). Your > suggestion hits against design there. The design simply doesn't take that case into account at all, so implementing it blindly doesn't seem wise. I daresay it doesn't make any sense for the button to be in the navigation toolbar in that case. > (In reply to comment #65) > > I'd expect _checkOnceCustomizedToolbars to screw up with multiple windows as > > well... > > This could happen only if sessionstore restores multiple windows, right? I just > tested that and it works pretty fine, it runs before next windows are open. What makes you believe it doesn't race?

> What makes you believe it doesn't race? the fact you did not describe what you think should happen (so it's hard for me to guess where to hook), and everything works fine when using the browser after a start.up with multiple windows that involves the init method. When the method runs there is only 1 window open, next windows find everything setup.

Two windows open up more or less in parallel with the original localstore data, one of them executes _checkOnceCustomizedToolbars and changes defaults, the other window misses them.

I don't understand, that code is not threaded afaik, first window opens, function runs (notice the pref is set immediately so next runs of the same method can't race with it), other windows open, other windows are opened by a js method in sessionstore that runs enqueued. But even if that could happen, that's a case that would happen if: - is first start with this patch - the session has multiple browser windows - we have to change something (so we are in a case where we have to uncollapse toolbar or toolbars are highly customized) - things run so that windows opens parallel (I don't understand how) and method changes values exactly after second window opens or start to open Even if this extremely rare case happens we will just don't recustomize browser (don't uncollapse or don't add the button), things the user can do easily (and in such a case it's not exactly a newbie since something is customized)

(In reply to comment #70) > I don't understand, that code is not threaded afaik, first window opens, > function runs (notice the pref is set immediately so next runs of the same > method can't race with it), other windows open, other windows are opened by a > js method in sessionstore that runs enqueued. What does enqueued exactly mean? Note that you're calling your stuff from delayedStartup. I'd be surprised if sessionstore waits for this.

sessionrestore start restore in delayed startup as well, this is the first part of delayed startup, that is delayed by 0. From all my tests windows restore always runs after this piece of code (just check when default browser request comes out for example)

(In reply to comment #72) > sessionrestore start restore in delayed startup as well, this is the first part > of delayed startup, that is delayed by 0. You're saying the first delayedStartup causes sessionstore to open subsequent windows?

afaict (I also asked in fx-team) it starts sessionrestore and subsequently restores windows (and that restore happens enqueued from what I saw phisically trying it). Btw, if you're worried I can move Places init methods before sessionrestore is inited in delayedStartup.

I suggest you take _checkOnceCustomizedToolbars to a followup bug and let Gavin or Dietrich review it... I'd rather not touch this hairy beast, if I may say so...

your patch does not allow to move the button and does not handle toolbar uncollapsing. So for example if I have hundreds of bookmarks on the bar and I never collapsed/uncollapsed it, it is disappearing.

I think this is the wrong approach when we have a patch ready that does all of that, and does it well. This will make lots of beta users sad. But I'm going on because beta is more important than what I feel, will check your patch tomorrow and attach a b-c test, then land. Other stuff will go to followups.

(In reply to comment #76) > your patch does not allow to move the button Right, this seems like a nice-to-have, not really a requirement for this bug. > and does not handle toolbar > uncollapsing. So for example if I have hundreds of bookmarks on the bar and I > never collapsed/uncollapsed it, it is disappearing. This is similar to users using the smart bookmark without ever having collapsed/uncollapsed the toolbar. Anyway, this is why I recommended a spinn-off bug. I don't feel very strongly about where to draw the line, but _checkOnceCustomizedToolbars seemed ugly enough to not give it the benefit of the doubt. (In reply to comment #77) > I think this is the wrong approach when we have a patch ready that does all of > that, and does it well. I wouldn't say it did it well. In particular making the button movable but then caring only for the two toolbars seems strange and may lead to unpredictable behavior. You also said yourself that the check for the default bookmarks was fragile.

(In reply to comment #78) > (In reply to comment #76) > > your patch does not allow to move the button > > Right, this seems like a nice-to-have, not really a requirement for this bug. it was a UX team requirement like other things you removed, and I can't see how a nice-to-have is not accepted if the patch is ready and working. > _checkOnceCustomizedToolbars seemed ugly enough to not give it the benefit of > the doubt. Where is the ugly part? using rdf service? > I wouldn't say it did it well. In particular making the button movable but then > caring only for the two toolbars seems strange and may lead to unpredictable > behavior. I have not found any unpredictable behavior, nor I see examples reported. > You also said yourself that the check for the default bookmarks was > fragile. There was no alternative, that is a bit different, I was handling the case for major of users (excluded linux custom builds), that was a better alternative than not handling it for anyone.

hm, wait your patch is also removing unsorted bookmarks, show in sidebar and view bookmarks toolbar, you're not following ANY mockup.

(In reply to comment #79) > (In reply to comment #78) > > (In reply to comment #76) > > > your patch does not allow to move the button > > > > Right, this seems like a nice-to-have, not really a requirement for this bug. > > it was a UX team requirement like other things you removed, and I can't see how > a nice-to-have is not accepted if the patch is ready and working. Because I think it wasn't ready and working. > > _checkOnceCustomizedToolbars seemed ugly enough to not give it the benefit of > > the doubt. > > Where is the ugly part? using rdf service? That, and the fiddling with the currentSet (the currentSet getter is going to contain bogus data if extensions moved non-customizable elements in the toolbar; adblock plus had this problem with the splitter between the location and search bars), and the check for the default bookmarks. > > I wouldn't say it did it well. In particular making the button movable but then > > caring only for the two toolbars seems strange and may lead to unpredictable > > behavior. > > I have not found any unpredictable behavior, nor I see examples reported. See the case where the bookmarks toolbar item would be moved to a different toolbar, for instance.

(In reply to comment #80) > hm, wait your patch is also removing unsorted bookmarks, show in sidebar and > view bookmarks toolbar, you're not following ANY mockup. I fail to spot any of this in attachment 425749 [details]. It's not there in the Bookmarks menu either.

I have an updated mockup and discussed every point with Stephen, since we were following this bug, we met and discussed every point, he sent me a list of points to implement, that is what I did, my patch is 100% current mockup discussed with Stephen. your patch is failing these points from the current requests: - "Show Sidebar" item - "Show Bookmarks Toolbar" item - it should appear at the end of whatever is on the toolbar. But if a user manually moves it then it should just stay put regardless of other variables. - In the palette on Linux - toolbar collapsed only if user never edited its contents I briefly discussed about unsorted menu and he agreed, but I did not update old menu because we still had to discuss if we were going to change it (users have an habit on it).

Where is the modified/updated mockup?

I'd like to see it as well.

Then it would have been appropriate to update the bug with that stuff (unless I missed it, but I think I didn't). Accusing me of "not following ANY mockup" is bogus when I did in fact follow the mockup that's available. To unblock beta 1 I suggest separate bugs be filed on adding more items to the menu.

From this mockup we discussed position of the entries, we decided to put them before bookmarks because otherwise they'd scroll out of view with long menus. Then we discussed unsorted bookmarks. Then we discussed pin to sidebar, and decided to go for Show sidebar because otherwise the button would flicker everytime sidebar gets opened/closed. I've pinged and asked on every single detail.

Pretending Stephen updates a mockup everyday just to post it to the bug is crazy, this is a project with a developer (me) a ux team member (stephen), we met and we discuss details, then I implement and he checks if results are satisfying, then we move to review. Pretending to know all the work and discussions that are behind current stuff is out of discussion.

your patch is also putting the button at the start of bookmarks toolbar while it should be at the end

I'll provide an updated version of your patch with correct position and correct popup entries, and re-ask review. then I'll split followups for all the other stuff. I don't see any other solution to exit this crazy tunnel.

bugzilla should be able to interdiff this with 1.6, if not let me know I'll generate one.

(In reply to comment #91) > bugzilla should be able to interdiff this with 1.6 I meant with "review in patch form"

(In reply to comment #88) > Pretending Stephen updates a mockup everyday just to post it to the bug is > crazy, this is a project with a developer (me) a ux team member (stephen), And a reviewer. I had no idea why your patch did what it did, which is obviously bad.

(In reply to comment #93) > I had no idea why your patch did what it did, which is obviously bad. my fault, I should have sum up discussions with UX in small chunks.

Comment on attachment 453697 [details] [diff] [review] patch v1.7 >--- a/browser/base/content/browser.xul >+++ b/browser/base/content/browser.xul >+ <toolbaritem id="bookmarks-menu-button-container" Oh, for correctness please add this to the nav-bar's defaultset attribute, although it may not be really needed.

Comment on attachment 453711 [details] [diff] [review] patch v1.8 oops, I added it in the wrong position

Status whiteboard says this can land ... will it? :)

(In reply to comment #99) > Status whiteboard says this can land ... will it? :) sure http://hg.mozilla.org/mozilla-central/rev/f7b40005cc19

Is there a bug regarding allowing the user to choose the behavior? I want it to never be in the bookmarks bar since my navigation bar has a lot of unused space on the account of not having the search bar.

I'll file follow-ups for things that were in original patch and not in what we pushed, that includes customize-ability of the button.

(In reply to comment #102) > I'll file follow-ups for things that were in original patch and not in what we > pushed, that includes customize-ability of the button. Good that I check this thread first... just about to file a bug for not being able to hide (customize) it. ;)

this is a subset of the test I was writing. Will do a tryserver run just to check is fine for all platforms. No hurry here.

This change requires a lot of Litmus tests updates now that the bookmarks toolbar isn't shown per default. CC'ing Tracy who owns that area.

I feel we do not need this widget on Mac OS X. The menu bar and "Bookmarks" is always visible. We're just duplicating the same functions.

I have the bookmarks toolbar shown, but the bookmark widget shows up again on each restart. Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; de; rv:1.9.3a6pre) Gecko/20100627 Minefield/3.7a6pre

(In reply to comment #107) > I have the bookmarks toolbar shown, but the bookmark widget shows up again on > each restart. Not sure I get the issue, the widget is always shown, if bookmarks toolbar is visible it's on bookmarks toolbar, if not it's on navigation bar

screenshot showing how the widget is actually in the navigation bar after restart for me.

Is your bookmark toolbar a custom toolbar with the bookmark-items element added to it, as opposed to the actual bookmarks toolbar?

I get the same, Bookmarks Toolbar showing and still get the widget.

I'm pretty sure it's the default bookmarks toolbar, here's what DOMI shows. Also, the widget moves to the bookmarks toolbar like expected if I hide the toolbar and show it again, but on restart, it pops back into the navigation bar.

I have just tested removing the Bookmarks Toolbar, and then enabling it again. The widget is not shown, however when you open a new window or restart the app, you get both the widget and toolbar together.

It seems be ok on Win7.. is the issue just on OSX? On Windows: The bookmarks toolbar shows the bookmarks toolbar widget. The Navigation toolbar shows the Navigation Bookmarks Widget. When the bookmarks toolbar is shown the widget shows on that bar, when the bookmarks toolbar is not show, its just on the nav bar.

The auto-created bookmark button seems to work fine if the personal-bookmark widget is on the bookmark toolbar. However, I've placed the personal-bookmark widget on the MENUbar for easy recall and dismissal with the ALT key. With the widget on the menubar, all hell breaks loose and there is no telling where the bookmark button will or will not pop up next especially on new windows or after a restart. See bug 575194

(In reply to comment #115) > The auto-created bookmark button seems to work fine if the personal-bookmark > widget is on the bookmark toolbar. This shouldn't matter for this patch, becase it moves the button inside the bookmarks-items element and not to the personal toolbar. Unless clearly there are issues when having that element in other places. My automatic test fails to detect if bookmarks toolbar is collapsed on Mac, sounds like a bug of isElementVisible function? Maybe it should use getBoundingClientRect() instead of relying on boxObject.

I have filed Bug 575218 for the button issue on Mac.

Comment on attachment 454488 [details] [diff] [review] test v1.1 r=me