Places Organizer window misses accesskeys

VERIFIED FIXED in Firefox 3 beta3

Status

()

Firefox
Bookmarks & History
P3
normal
VERIFIED FIXED
10 years ago
8 years ago

People

(Reporter: Cédric Corazza, Assigned: Cédric Corazza)

Tracking

({access})

Trunk
Firefox 3 beta3
access
Points:
---
Bug Flags:
blocking-firefox3 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 7 obsolete attachments)

(Assignee)

Description

10 years ago
Select the Bookmarks > Organize Bookmarks... menu, in the displayed window, it misses several accesskeys, especially in the bottom panel.
It also misses accesskeys in the Toolbar for "Organize" and "Import and Backup" buttons.
I will provide a patch till the end of this week
(Assignee)

Comment 1

10 years ago
Created attachment 287114 [details] [diff] [review]
Patch adding accesskeys to Places
Attachment #287114 - Flags: review?
(Assignee)

Comment 2

10 years ago
Comment on attachment 287114 [details] [diff] [review]
Patch adding accesskeys to Places

Adding requestee
Attachment #287114 - Flags: review? → review?(mano)

Comment 3

10 years ago
DUPLICATE of bug 400703 ?
(Assignee)

Comment 4

10 years ago
Aaron, is this bug a duplicate of bug 400703 ? If so, does the attached patch here adresses the concerns of bug 400703 ? Thanks

Comment 5

10 years ago
Not at dupe of bug 400703 -- the missing accesskeys is just one part of the accessibility problems in bug 400703. The buttons should be in the tab order, and even when the accesskeys are there (it is for one of the items), they don't seem to work.
Blocks: 400703
(Assignee)

Comment 6

10 years ago
Created attachment 288229 [details] [diff] [review]
Patch 2 addressing also concerns of bug 400703

After talking with Aaron on IRC, this one should fix this bug and bug 400703.
Attachment #287114 - Attachment is obsolete: true
Attachment #288229 - Flags: review?
Attachment #287114 - Flags: review?(mano)
(Assignee)

Updated

10 years ago
Attachment #288229 - Flags: review? → review?(mconnor)

Comment 7

10 years ago
>this one should fix this bug and bug 400703.

When you say "should", do you mean you tested it and it works?
(Assignee)

Comment 8

10 years ago
Created attachment 288346 [details] [diff] [review]
Patch changing toolbarbuttons for buttons with an hbox

No, I didn't test yesterday :-\ . The toolbar buttons were tabbable but pressing entry didn't do anything and the accesskeys didn't work.
I made some tests inserting a hbox for this three buttons and changing the toolbar buttons for simple buttons. This changes the UI but works. Buttons are tabbable and the accesskeys work.
I will attach a screenshot of the Places Organizer with these changes
(Assignee)

Comment 9

10 years ago
Created attachment 288347 [details]
Screenshot of the Places Organizer with simple buttons

The screenshot to see the UI changes with buttons instead of toolbar buttons
Comment on attachment 288346 [details] [diff] [review]
Patch changing toolbarbuttons for buttons with an hbox

Hi Cedric,

Can you please generate your patch using -u8p, per the instructions here:

http://developer.mozilla.org/en/docs/Creating_a_patch

Thanks!
(Assignee)

Comment 11

10 years ago
Created attachment 288352 [details] [diff] [review]
Patch with -u8p parameters as requested

Sorry, I wasn't aware of this... Here's the new patch with -u8p parameters
Attachment #288346 - Attachment is obsolete: true
(Assignee)

Comment 12

10 years ago
Comment on attachment 288229 [details] [diff] [review]
Patch 2 addressing also concerns of bug 400703

Removing review request as this patch doesn't fix all the concerns. Waiting for comments on the last attached patch
Attachment #288229 - Attachment is obsolete: true
Attachment #288229 - Flags: review?(mconnor)
(Assignee)

Updated

10 years ago
Attachment #288352 - Flags: review?(mconnor)
(Assignee)

Updated

10 years ago
Attachment #288352 - Flags: ui-review?(beltzner)
(Assignee)

Updated

10 years ago
Priority: -- → P2

Updated

10 years ago
Attachment #288352 - Flags: review?(mconnor) → review?(mano)
(Assignee)

Comment 13

10 years ago
I report here wonderings I spoke about in #granparadiso :

what's the status for bug 402104 ? I know it has only P2 priority, but I just wanted to know if it is the good way to fix it, though imho, I think either the original code or my patch broke the user experience, as this window, regarding the tool bar/Menu bar , has no equivalent in Firefox UI. I'm about to post a comment about it in the bug
[...]
My English is weak : these are supposed to be toolbar buttons (originally), but toolbar buttons are supposed to be shown as icons only, text only, or both : so they don't have accesskeys all over the Firefox UI. In this case, they have to drop down menus, so either toolbar buttons or simple button are not the appropriate way for this. Menu items are
Hope this was clearer...
Well, in short : should have I to propose a patch with menu items, or is this window going to be redesigned?
(Assignee)

Updated

10 years ago
Flags: blocking-firefox3?
Flags: blocking-firefox3? → blocking-firefox3+
Priority: P2 → P3
Target Milestone: --- → Firefox 3 M11
Comment on attachment 288352 [details] [diff] [review]
Patch with -u8p parameters as requested

This no longer applies, sorry. Also:
1. Please don't add accesskeys in menu in which find-as-you-type needs to work (folderMenuList)
2. Don't add accesskeys to disables controls (liveTitlesSeparator)
Attachment #288352 - Flags: review?(mano) → review-
(Assignee)

