The treeview displayed after hitting the progressive disclosure control for folders should be left aligned with the other fields, as opposed to the labels. The same goes for the listview under the progressive disclosure control for tags. Mockup available at: http://people.mozilla.com/~faaborg/files/granParadisoUI/places_NewBookmarkDialog_i9.png

Something which could be fixed for Firefox 3.1?

Yes, but it will not block the release.

Alex, do we have a tracking bug for interface work on 3.1?

>Alex, do we have a tracking bug for interface work on 3.1? not outside of the blocking‑firefox3.1 and wanted‑firefox3.1 lists. I have my own list of polish bugs that I want us to eventually target, but that isn't tied to any specific release.

this bug is eligible for bug 462081

i have an half-made patch for this, need to test it on Linux and Mac (harder for me without a Mac, will try hwv), will apply upon bug 411261

tested on XP, Vista, Ubuntu Needs fixes for Pinstripe still and tagging ui patch in

Marco, if you need OS X testing then create a tryserver build. I could have a look at this.

i'm already creating one... More than testing this probably needs some minor fix on Mac since pinstripe is the only theme providing styling on most bookmarks dialogs

i think much stuff on mac side needs fixing, but while there henrik could you take a look at functionality and aspect on other platforms too?

Marco: Feel feel to ping me or henrik if you need mac testing - I have plenty of mac hardware here to test on in Mountain View.

I have the following in my userchrome.css to make the size of the bookmarks panel more usable. With this try server build on XP the width of the name, folder and tags text boxes now no longer line up even closely. CODE: /* Increase initial size of bookmarks panel */ #editBookmarkPanelContent { min-width:410px !important; } /* Increase size of the expanded bookmarks panel */ #editBMPanel_folderTree { min-width:400px !important; min-height:400px !important; }

