413053 - Bookmark Dialogs: Align treeview and listview

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P3)

Description

[Alex Faaborg [:faaborg] (Firefox UX)]

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

Comment 1

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Something which could be fixed for Firefox 3.1?

Comment 2

[Dietrich Ayala (:dietrich)]

Yes, but it will not block the release.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 4

[Alex Faaborg [:faaborg] (Firefox UX)]

>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.

Comment 5

[Alex Faaborg [:faaborg] (Firefox UX)]

this bug is eligible for bug 462081

Comment 6

[Marco Bonardo [:mak]]

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

Comment 7

[Marco Bonardo [:mak]]

Attachment: patch

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

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 9

[Marco Bonardo [:mak]]

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

Comment 10

[Marco Bonardo [:mak]]

https://build.mozilla.org/tryserver-builds/2008-10-29_12:[e-mail]-try-89d83d687b4/

Comment 11

[Marco Bonardo [:mak]]

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?

Comment 12

[Marcia Knous [:marcia]]

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.

Comment 13

[Luke Iliffe (Harlequin99)]

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;
}

Comment 14

[Marco Bonardo [:mak]]

(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

Comment 15

[Marco Bonardo [:mak]]

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 :\

Comment 16

[Luke Iliffe (Harlequin99)]

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.

Comment 17

[Alex Faaborg [:faaborg] (Firefox UX)]

Attachment: Possible alignment on OS X

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.

Comment 18

[Markus Stange [:mstange]]

(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?

Comment 19

[Marco Bonardo [:mak]]

(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.

Comment 20

[Alex Faaborg [:faaborg] (Firefox UX)]

>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

Comment 21

[Marco Bonardo [:mak]]

Attachment: screen

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?

Comment 22

[Marco Bonardo [:mak]]

Attachment: wip

Comment 23

[Marco Bonardo [:mak]]

Attachment: 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.

Comment 24

[Marcia Knous [:marcia]]

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.

Comment 25

[Alex Faaborg [:faaborg] (Firefox UX)]

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 26

[Alex Faaborg [:faaborg] (Firefox UX)]

Comment on attachment 348978 [details]
screen

See comment above.

Comment 28

[Shawn Wilsher :sdwilsh]

Marco, do you plan on updating this?

Comment 29

[Marco Bonardo [:mak]]

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.

Comment 30

[Marco Bonardo [:mak]]

Attachment: wip 0.3

Comment 31

[Marco Bonardo [:mak]]

Attachment: wip 0.4

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.

Comment 32

[Marco Bonardo [:mak]]

Attachment: screenshot 0.4

Comment 33

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 34

[Alex Faaborg [:faaborg] (Firefox UX)]

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.

Comment 35

[Marco Bonardo [:mak]]

Attachment: patch v1

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.

Comment 36

[Marco Bonardo [:mak]]

trybuilds will appear here:
https://build.mozilla.org/tryserver-builds/2009-01-07_01:[e-mail]-try-8bb533ac94a/

Comment 37

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 38

[Marco Bonardo [:mak]]

(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 :\

Comment 39

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(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?

Comment 40

[Marco Bonardo [:mak]]

(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.

Comment 41

[Marco Bonardo [:mak]]

Attachment: patch v1.1

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.

Comment 42

[Marco Bonardo [:mak]]

v1.1 trybuilds:
https://build.mozilla.org/tryserver-builds/2009-01-07_13:[e-mail]-try-ee978384989/

Comment 43

[Marco Bonardo [:mak]]

Comment on attachment 355839 [details] [diff] [review]
patch v1.1

tested on all 4 platforms, asking first review to Dao

Comment 44

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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.

Comment 45

[Marco Bonardo [:mak]]

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

Comment 46

[Markus Stange [:mstange]]

This looks great, thanks Marco!

Comment 47

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(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?

Comment 48

[Marco Bonardo [:mak]]

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

Comment 49

[Dão Gottwald [:dao]]

Comment on attachment 355839 [details] [diff] [review]
patch v1.1

>+#editBookmarkPanelGrid > row[collapsed="true"] {
>   display: none;
> }

Uh, what's that?

Comment 50

[Marco Bonardo [:mak]]

(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.

Comment 51

[Dão Gottwald [:dao]]

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

Comment 52

[Marco Bonardo [:mak]]

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.

Comment 53

[Dão Gottwald [:dao]]

(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

Comment 54

[Marco Bonardo [:mak]]

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

Comment 55

[Marco Bonardo [:mak]]

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.

Comment 56

[Marco Bonardo [:mak]]

Attachment: patch v1.2

removed that stupid useless rule, thanks Dao!

Comment 57

[Dão Gottwald [: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?

Comment 58

[Marco Bonardo [:mak]]

(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.

Comment 59

[Marco Bonardo [:mak]]

(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

Comment 60

[Dão Gottwald [:dao]]

(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?

Comment 61

[Marco Bonardo [:mak]]

(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.

Comment 62

[Dão Gottwald [:dao]]

(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.

Comment 63

[Marco Bonardo [:mak]]

Attachment: patch v1.3

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 64

[Dão Gottwald [:dao]]

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.

Comment 65

[Marco Bonardo [:mak]]

tryserver builds will appear here:
https://build.mozilla.org/tryserver-builds/2009-01-12_05:[e-mail]-try-761d6ea6b41/

Comment 66

[Marco Bonardo [:mak]]

Attachment: patch v1.4

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

Comment 67

[Marco Bonardo [:mak]]

Comment on attachment 356520 [details] [diff] [review]
patch v1.4

asking additional review for Places specific changes.

Comment 68

[Dietrich Ayala (:dietrich)]

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

Comment 69

[Marco Bonardo [:mak]]

Attachment: patch v1.5

changed "uncollpase" -> un-collapse

Comment 70

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/acee30ba305a

Comment 71

[Marco Bonardo [:mak]]

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

asking approval for this polish bug.

Comment 72

[AKIHIRO Misaki (a.k.a Kuden)]

Attachment: screenshot

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.

Comment 73

[Marco Bonardo [:mak]]

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

Was it larger before?

Comment 74

[AKIHIRO Misaki (a.k.a Kuden)]

Attachment: screenshot 20090113

{Build ID: 20090113035246}

(In reply to comment #73)
> Was it larger before?

Yes.

Comment 75

[Marco Bonardo [:mak]]

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

Comment 76

[AKIHIRO Misaki (a.k.a Kuden)]

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

filed Bug 473536

Comment 78

[Luke Iliffe (Harlequin99)]

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 79

[Mike Beltzner [:beltzner, not reading bugmail]]

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

a191=beltzner

Comment 80

[Marco Bonardo [:mak]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/13f878f17fa1

Comment 81

[Henrik Skupin [:whimboo][⌚️UTC+1]]

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

Comment 82

[Alex Faaborg [:faaborg] (Firefox UX)]

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.

Comment 83

[Gervase Markham [:gerv]]

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