402104 - Places Organizer window misses accesskeys

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect, P3)

Description

[Cédric Corazza]

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

Comment 1

[Cédric Corazza]

Attachment: Patch adding accesskeys to Places

Comment 2

[Cédric Corazza]

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

Adding requestee

Comment 3

[pal-moz]

DUPLICATE of bug 400703 ?

Comment 4

[Cédric Corazza]

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

[Aaron Leventhal]

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.

Comment 6

[Cédric Corazza]

Attachment: Patch 2 addressing also concerns of bug 400703

After talking with Aaron on IRC, this one should fix this bug and bug 400703.

Comment 7

[Aaron Leventhal]

>this one should fix this bug and bug 400703.

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

Comment 8

[Cédric Corazza]

Attachment: 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

Comment 9

[Cédric Corazza]

Attachment: Screenshot of the Places Organizer with simple buttons

The screenshot to see the UI changes with buttons instead of toolbar buttons

Comment 10

[Dietrich Ayala (:dietrich)]

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!

Comment 11

[Cédric Corazza]

Attachment: Patch with -u8p parameters as requested

Sorry, I wasn't aware of this... Here's the new patch with -u8p parameters

Comment 12

[Cédric Corazza]

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

Comment 13

[Cédric Corazza]

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?

Comment 14

[Asaf Romano (gone)]

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)

Comment 15

[Cédric Corazza]

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

Comment 16

[Asaf Romano (gone)]

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.

Comment 17

[Cédric Corazza]

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

Comment 18

[Asaf Romano (gone)]

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.

Comment 19

[Cédric Corazza]

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

Comment 20

[Asaf Romano (gone)]

So, alt+t goes the tag field when the popup is open but to the Tools menu otherwise?

Comment 21

[Cédric Corazza]

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.

Comment 22

[Asaf Romano (gone)]

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.

Comment 23

[Cédric Corazza]

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.

Comment 24

[Asaf Romano (gone)]

one alt+tab, you mean?

and, once you close the popup, alt+t works again for opening the tools menu?

Comment 25

[Asaf Romano (gone)]

I think we should rather allow setting different accesskeys for "More" and "Less" (and set them both to "e" in en-US).

Comment 26

[Cédric Corazza]

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

Comment 27

[Cédric Corazza]

Attachment: 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;

Comment 28

[Asaf Romano (gone)]

The *property* name is accessKey, not accesskey.

Comment 29

[Cédric Corazza]

Attachment: Patch with 2 accesskeys for More/less button

Thanks Asaf. It works fine now.

Comment 30

[Asaf Romano (gone)]

Comment on attachment 297045 [details] [diff] [review]
Patch with 2 accesskeys for More/less button

r=mano, thanks.

Comment 31

[Asaf Romano (gone)]

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

Comment 32

[Cédric Corazza]

Verified.
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9b3pre) Gecko/2008011414 Minefield/3.0b3pre

Comment 33

[Gervase Markham [:gerv]]

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