When we switch to places bookmarks, we need management UI for those. This is a split-off of part 5 of bug 498596, for easier review (as now that part is isolated here).

This patch is rather large, it might make sense to break it apart to make the manager and the edit panel separate for reviews, possibly even a third one, but I'll look into that later in the process.

Comment on attachment 459044 [details] [diff] [review] v2: implement places bookmarks managerment Requesting additional review from Ian on all patches but the actual core - I'll take reviews from whoever gets around to it first.

Here's a diff of the original files from Firefox vs. our ported files in the patch, for easier review.

This patch has been bitrotted by various patches in browser-prefs.js, navigator.xul and suite/common/jar.mn In navigator.xul, like on bug 580660, it is possibly due to a patch in your tree, that is not checked in which includes the line: <panel id="notification-popup" position="after_start" noautofocus="true" hidden="true"/>

Comment on attachment 459044 [details] [diff] [review] v2: implement places bookmarks managerment >diff --git a/suite/browser/navigator.xul b/suite/browser/navigator.xul >+ <panel id="editBookmarkPanel" Could this be put into a suitable overlay, placesOverlay.xul perhaps, along with the related entities in places.dtd or does this cause problems?

Comment on attachment 459044 [details] [diff] [review] v2: implement places bookmarks managerment Having had discussions on irc, happy with this as it is.

