Missing accesskeys in Bookmark Manager.

RESOLVED FIXED in seamonkey2.0a3

Status

SeaMonkey
Bookmarks & History
--
minor
RESOLVED FIXED
12 years ago
8 years ago

People

(Reporter: Vidar Haarr (not reading bugmail), Assigned: wladow)

Tracking

(Blocks: 1 bug, {fixed-seamonkey2.0.1})

Trunk
seamonkey2.0a3
fixed-seamonkey2.0.1

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

12 years ago
Reassigning as per Bug #32644
Assignee: p_ch → nobody
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
Assignee: nobody → wladow
Status: NEW → ASSIGNED
Attachment #354050 - Flags: superreview?(neil)
Attachment #354050 - Flags: review?(neil)

Comment 3

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

Updated

9 years ago
Attachment #354050 - Flags: superreview?(neil)
Attachment #354050 - Flags: superreview+
Attachment #354050 - Flags: review?(neil)
Attachment #354050 - Flags: review+
Created attachment 355018 [details] [diff] [review]
Part 2: Show Columns menu, v1

- statically populated 'Show Columns' menu
- add accesskeys for all the items
Attachment #355018 - Flags: superreview?(neil)
Attachment #355018 - Flags: review?(neil)

Updated

9 years ago
Attachment #355018 - Flags: superreview?(neil)
Attachment #355018 - Flags: superreview-
Attachment #355018 - Flags: review?(neil)

Comment 5

9 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).
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
Attachment #355018 - Attachment is obsolete: true
Attachment #355119 - Flags: superreview?(neil)
Attachment #355119 - Flags: review?(neil)

Comment 7

9 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")
Attachment #355119 - Flags: superreview?(neil)
Attachment #355119 - Flags: superreview+
Attachment #355119 - Flags: review?(neil)
Attachment #355119 - Flags: review+
Created attachment 355156 [details] [diff] [review]
combined patch for checkin
[Checkin: Comment 9]
(Assignee)

Updated

9 years ago
Keywords: checkin-needed
(Assignee)

Updated

9 years ago
Target Milestone: --- → seamonkey2.0a3
Comment on attachment 355156 [details] [diff] [review]
combined patch for checkin
[Checkin: Comment 9]

http://hg.mozilla.org/comm-central/rev/e35a6c66d085
Attachment #355156 - Attachment description: combined patch for checkin → combined patch for checkin [Checkin: Comment 9]
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
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...
Created attachment 414727 [details] [diff] [review]
re-add the menupopup ID [Checkin: comment 12]

Right, sorry about that. Re-adding.
Attachment #414727 - Flags: review?(neil)
(Assignee)

Updated

8 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

8 years ago
Attachment #414727 - Flags: review?(neil)
Attachment #414727 - Flags: review+
Attachment #414727 - Flags: approval-seamonkey2.0.1+
(Assignee)

Updated

8 years ago
Keywords: checkin-needed
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
Attachment #414727 - Attachment description: re-add the menupopup ID → re-add the menupopup ID [Checkin: comment 12]
Status: REOPENED → RESOLVED
Last Resolved: 9 years ago8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Keywords: fixed-seamonkey2.0.1
You need to log in before you can comment on or make changes to this bug.