Robert Kaiser Assignee
	Description • 14 years ago

See bug 580660 comment #10 and following, bug 580662 comment #20 and following.

	Robert Kaiser Assignee
	Comment 1 • 14 years ago

OK, I hope this addresses the bug 580660 comments the way you wanted. :)
I also found out that FF uses the bookmarks toolbar icon as the icons for the bookmarks toolbar while customizing, which somehow sounded like a good idea, so I adopted that. ;-)

	Robert Kaiser Assignee
	Comment 2 • 14 years ago

And here's the patch addressing the bug 580662 comments.

	neil@parkwaycc.co.uk
	Comment 3 • 14 years ago

Comment on attachment 465432 [details] [diff] [review]
round 1, v1: address bug 580660 comments

This almost works, except for the overflow chevron which is still visible on the toolbar during customisation.

	neil@parkwaycc.co.uk
	Comment 4 • 14 years ago

Comment on attachment 465496 [details] [diff] [review]
round 2, v1: address bug 580662 comments

>         var menuitemPrefix = propertyPrefix;
>         var columnId = column.getAttribute("anonid");
>-        menuitemPrefix += columnId == "title" ? "name" : columnId;
>+        menuitemPrefix += columnId;
Nit: can simplify this to
var menuitemPrefix = propertyPrefix + column.getAttribute("anonid");

>+      <toolbarspring flex="1"/>
Nit: toolbarspring already has flex

>-          class="placesTree"
>+          class="plain placesTree"
[I take it you kept placesTree just in case ;-) ]

>           persist="width"
[Strange, I thought you would need to persist collapsed too, but apparently the splitter state seems to takes care of it.]

>+      <vbox id="searchModifiers" hidden="true">
>+        <toolbar id="organizerScopeBar" class="chromeclass-toolbar" align="center">
[Still not convinced that the vbox is necessary, the code could be switched to show and hide the toolbar instead. EDIT: but see below]

>+          <button id="scopeBarFolder" class="small-margin"
>+                  type="radio" group="scopeBar"
>+                  oncommand="PlacesQueryBuilder.onScopeSelected(this);"
>+                  accesskey="&search.scopeFolder.accesskey;"
>+                  emptytitle="&search.scopeFolder.label;" flex="1"/>
>+          <!-- The folder scope button should flex but not take up more room
>+                than its label needs.  The only simple way to do that is to
>+                set a really big flex on the spacer, e.g., 2^31 - 1. -->
>+          <spacer flex="2147483647"/>
[So, I thought I'd try setting a really long name on the folder to see what happens. And it's not nice ;-) I'm not quite sure why setting crop="center" (or right; not sure which is better) doesn't work, but I did get it to work by also setting orient="vertical". The dialog buttons in the XUL test suite have the same bug. The other workaround would be to add flex to the XBL somewhere.]

>+      <vbox flex="1">
[Already mentioned in my reply to mak.]

> #organizerScopeBar {
>   -moz-appearance: toolbox;
[I hadn't looked at the CSS before, and this line confused me. Maybe the searchModifiers should be a toolbox. Haven't actually tried this though.]

