Last Comment Bug 585601 - Address Neil's post-landing comments on places bookmarks
: Address Neil's post-landing comments on places bookmarks
Status: RESOLVED FIXED
:
Product: SeaMonkey
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: unspecified
: All All
: -- normal (vote)
: seamonkey2.1b1
Assigned To: Robert Kaiser
:
Mentors:
Depends on: 591779 SMPlacesBMarks 580660 580662 630654
Blocks: 589594 643271
  Show dependency treegraph
 
Reported: 2010-08-09 07:20 PDT by Robert Kaiser
Modified: 2011-03-20 06:54 PDT (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
round 1, v1: address bug 580660 comments (6.47 KB, patch)
2010-08-12 15:59 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
round 2, v1: address bug 580662 comments (23.83 KB, patch)
2010-08-12 18:01 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
round 1, v1.1: also fix the chevron in customize (cehcked in) (7.01 KB, patch)
2010-08-16 10:22 PDT, Robert Kaiser
neil: review+
Details | Diff | Splinter Review
round 2, v1.1: address recent comments (25.51 KB, patch)
2010-08-17 07:16 PDT, Robert Kaiser
no flags Details | Diff | Splinter Review
round 3, v1: mak's comment on the glue variables (checked in) (6.47 KB, patch)
2010-08-17 07:32 PDT, Robert Kaiser
neil: review+
Details | Diff | Splinter Review
round 2, v1.2: more comment addressing (checked in) (27.65 KB, patch)
2010-08-22 08:23 PDT, Robert Kaiser
neil: review+
Details | Diff | Splinter Review

Description Robert Kaiser 2010-08-09 07:20:23 PDT
See bug 580660 comment #10 and following, bug 580662 comment #20 and following.
Comment 1 Robert Kaiser 2010-08-12 15:59:03 PDT
Created attachment 465432 [details] [diff] [review]
round 1, v1: address bug 580660 comments

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. ;-)
Comment 2 Robert Kaiser 2010-08-12 18:01:22 PDT
Created attachment 465496 [details] [diff] [review]
round 2, v1: address bug 580662 comments

And here's the patch addressing the bug 580662 comments.
Comment 3 neil@parkwaycc.co.uk 2010-08-13 02:40:50 PDT
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.
Comment 4 neil@parkwaycc.co.uk 2010-08-13 04:31:54 PDT
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.
Comment 5 Robert Kaiser 2010-08-13 04:46:22 PDT
(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. ;-)
Comment 6 Robert Kaiser 2010-08-16 10:22:17 PDT
Created attachment 466349 [details] [diff] [review]
round 1, v1.1: also fix the chevron in customize (cehcked in)

Ah, I found the chevron when I had a small enough window that it was showed in the first place :)
Comment 7 Robert Kaiser 2010-08-17 06:30:38 PDT
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
Comment 8 Robert Kaiser 2010-08-17 07:16:07 PDT
Created attachment 466632 [details] [diff] [review]
round 2, v1.1: address recent comments

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.
Comment 9 Robert Kaiser 2010-08-17 07:32:48 PDT
Created attachment 466641 [details] [diff] [review]
round 3, v1: mak's comment on the glue variables (checked in)

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).
Comment 10 Robert Kaiser 2010-08-17 08:48:37 PDT
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
Comment 11 neil@parkwaycc.co.uk 2010-08-17 14:42:46 PDT
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?]
Comment 12 Stefan [:stefanh] 2010-08-17 16:30:41 PDT
(In reply to comment #11)
> [I guess stefanh gets to port the Mac changes?]

Yes, I will do that in bug 586026.
Comment 13 Robert Kaiser 2010-08-18 05:33:26 PDT
(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?
Comment 14 neil@parkwaycc.co.uk 2010-08-18 07:27:26 PDT
(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.
Comment 15 neil@parkwaycc.co.uk 2010-08-20 16:08:45 PDT
I filed bug 589328 on the access key only working the first time issue.
Comment 16 Robert Kaiser 2010-08-22 08:23:05 PDT
Created attachment 468140 [details] [diff] [review]
round 2, v1.2: more comment addressing (checked in)

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.
Comment 17 neil@parkwaycc.co.uk 2010-08-22 09:48:04 PDT
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.
Comment 18 neil@parkwaycc.co.uk 2010-08-22 10:00:29 PDT
(In reply to comment #17)
> There appears to be a bug with collapsing decks, stacks, grids and lists.
Filed bug 589569.
Comment 19 Robert Kaiser 2010-08-22 12:41:19 PDT
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.
Comment 20 Robert Kaiser 2010-08-22 12:49:00 PDT
(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.

Note You need to log in before you can comment on or make changes to this bug.