Last Comment Bug 322239 - Missing accesskeys in Bookmark Manager.
: Missing accesskeys in Bookmark Manager.
Status: RESOLVED FIXED
: fixed-seamonkey2.0.1
Product: SeaMonkey
Classification: Client Software
Component: Bookmarks & History (show other bugs)
: Trunk
: All All
: -- minor (vote)
: seamonkey2.0a3
Assigned To: Vlado Valastiak [:wladow] @ Mozilla.sk
:
Mentors:
Depends on:
Blocks: accesskey
  Show dependency treegraph
 
Reported: 2006-01-03 12:23 PST by Vidar Haarr (not reading bugmail)
Modified: 2009-11-27 04:54 PST (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
add accesskeys, v1 (1.55 KB, patch)
2008-12-21 07:53 PST, Vlado Valastiak [:wladow] @ Mozilla.sk
neil: review+
neil: superreview+
Details | Diff | Review
Part 2: Show Columns menu, v1 (5.61 KB, patch)
2009-01-01 02:37 PST, Vlado Valastiak [:wladow] @ Mozilla.sk
neil: superreview-
Details | Diff | Review
Part 2: Show Columns menu, v2 (5.67 KB, patch)
2009-01-02 07:32 PST, Vlado Valastiak [:wladow] @ Mozilla.sk
neil: review+
neil: superreview+
Details | Diff | Review
combined patch for checkin [Checkin: Comment 9] (7.26 KB, patch)
2009-01-02 13:30 PST, Vlado Valastiak [:wladow] @ Mozilla.sk
no flags Details | Diff | Review
re-add the menupopup ID [Checkin: comment 12] (781 bytes, patch)
2009-11-26 06:59 PST, Vlado Valastiak [:wladow] @ Mozilla.sk
neil: review+
neil: approval‑seamonkey2.0.1+
Details | Diff | Review

Description Vidar Haarr (not reading bugmail) 2006-01-03 12:23:11 PST
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.
Comment 1 Ray Booysen 2006-02-13 07:44:18 PST
Reassigning as per Bug #32644
Comment 2 Vlado Valastiak [:wladow] @ Mozilla.sk 2008-12-21 07:53:41 PST
Created attachment 354050 [details] [diff] [review]
add accesskeys, v1

> 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
Comment 3 neil@parkwaycc.co.uk 2008-12-31 06:18:07 PST
(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.
Comment 4 Vlado Valastiak [:wladow] @ Mozilla.sk 2009-01-01 02:37:32 PST
Created attachment 355018 [details] [diff] [review]
Part 2: Show Columns menu, v1

- statically populated 'Show Columns' menu
- add accesskeys for all the items
Comment 5 neil@parkwaycc.co.uk 2009-01-01 15:56:02 PST
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).
Comment 6 Vlado Valastiak [:wladow] @ Mozilla.sk 2009-01-02 07:32:15 PST
Created attachment 355119 [details] [diff] [review]
Part 2: Show Columns menu, v2

> >+    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 7 neil@parkwaycc.co.uk 2009-01-02 13:17:14 PST
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 8 Vlado Valastiak [:wladow] @ Mozilla.sk 2009-01-02 13:30:18 PST
Created attachment 355156 [details] [diff] [review]
combined patch for checkin
[Checkin: Comment 9]
Comment 9 Serge Gautherie (:sgautherie) 2009-01-02 22:59:17 PST
Comment on attachment 355156 [details] [diff] [review]
combined patch for checkin
[Checkin: Comment 9]

http://hg.mozilla.org/comm-central/rev/e35a6c66d085
Comment 10 Jens Hatlak (:InvisibleSmiley) 2009-10-23 12:55:15 PDT
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...
Comment 11 Vlado Valastiak [:wladow] @ Mozilla.sk 2009-11-26 06:59:17 PST
Created attachment 414727 [details] [diff] [review]
re-add the menupopup ID [Checkin: comment 12]

Right, sorry about that. Re-adding.

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