Vidar Haarr (not reading bugmail) Reporter
	Description • 19 years ago

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.

	Ray Booysen
	Comment 1 • 19 years ago

Reassigning as per Bug #32644

	Vlado Valastiak [:wladow] Assignee
	Comment 2 • 16 years ago

> 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

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

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

	Vlado Valastiak [:wladow] Assignee
	Comment 4 • 16 years ago

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

	neil@parkwaycc.co.uk
	Comment 5 • 16 years ago

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

	Vlado Valastiak [:wladow] Assignee
	Comment 6 • 16 years ago

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

	neil@parkwaycc.co.uk
	Comment 7 • 16 years ago

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")

	Serge Gautherie (:sgautherie)
	Comment 9 • 16 years ago

Comment on attachment 355156 [details] [diff] [review]
combined patch for checkin
[Checkin: Comment 9]

http://hg.mozilla.org/comm-central/rev/e35a6c66d085

	Jens Hatlak (:InvisibleSmiley)
	Comment 10 • 15 years ago

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

	Vlado Valastiak [:wladow] Assignee
	Comment 11 • 15 years ago

Right, sorry about that. Re-adding.

	Jens Hatlak (:InvisibleSmiley)
	Comment 12 • 15 years ago

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