The following elements need accesskeys in the Bookmark Manager: * All elements in the bookmark context menu. * View->Show columns-> All elements. * Tools->Search bookmarks...-> Entire dialog.

Reassigning as per Bug #32644

> The following elements need accesskeys in the Bookmark Manager: > * All elements in the bookmark context menu. Fixed in bug 176359 > * View->Show columns-> All elements. Unable to fix unless we rewrite the way this menu is populated. Firefox doesn't use accesskeys here either > * Tools->Search bookmarks...-> Entire dialog. fixed in this patch

(In reply to comment #2) > > * View->Show columns-> All elements. > Unable to fix unless we rewrite the way this menu is populated. Firefox doesn't > use accesskeys here either We could do something similar to what I did for History, which was to define the menu in XUL rather than relying on the JS to populate it.

- statically populated 'Show Columns' menu - add accesskeys for all the items

Comment on attachment 355018 [details] [diff] [review] Part 2: Show Columns menu, v1 >+ var column = document.getElementById(colid); I don't like this here as the bookmarks tree column ids aren't guaranteed to continue to work in future as they are anonymous XBL elements. Instead I'd prefer to use the old loop over the bookmarks view columns (below) instead. >- if (element) >- if (columns[i].hidden == "true") >- element.setAttribute("checked", "false"); >- else >- element.setAttribute("checked", "true"); ...although this part of the code could probably be simplified! >+ var column = document.getElementById(colid); Same problem, but here the fix is to adapt onViewMenuColumnItemSelected (which you rendered obsolete but forgot to remove).

> >+ var column = document.getElementById(colid); > I don't like this here as the bookmarks tree column ids aren't guaranteed to > continue to work in future as they are anonymous XBL elements. Instead I'd > prefer to use the old loop over the bookmarks view columns (below) instead. done > > >- if (element) > >- if (columns[i].hidden == "true") > >- element.setAttribute("checked", "false"); > >- else > >- element.setAttribute("checked", "true"); > ...although this part of the code could probably be simplified! simplified to > element.setAttribute("checked", columns[i].hidden != "true"); !columns[i].hidden doesn't work here > >+ var column = document.getElementById(colid); > Same problem, but here the fix is to adapt onViewMenuColumnItemSelected (which > you rendered obsolete but forgot to remove). done, using onViewMenuColumnItemSelected again

Comment on attachment 355119 [details] [diff] [review] Part 2: Show Columns menu, v2 >+ if (element) >+ element.setAttribute("checked", columns[i].hidden != "true"); Nit: incorrect indentation here I was confused by that != "true" bit until I realised that bookmarksTree.xml has its own definition of columns that is overriding the one in tree.xml (don't worry about that though, we can fix that in a followup bug.) >+ var colid = aEvent.target.getAttribute("id").replace(/columnMenuItem:/, ""); Nit: you can use .id instead of .getAttribute("id")

Comment on attachment 355156 [details] [diff] [review] combined patch for checkin [Checkin: Comment 9] http://hg.mozilla.org/comm-central/rev/e35a6c66d085

The fix for this bug removed id="columnsPopup" for no apparent reason, breaking the Flat Bookmark Editing extension. :-( I guess I'll have to find a workaround, unless re-adding the ID for SM 2.0.1 would be approved. KaiRo, what's your opinion? I don't want to waste my time here...

Right, sorry about that. Re-adding.

Comment on attachment 414727 [details] [diff] [review] re-add the menupopup ID [Checkin: comment 12] http://hg.mozilla.org/comm-central/rev/bbeee32cd90d http://hg.mozilla.org/releases/comm-1.9.1/rev/6941672f1171