>-toolbaritem > menubar {
>+toolbaritem > menubar,
>+toolbar > menubar {
This doesn't work because we don't use communicator.css in this window.

	Robert Kaiser Assignee
	Comment 5 • 14 years ago

(In reply to comment #3)
> Comment on attachment 465432 [details] [diff] [review]
> round 1, v1: address bug 580660 comments
> 
> This almost works, except for the overflow chevron which is still visible on
> the toolbar during customisation.

I did test this patch and never saw an overflow chevron, but I'll recheck.

(In reply to comment #4)
> >-          class="placesTree"
> >+          class="plain placesTree"
> [I take it you kept placesTree just in case ;-) ]

Yes, themers might want to do something with it, including those fancy people doing the 3.5 Firefox themes.

> >           persist="width"
> [Strange, I thought you would need to persist collapsed too, but apparently the
> splitter state seems to takes care of it.]

That's what I found out as well in testing. :)

> >+          <spacer flex="2147483647"/>
> [So, I thought I'd try setting a really long name on the folder to see what
> happens. And it's not nice ;-) I'm not quite sure why setting crop="center" (or
> right; not sure which is better) doesn't work, but I did get it to work by also
> setting orient="vertical". The dialog buttons in the XUL test suite have the
> same bug. The other workaround would be to add flex to the XBL somewhere.]

So, anything I should do as a result to this comment.

> > #organizerScopeBar {
> >   -moz-appearance: toolbox;
> [I hadn't looked at the CSS before, and this line confused me. Maybe the
> searchModifiers should be a toolbox. Haven't actually tried this though.]

hmm, converting to a toolbox might be logical, I'll see how that works.

> >-toolbaritem > menubar {
> >+toolbaritem > menubar,
> >+toolbar > menubar {
> This doesn't work because we don't use communicator.css in this window.

Then the latter is the bug I should fix, IMHO. ;-)

	Robert Kaiser Assignee
	Comment 6 • 14 years ago

Ah, I found the chevron when I had a small enough window that it was showed in the first place :)

	Robert Kaiser Assignee
	Comment 7 • 14 years ago

Comment on attachment 466349 [details] [diff] [review]
round 1, v1.1: also fix the chevron in customize (cehcked in)

Pushed round 1 as http://hg.mozilla.org/comm-central/rev/733a7d5f5853

	Robert Kaiser Assignee
	Comment 8 • 14 years ago

This patch should address the additional comments made in this bug. I also took the freedom to insert a splitter before the details deck so people can hide it if they want. I had that in mind from the beginning and think it fits pretty well with this patch.

	Robert Kaiser Assignee
	Comment 9 • 14 years ago

Here's one more I'd like to get in, but I didn't want to file another new bug for it. Marco commented in bug 570788 comment 17 that he'll not take our variable changes for glue at this time, so for easing future merging of patches on their side, we probably should "revert" our names to what they have as well. I'll leave this up to your review, but even if I understand your reasoning, I'd prefer to take this for ease of future porting (such minor differences are very likely to go unnoticed and cause bug in such work).

	Robert Kaiser Assignee
	Comment 10 • 14 years ago

Comment on attachment 466641 [details] [diff] [review]
round 3, v1: mak's comment on the glue variables (checked in)

Pushed the glue fixup as http://hg.mozilla.org/comm-central/rev/061a69146c3e

	neil@parkwaycc.co.uk
	Comment 11 • 14 years ago

Comment on attachment 466632 [details] [diff] [review]
round 2, v1.1: address recent comments

>+<?xml-stylesheet href="chrome://communicator/skin/"?>
So, the menu toolbar and the search scope bar are now collapsable... was that intentional?

>+          <label id="scopeBarTitle" value="&search.label;"/>
>+          <button id="scopeBarAll" class="small-margin"
>+                  type="radio" group="scopeBar"
>+                  oncommand="PlacesQueryBuilder.onScopeSelected(this);"
>+                  label="&search.scopeBookmarks.label;"
>+                  accesskey="&search.scopeBookmarks.accesskey;"/>
>+          <button id="scopeBarFolder" class="small-margin"
>+                  type="radio" group="scopeBar"
>+                  oncommand="PlacesQueryBuilder.onScopeSelected(this);"
>+                  accesskey="&search.scopeFolder.accesskey;"
>+                  emptytitle="&search.scopeFolder.label;" flex="1"/>
[Still don't like that accesskey, given that we don't even know what the label is, but the only other possibility is to make this a radiogroup.]

>+      <splitter id="detailsDeck-splitter"
>+                collapse="after"
>+                persist="state collapsed">
>+        <grippy/>
>+      </splitter>
A nice idea, but it didn't seem to resize or collapse sensibly. (Given the More/Less button, I'm not even sure it makes sense to resize it.) If you do get it to work, you can remove the top border on the details deck.

>+            <button type="image" id="infoBoxExpander"
>+                    class="expander-down"
>+                    oncommand="PlacesOrganizer.toggleAdditionalInfoFields();"
>+                    observes="paneElementsBroadcaster"/>
> 
>+            <label id="infoBoxExpanderLabel"
>+                    lesslabel="&detailsPane.less.label;"
>+                    lessaccesskey="&detailsPane.less.accesskey;"
>+                    morelabel="&detailsPane.more.label;"
>+                    moreaccesskey="&detailsPane.more.accesskey;"
>+                    value="&detailsPane.more.label;"
>+                    accesskey="&detailsPane.more.accesskey;"
>+                    control="infoBoxExpander"/>
Unfortunately two bugs affect us here.
The first is that an access key on a label doesn't click the button. This is a recent regression that should get fixed soon.
The second is that changing the access key on a label doesn't work correctly. The workaround seems to be to set the access key on the button instead. (I'm not sure but you might need to set the access key before the label value.)
Also the access key still conflicts with the Edit menu. (Rather than Less, would Fewer make, um, more sense? Editor dialogs uses Fewer, I think.)
Let me know whether you would prefer to fix these issues separately.

>+toolbar > toolbaritem > menubar,
>+toolbar > menubar {
I was toying with the idea of putting the menubar in its own toolbaritem (customisable bookmarks toolbar, anyone?) but I decided it wasn't worth it.

>diff --git a/suite/themes/classic/communicator/bookmarks/bookmarksManager.css b/suite/themes/classic/communicator/bookmarks/bookmarksManager.css
[I guess stefanh gets to port the Mac changes?]

	Stefan [:stefanh]
	Comment 12 • 14 years ago

(In reply to comment #11)
> [I guess stefanh gets to port the Mac changes?]

Yes, I will do that in bug 586026.

	Robert Kaiser Assignee
	Comment 13 • 14 years ago

(In reply to comment #11)
> >+<?xml-stylesheet href="chrome://communicator/skin/"?>
> So, the menu toolbar and the search scope bar are now collapsable... was that
> intentional?

For the menu toolbar, yes, looks fine - the search scope bar probably needs xpfe="false", thanks for noticing.

> >+      <splitter id="detailsDeck-splitter"
> >+                collapse="after"
> >+                persist="state collapsed">
> >+        <grippy/>
> >+      </splitter>
> A nice idea, but it didn't seem to resize or collapse sensibly. (Given the
> More/Less button, I'm not even sure it makes sense to resize it.) If you do get
> it to work, you can remove the top border on the details deck.

Collapse seems to work right for me, but you're right that resize doesn't make much sense. I guess there's no way to make a splitter that can only collapse but not resize?
And yes, I noted the border, will remove when the idea persists.

> The first is that an access key on a label doesn't click the button. This is a
> recent regression that should get fixed soon.
> The second is that changing the access key on a label doesn't work correctly.

Is this second one filed as a bug as well?

> The workaround seems to be to set the access key on the button instead. (I'm
> not sure but you might need to set the access key before the label value.)

Won't that make the accesskey be displayed on the button, which looks really ugly here?

> Also the access key still conflicts with the Edit menu. (Rather than Less,
> would Fewer make, um, more sense? Editor dialogs uses Fewer, I think.)
> Let me know whether you would prefer to fix these issues separately.

I think looking into the conflicting accesskeys is something that might make sense in a followup, yes.

> >diff --git a/suite/themes/classic/communicator/bookmarks/bookmarksManager.css b/suite/themes/classic/communicator/bookmarks/bookmarksManager.css
> [I guess stefanh gets to port the Mac changes?]

Well, right now, there's no Mac-specific file for this, is there?

	neil@parkwaycc.co.uk
	Comment 14 • 14 years ago

(In reply to comment #13)
> (In reply to comment #11)
> Collapse seems to work right for me, but you're right that resize doesn't make
> much sense. I guess there's no way to make a splitter that can only collapse
> but not resize?
Try <splitter disabled="true"> but you might have to override the cursor.

> > The second is that changing the access key on a label doesn't work correctly.
> Is this second one filed as a bug as well?
I'm not sure. I'll have to ask the guy who spotted it.

> > The workaround seems to be to set the access key on the button instead. (I'm
> > not sure but you might need to set the access key before the label value.)
> Won't that make the accesskey be displayed on the button, which looks really
> ugly here?
It didn't in my basic test in DOM Inspector - if you have a label, the access key still appears on the label, even if you set it on the control.

> > [I guess stefanh gets to port the Mac changes?]
> Well, right now, there's no Mac-specific file for this, is there?
Oh, I hadn't noticed, sorry.

	neil@parkwaycc.co.uk
	Comment 15 • 14 years ago

I filed bug 589328 on the access key only working the first time issue.

	Robert Kaiser Assignee
	Comment 16 • 14 years ago

This should address the recent comments, make the grippy on the search scope toolbar going away and makes the splitter at the bottom be disabled and its cursor normal, but the grippy still working.

	neil@parkwaycc.co.uk
	Comment 17 • 14 years ago

Comment on attachment 468140 [details] [diff] [review]
round 2, v1.2: more comment addressing (checked in)

> #contentTitle {
>   width: 0px;
> }
[I can't find this element... looks like it got removed from Places too.]

>+   content/communicator/bookmarks/organizer.css                     (bookmarks/organizer.css)
[Why not bookmarksManager.css?]

>+/* Info box */
>+#detailsDeck {
>   padding: 5px;
There appears to be a bug with collapsing decks, stacks, grids and lists.
To work around it, please change these to a 5px margin instead.

	neil@parkwaycc.co.uk
	Comment 18 • 14 years ago

(In reply to comment #17)
> There appears to be a bug with collapsing decks, stacks, grids and lists.
Filed bug 589569.

	Robert Kaiser Assignee
	Comment 19 • 14 years ago

Comment on attachment 468140 [details] [diff] [review]
round 2, v1.2: more comment addressing (checked in)

Pushed as http://hg.mozilla.org/comm-central/rev/64ad57c8da27 with the last nits addressed.

	Robert Kaiser Assignee
	Comment 20 • 14 years ago

(In reply to comment #13)
> > Also the access key still conflicts with the Edit menu. (Rather than Less,
> > would Fewer make, um, more sense? Editor dialogs uses Fewer, I think.)
> > Let me know whether you would prefer to fix these issues separately.
> 
> I think looking into the conflicting accesskeys is something that might make
> sense in a followup, yes.

Filed bug 589594 for that.