(In reply to comment #13) > I have the following in my userchrome.css to make the size of the bookmarks > panel more usable. With this try server build on XP the width of the name, > folder and tags text boxes now no longer line up even closely. i doubt we can support lining up with all custom userchromes... hwv will see if i can do something about that

Thanks Marcia. actually this approach can't be done on Mac, since in the contextual dialog these elements take up all the width. Pinstripe is much different than other themes here, so we should ifdef everything and let the old behaviour on Mac, or add a bunch of margin to elements instead of aligning them through the grid :\

Just to let you know according to https://bugzilla.mozilla.org/show_bug.cgi?id=411261#c59 this bug will also cover the alignment of the tree & list views in the modified bookmark properties dialog that contains the tagging UI. The try server builds containing the new properties dialog are in comment 45 in that bug.

Because we want to use the same code for all of the various types of windows, here is an alternative alignment on OS X for the star panel. We would probably want to make the background for the panel and the tree area the same, with a thin white line around the tree area. We could clean this up in bug 462651.

(In reply to comment #17) > Because we want to use the same code for all of the various types of windows, Which types of windows are you referring to?

(In reply to comment #18) > (In reply to comment #17) > > Because we want to use the same code for all of the various types of windows, > > Which types of windows are you referring to? I suppose star panel and bookmarks dialogs.

>Which types of windows are you referring to? my comment was from an irc discussion with Marco, he recently informed me that we actually have 15 (!) bookmark windows, details in bug 459958

this is the better align i could get using two nested grids and setting a min-width for the panel (will be useful also if we're going to make it resizeable). I had to reorder fields and put the folderTree as the last element though. The tag selector is aligned to fields, while folderTree is taking the full dialog (notice we don't want to make it smaller than this since it would be really hard managing folders of folders). Alex, could this be acceptable?

fixed some glitch... only remaining issue is Mac fields (tagfield and folder menulist) being fat, but i have problems debugging them without a Mac.

Marco: If it would help, you can spin up a tryserver build, and I can see what those fields look like on Mac. (In reply to comment #23) > Created an attachment (id=349218) [details] > wip 0.2 > > fixed some glitch... only remaining issue is Mac fields (tagfield and folder > menulist) being fat, but i have problems debugging them without a Mac.

The different widths from the tag expanded list view and the folders tree view is pretty strange. Ideally these would all line up on the left (aligned to the end of the label instead of the start), and we could make the panel as a whole wider when the user expands either progressive disclosure control.

Comment on attachment 348978 [details] screen See comment above.

Marco, do you plan on updating this?

Sure, i want to revise some change to implement what Alex proposed (all aligned to the right and the panel enlarging on foldertree open), and invert grids ids to avoid a css change that could ideally hit themes (i really doubt but better be paranoid). I'll do next week probably.

panel width increases by 1.5 when opening the folderTree, plus fixes for menulist icons margins. Tested on XP only for now, i'll generate some trybuild and test on other platforms. Alex, i think it does not matter enlarging the panel for Tags selector, they are not hierarchic, and unless a user adds some really long tag, there's no gain in doing that.

Marco, can we have a better alignment on the right side? The drop markers are positioned a bit outside of the alignment line.

Comment on attachment 355120 [details] screenshot 0.4 This is a great improvement. We might want to make some additional changes with padding on buttons, and the size of buttons, but we can do that in a follow up, and shouldn't delay getting this in.

this solves the issues i found with previous versions, and Henrik's comment 33. Tested on Linux, PPC Mac, XP and Vista. Hwv i've started generating new trybuilds, so if Marcia and /or Henrik could help testing them would be really appreciated. As soon as trybuilds are tested i'll go for review.

trybuilds will appear here: https://build.mozilla.org/tryserver-builds/2009-01-07_01:32-mak77@bonardo.net-try-8bb533ac94a/

Marco, I've taken some time to check this tryserver build on OS X. Following issues I've noticed: * Clicking the expanding button increases the height of the folder tree before the width of the dialog is applied. That looks a bit odd. IMO it should happen at the same. * The star panel is totally miss-placed. It's not under the star button but shifted around 30px to the left * Each time you open the dialog and the folder tree the dialog gets wider and wider until it reaches both sides of the screen. Tip: to reproduce close the dialog while it is enlarged. * Shift+Tab doesn't work. You get stuck in the name text field * Expand/Collapse Buttons: Everytime I use keyboard navigation to go through the controls I hit the Enter key while trying to open the folder or tag list. And each time the dialog closes. :/ I believe that will happen for a lot of other people. That's all for now.

(In reply to comment #37) > * Clicking the expanding button increases the height of the folder tree before > the width of the dialog is applied. That looks a bit odd. IMO it should happen > at the same. that could be a bit tricky because those are 2 different actions, the first is an uncollapse, the second is a width change. > * The star panel is totally miss-placed. It's not under the star button but > shifted around 30px to the left This is bug 471865 i fear, did you test on Mac? > * Each time you open the dialog and the folder tree the dialog gets wider and > wider until it reaches both sides of the screen. Tip: to reproduce close the > dialog while it is enlarged. true, will fix next version > * Shift+Tab doesn't work. You get stuck in the name text field WFM on Windows, which site? does it have microsummaries? :\ > * Expand/Collapse Buttons: Everytime I use keyboard navigation to go through > the controls I hit the Enter key while trying to open the folder or tag list. > And each time the dialog closes. :/ I believe that will happen for a lot of > other people. this happens on current trunk too, so i think you should file a new bug for it :\

(In reply to comment #38) > that could be a bit tricky because those are 2 different actions, the first is > an uncollapse, the second is a width change. So it's not possible to have an auto width here? Or doesn't it matter and the same will happen? > This is bug 471865 i fear, did you test on Mac? Yeah. That's bug 471865. > > * Shift+Tab doesn't work. You get stuck in the name text field > > WFM on Windows, which site? does it have microsummaries? :\ Did that on http://crash-stats.mozilla.com/topcrasher/byversion/Firefox/3.0.2pre > > * Expand/Collapse Buttons: Everytime I use keyboard navigation to go through > > the controls I hit the Enter key while trying to open the folder or tag list. > > And each time the dialog closes. :/ I believe that will happen for a lot of > > other people. > > this happens on current trunk too, so i think you should file a new bug for it So this is not expected and we want to user Enter for the open/close action?

(In reply to comment #39) > (In reply to comment #38) > > that could be a bit tricky because those are 2 different actions, the first is > > an uncollapse, the second is a width change. > > So it's not possible to have an auto width here? Or doesn't it matter and the > same will happen? i could try setting a bigger width to the folder tree before uncollapsing, not sure it will work as expected though. > > > * Shift+Tab doesn't work. You get stuck in the name text field > > > > WFM on Windows, which site? does it have microsummaries? :\ > > Did that on > http://crash-stats.mozilla.com/topcrasher/byversion/Firefox/3.0.2pre Still not reproducible on Win. > > > * Expand/Collapse Buttons: Everytime I use keyboard navigation to go through > > > the controls I hit the Enter key while trying to open the folder or tag list. > > > And each time the dialog closes. :/ I believe that will happen for a lot of > > > other people. > > > > this happens on current trunk too, so i think you should file a new bug for it > > So this is not expected and we want to user Enter for the open/close action? I'm not sure if that's expected or not (Alex is probably a better target for the question), Enter is some sort of "confirm" that you have finished editing, and probably on the expander you should use the space bar. like Esc is for cancel current changes. So the best bet is probably to file a bug and let UX team choice to confirm or wontfix it.

slightly different approach, instead of using a listener i'm setting a min-width on the folderTree, the panel auto enlarges when it's visible. Should solve both panel growing and folderTree opened in 2 steps. I'll generate new trybuilds to test.

v1.1 trybuilds: https://build.mozilla.org/tryserver-builds/2009-01-07_13:11-mak77@bonardo.net-try-ee978384989/

Comment on attachment 355839 [details] [diff] [review] patch v1.1 tested on all 4 platforms, asking first review to Dao

That looks better. All the remaining issues from my comment 37 are covered by other bugs. Marco, can we enhance the timing for expanding the folder view? There is a 0.5s lag until the dialog is resized. Collapsing is much faster.

that lag is probably due to some underlying operation, i did not add any "artificial" delay.

This looks great, thanks Marco!

(In reply to comment #45) > that lag is probably due to some underlying operation, i did not add any > "artificial" delay. Shall I file it immediately as a new bug?

better waiting to fix this, and make that a followup to investigate causes.

Comment on attachment 355839 [details] [diff] [review] patch v1.1 >+#editBookmarkPanelGrid > row[collapsed="true"] { > display: none; > } Uh, what's that?

(In reply to comment #49) > (From update of attachment 355839 [details] [diff] [review]) > >+#editBookmarkPanelGrid > row[collapsed="true"] { > > display: none; > > } > > Uh, what's that? even if collapsed rows are taking up space, plus the folderTree row is larger (to force the panel to enlarge) and should not be displayed.

So why are we using @collapsed rather than @hidden?

while i did not originally create the panel, all its code uses collpased, hwv i don't think we need to know the position of the elements even if they are collapsed, maybe it's faster for tabular views like grids... i could maybe obtain the same result with negative top/bottom margins on the collapsed element.

(In reply to comment #52) > while i did not originally create the panel, all its code uses collpased, hwv i > don't think we need to know the position of the elements even if they are > collapsed, And even if you did, you wouldn't get the positions, due to display:none. That's identical to what @hidden does: http://mxr.mozilla.org/mozilla-central/source/toolkit/content/xul.css#23

you're right, i'll try alternate ways.

or better, i'll remove that rule at all since it was addressing an issue i had before changing the panel code, argh. New patch (unbitrotted too) coming.

removed that stupid useless rule, thanks Dao!

Comment on attachment 356166 [details] [diff] [review] patch v1.2 >+++ b/browser/components/places/content/editBookmarkOverlay.xul >+ <hbox align="center" id="editBMPanel_selectionCount" hidden="true"> >+ <vbox id="editBMPanel_itemsCountBox" align="center"> >+ <label id="editBMPanel_itemsCountText"/> >+ </vbox> >+ </hbox> What are these boxes doing? >+ <vbox> >+ <button id="editBMPanel_foldersExpander" >+ class="expander-down" >+ tooltiptext="&editBookmarkOverlay.foldersExpanderDown.tooltip;" >+ tooltiptextdown="&editBookmarkOverlay.foldersExpanderDown.tooltip;" >+ tooltiptextup="&editBookmarkOverlay.expanderUp.tooltip;" >+ oncommand="gEditItemOverlay.toggleFolderTreeVisibility();" >+ observes="paneElementsBroadcaster"/> >+ </vbox> What's the point of that vbox? Same question for editBMPanel_tagsSelectorExpander. >+ <row align="center" id="editBMPanel_folderTreeRow" collapsed="true"> >+ <label/> This looks like it should be a spacer, not a label. Same in editBMPanel_tagsSelectorRow. >+ <hbox id="editBMPanel_newFolderBox"> >+ <button label="&editBookmarkOverlay.newFolderButton.label;" >+ id="editBMPanel_newFolderButton" >+ accesskey="&editBookmarkOverlay.newFolderButton.accesskey;" >+ oncommand="gEditItemOverlay.newFolder();"/> >+ <spacer flex="1"/> >+ </hbox> Why the spacer? You may want to add pack="start" to the hbox, although I think even that could be redundant. >+++ b/browser/themes/gnomestripe/browser/browser.css >+#editBookmarkPanel #editBMPanel_folderTree { >+ min-width: 300px; >+} Remove #editBookmarkPanel from that selector? I don't think there's any other #editBMPanel_folderTree that browser.css could style. pinstripe also has some similar selectors. >+++ b/browser/themes/gnomestripe/browser/places/editBookmarkOverlay.css >+#editBMPanel_folderTree { >+ margin: 2px 4px; > } The horizontal margin is already there, I think, so margin-top/bottom: 2px should be sufficient. Same for winstripe. >+++ b/browser/themes/pinstripe/browser/places/editBookmarkOverlay.css >+#editBMPanel_folderTree { >+ margin: 6px 4px 0px 4px; > } 0 instead of 0px, please. >+++ b/browser/themes/winstripe/browser/places/editBookmarkOverlay.css >@@ -52,34 +52,38 @@ > > > /**** expanders ****/ > > .expander-up, > .expander-down { > min-width: 0; > margin: 0; >+ margin-right: 4px; -moz-margin-end?

(In reply to comment #57) > (From update of attachment 356166 [details] [diff] [review]) > >+++ b/browser/components/places/content/editBookmarkOverlay.xul > > >+ <hbox align="center" id="editBMPanel_selectionCount" hidden="true"> > >+ <vbox id="editBMPanel_itemsCountBox" align="center"> > >+ <label id="editBMPanel_itemsCountText"/> > >+ </vbox> > >+ </hbox> > > What are these boxes doing? we had those ids before and i wanted to preserve them since we should try to not do changes that could hit themes at this point. So since we were wrongly using a row before, i changed it to an hbox > > >+ <vbox> > >+ <button id="editBMPanel_foldersExpander" > >+ class="expander-down" > >+ tooltiptext="&editBookmarkOverlay.foldersExpanderDown.tooltip;" > >+ tooltiptextdown="&editBookmarkOverlay.foldersExpanderDown.tooltip;" > >+ tooltiptextup="&editBookmarkOverlay.expanderUp.tooltip;" > >+ oncommand="gEditItemOverlay.toggleFolderTreeVisibility();" > >+ observes="paneElementsBroadcaster"/> > >+ </vbox> > > What's the point of that vbox? Same question for > editBMPanel_tagsSelectorExpander. practically, the expanders take the full height of the grid's row, that would not be an issue generally, but on win Vista (for example) the menulist is higher than normal text fields. So as soon as an element with an expander is higher the expander becomes stretched. This stretch the vbox instead. > > >+ <row align="center" id="editBMPanel_folderTreeRow" collapsed="true"> > >+ <label/> > > This looks like it should be a spacer, not a label. Same in > editBMPanel_tagsSelectorRow. ok, i didn't know if a label here was needed for accessibility reasons, i'll change it > > >+ <hbox id="editBMPanel_newFolderBox"> > >+ <button label="&editBookmarkOverlay.newFolderButton.label;" > >+ id="editBMPanel_newFolderButton" > >+ accesskey="&editBookmarkOverlay.newFolderButton.accesskey;" > >+ oncommand="gEditItemOverlay.newFolder();"/> > >+ <spacer flex="1"/> > >+ </hbox> > > Why the spacer? You may want to add pack="start" to the hbox, although I think > even that could be redundant. i'll check, that code was already there iirc > >+++ b/browser/themes/gnomestripe/browser/browser.css > > >+#editBookmarkPanel #editBMPanel_folderTree { > >+ min-width: 300px; > >+} > > Remove #editBookmarkPanel from that selector? I don't think there's any other > #editBMPanel_folderTree that browser.css could style. pinstripe also has some > similar selectors. No, that's needed, i need to force the min width only in the Star UI, not in properties dialogs nor in the Library. If it would not be like that i'd put the rule in editBookmarkOverlay.css > >+++ b/browser/themes/gnomestripe/browser/places/editBookmarkOverlay.css > > >+#editBMPanel_folderTree { > >+ margin: 2px 4px; > > } > > The horizontal margin is already there, I think, so margin-top/bottom: 2px > should be sufficient. Same for winstripe. will check, iriginally i had to put it > >+++ b/browser/themes/pinstripe/browser/places/editBookmarkOverlay.css > > >+#editBMPanel_folderTree { > >+ margin: 6px 4px 0px 4px; > > } > > 0 instead of 0px, please. ok > >+++ b/browser/themes/winstripe/browser/places/editBookmarkOverlay.css > >@@ -52,34 +52,38 @@ > > > > > > /**** expanders ****/ > > > > .expander-up, > > .expander-down { > > min-width: 0; > > margin: 0; > >+ margin-right: 4px; > > -moz-margin-end? i don't know if the star dialog is reversed in RTL or only the input contents are, so i'll try with an RTL language pack if that's the case.

(In reply to comment #58) > > -moz-margin-end? > > i don't know if the star dialog is reversed in RTL or only the input contents > are, so i'll try with an RTL language pack if that's the case. ok i checked with forceRTL extension, and -moz-margin-end is correct, will fix

(In reply to comment #58) > (In reply to comment #57) > > (From update of attachment 356166 [details] [diff] [review] [details]) > > >+++ b/browser/components/places/content/editBookmarkOverlay.xul > > > > >+ <hbox align="center" id="editBMPanel_selectionCount" hidden="true"> > > >+ <vbox id="editBMPanel_itemsCountBox" align="center"> > > >+ <label id="editBMPanel_itemsCountText"/> > > >+ </vbox> > > >+ </hbox> > > > > What are these boxes doing? > > we had those ids before and i wanted to preserve them since we should try to > not do changes that could hit themes at this point. So since we were wrongly > using a row before, i changed it to an hbox Well, you're moving it to a different position in the document, right? So even if a theme is using these ids (very unlikely, imho, but could be checked by searching on AMO), it's quite possible that things won't look right. I'd certainly get rid of editBMPanel_itemsCountBox, and editBMPanel_selectionCount should just have pack="center". > > >+ <vbox> > > >+ <button id="editBMPanel_foldersExpander" > > >+ class="expander-down" > > >+ tooltiptext="&editBookmarkOverlay.foldersExpanderDown.tooltip;" > > >+ tooltiptextdown="&editBookmarkOverlay.foldersExpanderDown.tooltip;" > > >+ tooltiptextup="&editBookmarkOverlay.expanderUp.tooltip;" > > >+ oncommand="gEditItemOverlay.toggleFolderTreeVisibility();" > > >+ observes="paneElementsBroadcaster"/> > > >+ </vbox> > > > > What's the point of that vbox? Same question for > > editBMPanel_tagsSelectorExpander. > > practically, the expanders take the full height of the grid's row, that would > not be an issue generally, but on win Vista (for example) the menulist is > higher than normal text fields. So as soon as an element with an expander is > higher the expander becomes stretched. This stretch the vbox instead. align="center" on the parent seems like the right way to address this. > > >+++ b/browser/themes/gnomestripe/browser/browser.css > > > > >+#editBookmarkPanel #editBMPanel_folderTree { > > >+ min-width: 300px; > > >+} > > > > Remove #editBookmarkPanel from that selector? I don't think there's any other > > #editBMPanel_folderTree that browser.css could style. pinstripe also has some > > similar selectors. > > No, that's needed, i need to force the min width only in the Star UI, not in > properties dialogs nor in the Library. If it would not be like that i'd put the > rule in editBookmarkOverlay.css browser.css is used in the properties dialogs and in the library?

(In reply to comment #60) > > No, that's needed, i need to force the min width only in the Star UI, not in > > properties dialogs nor in the Library. If it would not be like that i'd put the > > rule in editBookmarkOverlay.css > > browser.css is used in the properties dialogs and in the library? probably not, but i really prefer retaining the external id, so it's really clear that such change should apply only to that particular panel and in future nobody will try to move that code into editBookmarkOverlay.css.

(In reply to comment #61) > probably not, but i really prefer retaining the external id, so it's really > clear that such change should apply only to that particular panel and in future > nobody will try to move that code into editBookmarkOverlay.css. In that case, adding a comment seems more appropriate.

Dao's comments addressed. i'm generating new trybuilds to test this new version on all platforms. After Dao's review i'll ask an additional review later to Dietrich for Places code changes.

Comment on attachment 356499 [details] [diff] [review] patch v1.3 >+ <hbox id="editBMPanel_selectionCount" hidden="true" >+ align="center" pack="center"> >+ <label id="editBMPanel_itemsCountText"/> >+ </hbox> align="center" looks like a no-op. <label is wrongly indented. >+ </menulist> >+ <button id="editBMPanel_foldersExpander" also wrongly indented Looks good... waiting for the tryserver builds.

tryserver builds will appear here: https://build.mozilla.org/tryserver-builds/2009-01-12_05:11-mak77@bonardo.net-try-761d6ea6b41/

fixed indentation and removed align="center". trybuilds were tested ok on Ubuntu, XP, Vista. i'm only missing check on Mac.

Comment on attachment 356520 [details] [diff] [review] patch v1.4 asking additional review for Places specific changes.

Comment on attachment 356520 [details] [diff] [review] patch v1.4 >diff --git a/browser/themes/gnomestripe/browser/browser.css b/browser/themes/gnomestripe/browser/browser.css >--- a/browser/themes/gnomestripe/browser/browser.css >+++ b/browser/themes/gnomestripe/browser/browser.css >@@ -1083,16 +1083,21 @@ toolbar[iconsize="small"] #paste-button[ > #editBookmarkPanelStarIcon[unstarred] { > list-style-image: url("chrome://browser/skin/places/unstarred48.png"); > } > > #editBookmarkPanelTitle { > font-size: 130%; > } > >+/* Implements editBookmarkPanel resizing on folderTree uncollpase. */ >+#editBMPanel_folderTree { >+ min-width: 300px; >+} >+ spelling nit: "uncollapse", but since that's not actually a word, please regrammarize this comment. r=me

changed "uncollpase" -> un-collapse

Comment on attachment 356623 [details] [diff] [review] patch v1.5 asking approval for this polish bug.

Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9.2a1pre) Gecko/20090114 Minefield/3.2a1pre I feel that "Tags:" is too narrow for a large font. Perhaps, this influences L10N.

(In reply to comment #72) > I feel that "Tags:" is too narrow for a large font. > Perhaps, this influences L10N. Was it larger before?

{Build ID: 20090113035246} (In reply to comment #73) > Was it larger before? Yes.

please file a new bug as a regression blocking this, with those 2 screenshots.

(In reply to comment #75) > please file a new bug as a regression blocking this, with those 2 screenshots. filed Bug 473536

Did this ever get approval to be landed on the 1.9.1 tree? Is it likely to make 3.1 before final release?

Comment on attachment 356623 [details] [diff] [review] patch v1.5 a191=beltzner

Verified fixed with builds on OS X and Windows: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.2a1pre) Gecko/20090420 Minefield/3.6a1pre ID:20090420031158 Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b4pre) Gecko/20090421 Shiretoko/3.5b4pre ID:20090421030848

This bug's priority relative to the set of other polish bugs is: P1 - Polish issue that appears in the main window, or is something that the user may encounter several times a day. Bookmarking is a common enough action for this to be p1, previously listed as high visibility.

Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h". In Thunderbird 3.0b, you do that as follows: Tools | Message Filters Make sure the correct account is selected. Click "New" Conditions: Body contains places-to-b-and-h Change the action to "Delete Message". Select "Manually Run" from the dropdown at the top. Click OK. Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter. Gerv