Comment 15

10 years ago
Created attachment 293592 [details] [diff] [review]
Patch addressing comment #14

This patch addresses comment #14.
I removed class="tabbable" as buttons are tabbable by default.
I'm still concerned by comment #12 :
We ended up using simple buttons as toolbarbuttons don't support accesskey. Now, these buttons are accessible, but one needs to make two steps to access the dropdown menus. For instance, for "Organize", one needs to make ALT+O and Enter, and then, to access the dropdown menu items, the only way is to use the down arrow, because pressing ALT+whatever will close the dropdown menu and put the focus on the button again.
imho, we should drop using the fancy toolbarbuttons or buttons with icons in this window and use regular menu items to get this accessible the way it does, unless there's a turnaround I'm not aware of or skilled to implement.
Attachment #288352 - Attachment is obsolete: true
Attachment #293592 - Flags: review?(mano)
Attachment #288352 - Flags: ui-review?(beltzner)
It would be better to just fix the accesskeys here, and leave the toolbar issues to a follow up. Please also make sure that the accesskeys used in the details pane also work (and don't break!) the panel in the main browser window.
(Assignee)

Comment 17

10 years ago
(In reply to comment #16)
> It would be better to just fix the accesskeys here, and leave the toolbar
> issues to a follow up.

Do you mean I should let the current "Organize", "Views" and "Import and Backup" items as they currently are and just fix the accesskeys in the details pane?

> Please also make sure that the accesskeys used in the
> details pane also work

They were working at the time when I compiled this patch

> (and don't break!) the panel in the main browser window.

I'm not sure to understand this part : did you notice something broken with the last patch? I didn't notice.

I didn't test the patch myself yet, I'm just pointing out that these accesskeys ought not to conflict with global accesskeys in the browser window (e.g. Alt+T may conflict with the Tools menu). And yes, let's go with a follow up for the toolbarbuttons->buttons issue.
(Assignee)

Comment 19

10 years ago
Created attachment 296731 [details] [diff] [review]
Final patch, hopefully

- All accesskeys work fine
- Switching back and forth between the places window and the main browser window, ALT+T, for instance, behaves as expected in both windows
- Find as you type feature also works as long as the focus is not in one of the fields/buttons of the details pane. Pressing ESC frees the focus, and the fayt feature works again, which is the expected behaviour I think.
I will open the followup bug after this one will be closed.
Attachment #293592 - Attachment is obsolete: true
Attachment #296731 - Flags: review?(mano)
Attachment #293592 - Flags: review?(mano)
So, alt+t goes the tag field when the popup is open but to the Tools menu otherwise?
(Assignee)

Comment 21

10 years ago
Yes, it does, even if you keep the Places window open and if you switch to the main browser window, ALT+T opens the Tools menu.
Not that:
1. Open main browser one
2. click twice on the star icon to open the popup
while the popup is open, alt+t should select the tags field, not open the tools menu.
(Assignee)

Comment 23

10 years ago
Ah, ok. I misunderstood your point. I was focussing on the Places window, not the popup window from the star icon.
I tested :
- double clicking on the star opens the popup with the text in the "Name" field selected.
- then, ALT+T put the focus in the Tags field and do not pop down the Tools menu; and one can tab cycle in the popup window.
one alt+tab, you mean?

and, once you close the popup, alt+t works again for opening the tools menu?
I think we should rather allow setting different accesskeys for "More" and "Less" (and set them both to "e" in en-US).
(Assignee)

Comment 26

10 years ago
(In reply to comment #24)
> one alt+tab, you mean?

I meant, once the popup is open, you can use tab to access any tabbable field/button inside the popup window. Pressing ESC close the popup.
> and, once you close the popup, alt+t works again for opening the tools menu?
Yes, it does.

(Assignee)

Comment 27

10 years ago
Created attachment 296997 [details] [diff] [review]
Attempt to add 2 accesskeys for the Less/More button

(In reply to comment #25)
> I think we should rather allow setting different accesskeys for "More" and
> "Less" (and set them both to "e" in en-US).
I ended up with that code to add 2 accesskeys for Less/More button. This doesn't work and I can't figure out why. To test it, I used "s" for &detailsPane.less.accesskey; but I only got the value of &detailsPane.more.accesskey;
The *property* name is accessKey, not accesskey.
(Assignee)

Comment 29

10 years ago
Created attachment 297045 [details] [diff] [review]
Patch with 2 accesskeys for More/less button

Thanks Asaf. It works fine now.
Attachment #296731 - Attachment is obsolete: true
Attachment #296997 - Attachment is obsolete: true
Attachment #297045 - Flags: review?(mano)
Attachment #296731 - Flags: review?(mano)
Comment on attachment 297045 [details] [diff] [review]
Patch with 2 accesskeys for More/less button

r=mano, thanks.
Attachment #297045 - Flags: review?(mano) → review+
(Assignee)

Updated

10 years ago
Keywords: checkin-needed
mozilla/browser/components/places/content/editBookmarkOverlay.xul 1.10
mozilla/browser/components/places/content/places.js 1.121
mozilla/browser/components/places/content/places.xul 1.102
mozilla/browser/locales/en-US/chrome/browser/places/editBookmarkOverlay.dtd 1.6
mozilla/browser/locales/en-US/chrome/browser/places/places.dtd 1.40
Status: NEW → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Keywords: checkin-needed
(Assignee)

Comment 32

10 years ago
Verified.
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008011414 Minefield/3.0b3pre
Status: RESOLVED → VERIFIED
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.