(In reply to comment #5) > >+ <panel id="editBookmarkPanel" > Could this be put into a suitable overlay, placesOverlay.xul perhaps, along > with the related entities in places.dtd or does this cause problems? That's something should be able to do quite easily, but I fear the entries in the .properties need to stay for now, as we don't have a good other bundle available as an object right now (I think).

Here's an patch that's updated for Ian's comments and unbitrotted. I'm asking ui-r from Neil for now, not sure if and how deeply he wants to look into this before checkin, which I hope to be able to do before the weekend, core review permitting, so that we have a few nightlies before we cut a3 early next week with this in.

Comment on attachment 461283 [details] [diff] [review] diff of Firefox places files vs. SeaMonkey places files >+<?xml-stylesheet href="chrome://communicator/content/places/places.css"?> > <?xml-stylesheet href="chrome://global/skin/"?> >-<?xml-stylesheet href="chrome://browser/skin/places/editBookmarkOverlay.css"?> >-<?xml-stylesheet href="chrome://browser/skin/places/places.css"?> >-<?xml-stylesheet href="chrome://browser/content/places/places.css"?> >+<?xml-stylesheet href="chrome://communicator/skin/bookmarks/editBookmarkOverlay.css"?> >+<?xml-stylesheet href="chrome://communicator/skin/bookmarks/bookmarks.css"?> IMHO global (or communicator) skin should come first, then content places.css, then skin bookmarks.css (so Firefox was wrong too.) >+ var file = fp.file.QueryInterface(Components.interfaces.nsILocalFile); > importer.importHTMLFromFile(file, false); fp.file is already an nsILocalFile so you can write importer.importHTMLFromFile(fp.file, false); (Actually this works anyway, because XPConnect will QI parameters for you.) > #ifdef ADVANCED_STARRING_UI What's this? > // set the pref >- var prefs = Cc["@mozilla.org/preferences-service;1"]. >- getService(Ci.nsIPrefBranch); >+ var prefs = Components.classes["@mozilla.org/preferences-service;1"]. >+ getService(Components.interfaces.nsIPrefBranch); > prefs.setCharPref("browser.bookmarks.editDialog.firstEditField", aNewField); I take it there's no Services handy?

Comment on attachment 462468 [details] [diff] [review] v2.1: bookmarks management, updated for comments >diff --git a/suite/browser/navigator.js b/suite/browser/navigator.js ... >+__defineGetter__("PluralForm", function() { >+ Components.utils.import("resource://gre/modules/PluralForm.jsm"); >+ return this.PluralForm; >+}); >+__defineSetter__("PluralForm", function (val) { >+ delete this.PluralForm; >+ return this.PluralForm = val; >+}); Odd place to put this, given that you don't add any consumers. >- <popup id="placesContext"/> >+ <menupopup id="placesContext"/> This is confusing... >+ var uri = PlacesUIUtils.createFixedURI(value); >+ return true; Nit: result not used, no need to assign it to a variable. >+ // HACK: as we're importing the actual PlacesUIUtils but that name is taken >+ // by a cut-down history-specific version, store that latter one temporarily >+ var HistoryUtils = PlacesUIUtils; >+ Components.utils.import("resource:///modules/PlacesUIUtils.jsm"); Why not import into a temporary scope? >+# Provide another URI for the bookmark properties dialog so we can persist the attributes separately >+ content/communicator/bookmarks/bm-props2.xul (bookmarks/bm-props.xul) Why? (But you should use a chrome override rather than a copy.)

(In reply to comment #10) > Comment on attachment 462468 [details] [diff] [review] > v2: bookmarks management, updated for comments > > >diff --git a/suite/browser/navigator.js b/suite/browser/navigator.js > ... > >+__defineGetter__("PluralForm", function() { > >+ Components.utils.import("resource://gre/modules/PluralForm.jsm"); > >+ return this.PluralForm; > >+}); > >+__defineSetter__("PluralForm", function (val) { > >+ delete this.PluralForm; > >+ return this.PluralForm = val; > >+}); > Odd place to put this, given that you don't add any consumers. I knew there was something I wanted to mention on this patch, ah well, glad Neil spotted it as well.

(In reply to comment #9) > Comment on attachment 461283 [details] [diff] [review] > diff of Firefox places files vs. SeaMonkey places files > > >+<?xml-stylesheet href="chrome://communicator/content/places/places.css"?> > > <?xml-stylesheet href="chrome://global/skin/"?> > >-<?xml-stylesheet href="chrome://browser/skin/places/editBookmarkOverlay.css"?> > >-<?xml-stylesheet href="chrome://browser/skin/places/places.css"?> > >-<?xml-stylesheet href="chrome://browser/content/places/places.css"?> > >+<?xml-stylesheet href="chrome://communicator/skin/bookmarks/editBookmarkOverlay.css"?> > >+<?xml-stylesheet href="chrome://communicator/skin/bookmarks/bookmarks.css"?> > IMHO global (or communicator) skin should come first, then content places.css, > then skin bookmarks.css (so Firefox was wrong too.) Hmm, I'm generally more a fan of first importing all content and then skin, so that any content stuff can never override any skin stuff just because of the "later in processing" rule. I agree they have it wrong as well, though. In any case, to get it landed yesterday, I'll follow whatever you say just to get it in. > >+ var file = fp.file.QueryInterface(Components.interfaces.nsILocalFile); > > importer.importHTMLFromFile(file, false); > fp.file is already an nsILocalFile so you can write > importer.importHTMLFromFile(fp.file, false); > (Actually this works anyway, because XPConnect will QI parameters for you.) Hmm, I guess I should backport that one. > > #ifdef ADVANCED_STARRING_UI > What's this? Something that really needs to go away, as I learned. See bug 581253, which I'll port into my patches. > > // set the pref > >- var prefs = Cc["@mozilla.org/preferences-service;1"]. > >- getService(Ci.nsIPrefBranch); > >+ var prefs = Components.classes["@mozilla.org/preferences-service;1"]. > >+ getService(Components.interfaces.nsIPrefBranch); > > prefs.setCharPref("browser.bookmarks.editDialog.firstEditField", aNewField); > I take it there's no Services handy? Unfortunately not in all scopes we are executing this. (In reply to comment #10) > Comment on attachment 462468 [details] [diff] [review] > v2: bookmarks management, updated for comments > > >diff --git a/suite/browser/navigator.js b/suite/browser/navigator.js > ... > >+__defineGetter__("PluralForm", function() { > >+ Components.utils.import("resource://gre/modules/PluralForm.jsm"); > >+ return this.PluralForm; > >+}); > >+__defineSetter__("PluralForm", function (val) { > >+ delete this.PluralForm; > >+ return this.PluralForm = val; > >+}); > Odd place to put this, given that you don't add any consumers. I do, in browser-places.js, and the addons doorhanger patch will use it in navigator.js directly, so it's best to leave it here, I think. > >- <popup id="placesContext"/> > >+ <menupopup id="placesContext"/> > This is confusing... Well, that one may be better off in the browser-ui patch, but then, they all land together anyhow. > >+ var uri = PlacesUIUtils.createFixedURI(value); > >+ return true; > Nit: result not used, no need to assign it to a variable. I guess that's one more I should backport :) > >+ // HACK: as we're importing the actual PlacesUIUtils but that name is taken > >+ // by a cut-down history-specific version, store that latter one temporarily > >+ var HistoryUtils = PlacesUIUtils; > >+ Components.utils.import("resource:///modules/PlacesUIUtils.jsm"); > Why not import into a temporary scope? Because I think it's easier to remove this once we move history to that new core, isn't it? > >+# Provide another URI for the bookmark properties dialog so we can persist the attributes separately > >+ content/communicator/bookmarks/bm-props2.xul (bookmarks/bm-props.xul) > Why? (But you should use a chrome override rather than a copy.) mak says: We want to store size of the dialog and other localstore stuff for each mode, one mode is modal and the other one is something other... one of the two modes allows the dialog to be resized, so we want to persist the size only for the resizeable dialog. That sux, but that UI needs to be revised regardless. Please let us get this in the way it is now and port the revision when they do it. We have already been dragging this review along waaaaaaaaaaay to long as it is.

(In reply to comment #12) > (In reply to comment #9) > > >- <popup id="placesContext"/> > > >+ <menupopup id="placesContext"/> > > This is confusing... Ah, I missed that this is FF vs. SM - I'll need to check, maybe they have a bug there - or I did the diff to a not-really-current state, which I's wonder about.

OK, I don't find that <popup> on any side any more, I guess it has been fixed.

OK, this patch here integrates bug 581253 and addresses Neil's comments to the FF vs. SM diff as well.

(In reply to comment #12) > (In reply to comment #9) > Hmm, I'm generally more a fan of first importing all content and then skin, so > that any content stuff can never override any skin stuff just because of the > "later in processing" rule. How I see it is that we need places-specific stuff after global stuff, and places-specific skin last. > > > #ifdef ADVANCED_STARRING_UI > > What's this? > Something that really needs to go away, as I learned. So that means no more preprocessing? Yay! > (In reply to comment #10) > > Odd place to put this, given that you don't add any consumers. > I do, in browser-places.js, and the addons doorhanger patch will use it in > navigator.js directly, so it's best to leave it here, I think. In that case would you mind moving it to the top of the file, just after the real import? > > Why not import into a temporary scope? > Because I think it's easier to remove this once we move history to that new > core, isn't it? And the PlacesUIUtils module is incompatible with the history UI? > > Why? (But you should use a chrome override rather than a copy.) > mak says: We want to store size of the dialog and other localstore stuff for > each mode, one mode is modal and the other one is something other... one of the > two modes allows the dialog to be resized, so we want to persist the size only > for the resizeable dialog. OK, I guess it makes sense for now.

(In reply to comment #16) > (In reply to comment #12) > > (In reply to comment #9) > > Hmm, I'm generally more a fan of first importing all content and then skin, so > > that any content stuff can never override any skin stuff just because of the > > "later in processing" rule. > How I see it is that we need places-specific stuff after global stuff, and > places-specific skin last. That's another reasonable assumption, I guess ;-) > > > > #ifdef ADVANCED_STARRING_UI > > > What's this? > > Something that really needs to go away, as I learned. > So that means no more preprocessing? Yay! Hah, thanks for reminding me of one more thing I should do in this patch to follow this up ;-) > > (In reply to comment #10) > > > Odd place to put this, given that you don't add any consumers. > > I do, in browser-places.js, and the addons doorhanger patch will use it in > > navigator.js directly, so it's best to leave it here, I think. > In that case would you mind moving it to the top of the file, just after the > real import? Makes sense, will do. > > > Why not import into a temporary scope? > > Because I think it's easier to remove this once we move history to that new > > core, isn't it? > And the PlacesUIUtils module is incompatible with the history UI? The current new one is, yes, but once we move the history over this will be gone and history UI will use it. This hack is a temporary workaround as long as old history code doesn't use the new places core. I only added it because I saw errors and wanted to put off solving those things in history code when we port history over. I know it's not beautiful, but I really didn't want to pull in even more work into this patch set, let's dig into history UI in that followup.

This is only a slight bitrot update for jar.mn changes (yay for not preprocessing browser-places.js).

Pushed as http://hg.mozilla.org/comm-central/rev/b7f04f55aa20 after a formal "go" from Neil.

Comment on attachment 461283 [details] [diff] [review] diff of Firefox places files vs. SeaMonkey places files > <toolbox id="placesToolbox"> > <toolbar class="chromeclass-toolbar" id="placesToolbar" align="center"> >- <toolbarbutton id="back-button" class="toolbarbutton-1 chromeclass-toolbar-additional" >- command="OrganizerCommand:Back" >- tooltiptext="&backButton.tooltip;" >- disabled="true"/> >- >- <toolbarbutton id="forward-button" class="toolbarbutton-1 chromeclass-toolbar-additional" >- command="OrganizerCommand:Forward" >- tooltiptext="&forwardButton.tooltip;" >- disabled="true"/> >- >-#ifdef XP_MACOSX >- <toolbarbutton type="menu" class="tabbable" >- onpopupshowing="document.getElementById('placeContent').focus()" >-#else > <menubar id="placesMenu"> >- <menu accesskey="&organize.accesskey;" class="menu-iconic" >-#endif >- id="organizeButton" label="&organize.label;" >- tooltiptext="&organize.tooltip;"> >- <menupopup id="organizeButtonPopup"> >+ <menu id="menu_File"> >+ <menupopup id="menu_FilePopup"> So, it turns out that menubars in toolbars suck, unless you're very careful. Or limit it to the Mac, which has its own funky menu system anyway.

Comment on attachment 461283 [details] [diff] [review] diff of Firefox places files vs. SeaMonkey places files > <tree id="placeContent" class="placesTree" context="placesContext" > hidecolumnpicker="true" > flex="1" type="places" >- flatList="true" You forgot to unhide the columnpicker.

Comment on attachment 463202 [details] [diff] [review] v2.3: bookmarks management, updated for bitrot from browser-ui patch update >+ <hbox flex="1" id="placesView"> >+ <tree id="placesList" >+ class="placesTree" >+ type="places" >+ hidecolumnpicker="true" context="placesContext" >+ onselect="PlacesOrganizer.onPlaceSelected(true);" >+ onclick="PlacesOrganizer.onTreeClick(event);" >+ onfocus="PlacesOrganizer.onTreeFocus(event);" >+ seltype="single" >+ persist="width" >+ width="200" >+ minwidth="100" >+ maxwidth="400"> >+ <treecols> >+ <treecol anonid="title" flex="1" primary="true" hideheader="true"/> >+ </treecols> >+ <treechildren flex="1"/> >+ </tree> >+ <splitter collapse="none" persist="state"></splitter> >+ <vbox id="contentView" flex="4"> Enh: I'd like to be able to collapse this splitter with a grippy.

(In reply to comment #22) >(From update of attachment 463202 [details] [diff] [review]) >>+ <splitter collapse="none" persist="state"></splitter> >Enh: I'd like to be able to collapse this splitter with a grippy. Oh, and that persist is useless as it stands, as it needs a both a useful collapse attribute and an id!

(In reply to comment #20) > So, it turns out that menubars in toolbars suck, unless you're very careful. Ah, looks like we had to work around it already, see suite/themes/classic/communicator/toolbar.css, lines 52-54: toolbaritem > menubar { border-bottom: 0px none; }

(In reply to comment #24) > (In reply to comment #20) > > So, it turns out that menubars in toolbars suck, unless you're very careful. > Ah, looks like we had to work around it already I was about to answer that almost all our menubars are inside toolbars anyhow nowadays, aren't they?

Comment on attachment 463202 [details] [diff] [review] v2.3: bookmarks management, updated for bitrot from browser-ui patch update >diff --git a/suite/themes/classic/communicator/bookmarks/bookmarksManager.css b/suite/themes/classic/communicator/bookmarks/bookmarksManager.css >new file mode 100644 >--- /dev/null >+++ b/suite/themes/classic/communicator/bookmarks/bookmarksManager.css >@@ -0,0 +1,71 @@ >+ >+/* Place List, Place Content */ >+.placesTree { >+ margin: 0px; >+} >+ >+#placesList { >+ -moz-appearance: none; >+ margin: 0px; >+ border: none; >+ padding: 0; >+} >+ >+#placeContent { >+ -moz-appearance: none; >+ border: 0px; >+} All these rules could be replaced by class="plain" on the trees in question.

Comment on attachment 463202 [details] [diff] [review] v2.3: bookmarks management, updated for bitrot from browser-ui patch update Sorry for only commenting on these one at a time, it must get boring for you. >+ <menupopup id="menu_EditPopup"> >+ <menuitem id="orgCut" ... >+ <menuitem id="orgCopy" ... >+ <menuitem id="orgPaste" ... >+ <menuitem id="orgUndo" ... >+ <menuitem id="orgRedo" Correct order is: Undo Redo ---- Cut Copy Paste Delete ---- Select All ---- Move...

(In reply to comment #27) > Sorry for only commenting on these one at a time, it must get boring for you. Not boring, but my TODO bugmail explodes to an amount where I start feeling like ignoring it all ;-)

Comment on attachment 463202 [details] [diff] [review] v2.3: bookmarks management, updated for bitrot from browser-ui patch update >diff --git a/suite/common/bookmarks/bookmarksManager.xul b/suite/common/bookmarks/bookmarksManager.xul I think this covers all the remaining issues I have with this particular file. >+ <!-- Command Keys --> >+ <key id="placesKey_find:all" >+ command="OrganizerCommand_find:all" >+ key="&cmd.find.key;" >+ modifiers="accel"/> >+ <key id="placesKey_find:current" >+ command="OrganizerCommand_find:current" >+ key="&cmd.find.key;" >+ modifiers="accel,shift"/> It wasn't clear that these keys existed. They're much better than the Alt+K and Alt+R keys, especially when there's no R in the folder name (Have you seen how ugly that looks?). I think it would be nice to add them to the menu, maybe the Edit menu? Bonus points for dynamically updating the label, so that it becomes obvious what they do. >+ <spacer flex="1"/> Shouldn't this be a toolbarspring? >+ class="placesTree" As far as I can see, class="plain" works just as well, or better (i.e. all the border/margin/appearance styles can be removed from the CSS). >+ persist="width" [Would need to persist collapsed too, for the grippy] >+ <splitter collapse="none" persist="state"></splitter> [Would need an id and collapse="before" and a grippy to make persist="state" make sense. Also might as well use resizeafter="grow" since you're not persisting the width of the following element.] >+ <vbox id="contentView" flex="4"> >+ <deck id="contentDeck" flex="1"> >+ <vbox id="defaultView" flex="1"> >+ <vbox id="searchModifiers" hidden="true"> >+ <toolbar id="organizerScopeBar" class="chromeclass-toolbar" align="center"> Somewhat excessive number of elements. Is this designed to be compatible with extensions overlaying stuff on to the bookmarks manager? If not, you do need one vbox, of course, but it only needs a flex of 1, and the only id that seems to get used is searchModifiers, or change the code to show organizerScopeBar. >+ <toolbarbutton id="scopeBarAll" type="radio" group="scopeBar" These toolbarbuttons look really crummy. An ordinary button looks much nicer. And without any extra style rules too! >+ <button id="saveSearch" class="small" This button also looks ugly, please drop the class="small" and CSS. But if you want to save space on the buttons, try class="small-margin" instead. >+ <tree id="placeContent" class="placesTree" context="placesContext" Again, class="plain" >+ hidecolumnpicker="true" Don't want this. >+ <treecol label="&col.name.label;" id="placesContentTitle" anonid="title" flex="5" primary="true" ordinal="1" I guess the ids here are for compatibility with Firefox? Otherwise I would have used id="title" and no anonid. [You don't need ordinal here. I think Firefox has it because they changed the id, which would have forgotten the ordinal, and the new column would have become the last column.] >+ <hbox id="infoPaneBox" style="height: 11em;"> Seems to be unnecessary (styling can me moved to the deck) >+ <deck flex="1" id="detailsDeck"> >+ <vbox id="editBookmarkPanelContent"/> >+ <spacer flex="1"/> <vbox id="editBookmarkPanelContent" flex="1"/> should work. >+ <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"/> Strange, I can't get the accesskey to work on the label. Maybe a XUL bug? (It works fine if I put the accesskey on the button.) >+ <spacer flex="1"/> Unnecessary.

Comment on attachment 463202 [details] [diff] [review] v2.3: bookmarks management, updated for bitrot from browser-ui patch update [Spinoff from the anonid discussion above] >+ // for string properties, use "name" as the id, instead of "title" >+ // see bug #386287 for details >+ var columnId = column.getAttribute("anonid"); >+ menuitemPrefix += columnId == "title" ? "name" : columnId; >+ label = PlacesUIUtils.getString(menuitemPrefix + ".label"); Why can't we rename the col.name.label string to col.title.label etc?

(In reply to comment #28) > (In reply to comment #27) > > Sorry for only commenting on these one at a time, it must get boring for you. > Not boring, but my TODO bugmail explodes to an amount where I start feeling > like ignoring it all ;-) Sometimes I wish I could ignore all my TODO review requests ;-)

(In reply to comment #27) > Comment on attachment 463202 [details] [diff] [review] > Correct order is: > Undo > Redo > ---- > Cut > Copy > Paste > Delete > ---- > Select All > ---- > Move... sounds correct and it is coherent with main Firefox Edit menu. I'ts ok to backport the change to our code.

commenting on backporting: (In reply to comment #29) > >diff --git a/suite/common/bookmarks/bookmarksManager.xul b/suite/common/bookmarks/bookmarksManager.xul > I think this covers all the remaining issues I have with this particular file. > > >+ <!-- Command Keys --> > >+ <key id="placesKey_find:all" > >+ command="OrganizerCommand_find:all" > >+ key="&cmd.find.key;" > >+ modifiers="accel"/> > >+ <key id="placesKey_find:current" > >+ command="OrganizerCommand_find:current" > >+ key="&cmd.find.key;" > >+ modifiers="accel,shift"/> > It wasn't clear that these keys existed. They're much better than the Alt+K and > Alt+R keys, especially when there's no R in the folder name (Have you seen how > ugly that looks?). I think it would be nice to add them to the menu, maybe the > Edit menu? Bonus points for dynamically updating the label, so that it becomes > obvious what they do. If we do that, they should appear only when a search is active, but the menu is becoming cluttered, so to backport this you'll need a separate file to evaluate that (even if atm the idea was to kill the library so it could get low traction) > >+ <spacer flex="1"/> > Shouldn't this be a toolbarspring? What's the usability difference from the spacer apart having a nice name? > >+ class="placesTree" > As far as I can see, class="plain" works just as well, or better (i.e. all the > border/margin/appearance styles can be removed from the CSS). seems ok, i'm just worried by the -moz-appearance: none stuff for native styling of trees (not that today we have something so nice) > >+ <vbox id="contentView" flex="4"> > >+ <deck id="contentDeck" flex="1"> > >+ <vbox id="defaultView" flex="1"> > >+ <vbox id="searchModifiers" hidden="true"> > >+ <toolbar id="organizerScopeBar" class="chromeclass-toolbar" align="center"> > Somewhat excessive number of elements. Is this designed to be compatible with > extensions overlaying stuff on to the bookmarks manager? If not, you do need > one vbox, of course, but it only needs a flex of 1, and the only id that seems > to get used is searchModifiers, or change the code to show organizerScopeBar. This stuff was containinig advanced UI and it was cut multiple times just to have it acceptable, thus it's a mess. Fee free to suggest/backport improvements > >+ <toolbarbutton id="scopeBarAll" type="radio" group="scopeBar" > These toolbarbuttons look really crummy. An ordinary button looks much nicer. > And without any extra style rules too! > >+ <button id="saveSearch" class="small" > This button also looks ugly, please drop the class="small" and CSS. But if you > want to save space on the buttons, try class="small-margin" instead. will need ui-r to backport, but we all agree this stuff is messed up, nobody had time to clean it up. > >+ <treecol label="&col.name.label;" id="placesContentTitle" anonid="title" flex="5" primary="true" ordinal="1" > I guess the ids here are for compatibility with Firefox? Otherwise I would have > used id="title" and no anonid. > [You don't need ordinal here. I think Firefox has it because they changed the > id, which would have forgotten the ordinal, and the new column would have > become the last column.] yeah there was some crazy stuff happening here in the past, and this is the result. I probably tries some time ago to clean it up but did not have time. > >+ <hbox id="infoPaneBox" style="height: 11em;"> > Seems to be unnecessary (styling can me moved to the deck) don't recall why it is there > >+ <deck flex="1" id="detailsDeck"> > > >+ <vbox id="editBookmarkPanelContent"/> > >+ <spacer flex="1"/> > <vbox id="editBookmarkPanelContent" flex="1"/> should work. > > >+ <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"/> > Strange, I can't get the accesskey to work on the label. Maybe a XUL bug? > (It works fine if I put the accesskey on the button.) > > >+ <spacer flex="1"/> > Unnecessary. you can test these changes and backport them if testing is good.

(In reply to comment #30) > Comment on attachment 463202 [details] [diff] [review] > v2.3: bookmarks management, updated for bitrot from browser-ui patch update > > [Spinoff from the anonid discussion above] > > >+ // for string properties, use "name" as the id, instead of "title" > >+ // see bug #386287 for details > >+ var columnId = column.getAttribute("anonid"); > >+ menuitemPrefix += columnId == "title" ? "name" : columnId; > >+ label = PlacesUIUtils.getString(menuitemPrefix + ".label"); > Why can't we rename the col.name.label string to col.title.label etc? details are in the above bug, btw string is name and looks like Pike asked to change string to col.name. I did not look at it too deeply

(In reply to comment #33) > > >+ class="placesTree" > > As far as I can see, class="plain" works just as well, or better (i.e. all the > > border/margin/appearance styles can be removed from the CSS). > > seems ok, i'm just worried by the -moz-appearance: none stuff for native > styling of trees (not that today we have something so nice) Well, currently, you have that -moz-appearance:none in for your themes as well, right? And from what I'm seeing, http://mxr.mozilla.org/comm-central/source/mozilla/toolkit/themes/winstripe/global/global.css#259 and its brothers all have the same definitions.

(In reply to comment #29) > >+ <treecol label="&col.name.label;" id="placesContentTitle" anonid="title" flex="5" primary="true" ordinal="1" > I guess the ids here are for compatibility with Firefox? Otherwise I would have > used id="title" and no anonid. > [You don't need ordinal here. I think Firefox has it because they changed the > id, which would have forgotten the ordinal, and the new column would have > become the last column.] We really want to keep the same IDs as Firefox here because there are add-ons e.g. adding columns, which may want to insert before/after the right column.

(In reply to comment #30) > Why can't we rename the col.name.label string to col.title.label etc? We can, Firefox can't, see bug 386287. Well, perhaps now they could, but it would just be uselessly adding L10n work. With L20n, they'll be able to correct as well. :)

(In reply to comment #29) > >+ <!-- Command Keys --> > >+ <key id="placesKey_find:all" > >+ command="OrganizerCommand_find:all" > >+ key="&cmd.find.key;" > >+ modifiers="accel"/> > >+ <key id="placesKey_find:current" > >+ command="OrganizerCommand_find:current" > >+ key="&cmd.find.key;" > >+ modifiers="accel,shift"/> > It wasn't clear that these keys existed. They're much better than the Alt+K and > Alt+R keys, especially when there's no R in the folder name (Have you seen how > ugly that looks?). I think it would be nice to add them to the menu, maybe the > Edit menu? Bonus points for dynamically updating the label, so that it becomes > obvious what they do. I'll file a separate followup for those.

Patch is up as attachment 465496 [details] [diff] [review] on bug 585601 and I filed bug 586840 for those keys.

(In reply to comment #35) > (In reply to comment #33) > > > >+ class="placesTree" > > > As far as I can see, class="plain" works just as well, or better (i.e. all the > > > border/margin/appearance styles can be removed from the CSS). > > > > seems ok, i'm just worried by the -moz-appearance: none stuff for native > > styling of trees (not that today we have something so nice) > > Well, currently, you have that -moz-appearance:none in for your themes as well, > right? I've found it in a couple places, so it could be fine to use plain.

(In reply to comment #33) > (In reply to comment #29) > > >+ <!-- Command Keys --> > > >+ <key id="placesKey_find:all" > > >+ command="OrganizerCommand_find:all" > > >+ key="&cmd.find.key;" > > >+ modifiers="accel"/> > > >+ <key id="placesKey_find:current" > > >+ command="OrganizerCommand_find:current" > > >+ key="&cmd.find.key;" > > >+ modifiers="accel,shift"/> > > It wasn't clear that these keys existed. They're much better than the Alt+K and > > Alt+R keys, especially when there's no R in the folder name (Have you seen how > > ugly that looks?). I think it would be nice to add them to the menu, maybe the > > Edit menu? Bonus points for dynamically updating the label, so that it becomes > > obvious what they do. > If we do that, they should appear only when a search is active, but the menu is > becoming cluttered, so to backport this you'll need a separate file to evaluate > that (even if atm the idea was to kill the library so it could get low > traction) Actually I think it would be nice for people to know that Ctrl+Shift+F focuses the searchbar and switches into "search current folder" mode. > > >+ <spacer flex="1"/> > > Shouldn't this be a toolbarspring? > What's the usability difference from the spacer apart having a nice name? No flex required, and compatiblity with possible future toolbar customisation. > > >+ class="placesTree" > > As far as I can see, class="plain" works just as well, or better (i.e. all the > > border/margin/appearance styles can be removed from the CSS). > seems ok, i'm just worried by the -moz-appearance: none stuff for native > styling of trees (not that today we have something so nice) As KaiRo mentioned, you already turn that off (since you don't want the border). > This stuff was containinig advanced UI and it was cut multiple times just to > have it acceptable, thus it's a mess. Fee free to suggest/backport improvements The deck id="contentDeck" appears to be unnecessary (and used to waste a widget, although nowadays it might only be a layer or something); one of contentView and defaultView appear to be redundant; searchModifiers appears to be redundant, but you would have to move the show/hiding code on to the organizerScopeBar instead (or rename the toolbar to searchModifiers); after the toolbar you have a vbox that appears to be redundant; the infoPaneBox appears to be redundant, but you would have to move the CSS on to the detailsDeck instead. Also a couple of spacers could be removed (in one case moving the flex to the previous box). > > >+ <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"/> > > Strange, I can't get the accesskey to work on the label. Maybe a XUL bug? > > (It works fine if I put the accesskey on the button.) Actually it doesn't quite work if I put the accesskey on the button, because it's the letter "E" and therefore grabs the "Edit" menu acccess key :-(

(In reply to comment #29) >(From update of attachment 463202 [details] [diff] [review]) >>+ <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"/> >Strange, I can't get the accesskey to work on the label. Maybe a XUL bug? >(It works fine if I put the accesskey on the button.) I filed bug 586961, which is a regression from bug 547996.

(In reply to comment #42) > (In reply to comment #29) > >(It works fine if I put the accesskey on the button.) > I filed bug 586961, which is a regression from bug 547996. Although it might not be an easy fix, so we may have to work around it by setting the access keys on the button rather than the label.