241469 - Help interface not keyboard accessible

Status: VERIFIED FIXED

(SeaMonkey :: Help Documentation, defect)

Description

[Aaron Leventhal]

Bring up Help -> Contents

There are 4 tabs - Glossary, Index, Search, Contents

There is no way to switch between them using the keyboard.

* You should be able to tab to the currently open one and press Up/Down arrow
keys to switch between them.
* Ctrl+Tab and Ctrl+Shift+Tab should switch the currently visible one.

Comment 1

[Aaron Leventhal]

One more thing to do, use accesskeys:

_G_lossary
_I_ndex
_S_earch
_C_ontents

so that Alt+letter navigates to the associated panel.

Comment 2

[R.J. Keller]

pkw: please file a bug for Firefox Help at
http://bugzilla.mozdev.org/enter_bug.cgi?product=firebirdhelp for this bug, so
that we can get this in both help viewers.

Comment 3

[Philip K. Warren]

Attachment: Patch v1

This patch adds support for the accesskeys for each of the help tabs, and also
adds support for switching between tabs with Ctrl+Tab/Ctrl+Shift+Tab. In
addition, I removed the use of the texttab-sidebar class (it doesn't seem to be
defined anywhere anymore). I also needed to inherit the "accesskey" attribute
in the sidebar xbl, otherwise the access keys were not underlined properly.

Comment 4

[R.J. Keller]

Comment on attachment 147086 [details] [diff] [review]
Patch v1

> +function showRelativePanel(direction) {
> +  // direction == 1 to view next help panel, -1 to view prev panel
> +  var buttons = [

May you please add this comment at the top of the method:

///showRelativePanel() - Shows the next panel in the Help Viewer (generally
supposed to be set
//   off if the user hits the tab key.
// direction - Set this parameter to 1 to go to the next panel, or -1 to go to
the previous panel.

I'd like to have a method description for each method in help.js.

Just to be picky, can't we use a boolean variable instead of 1 and -1? This way
seems sloppier.

> +function showRelativePanel(direction) {
> +  // direction == 1 to view next help panel, -1 to view prev panel
> +  var buttons = [
> +    'help-glossary-btn',
> +    'help-index-btn',
> +    'help-search-btn',
> +    'help-toc-btn'
> +  ];

Can we get this into a public variable outside the method? In the future I am
planning on implementing the ability to add sidebars to the help viewer. This
would cause problems with that change.

> +  var i, btn, selectedIndex = -1;
> +  for (i = 0; i < buttons.length; i++) {
> +    btn = document.getElementById(buttons[i]);
> +    if (btn.getAttribute("selected") == "true") {
> +      selectedIndex = i;
> +      break;
> +    }

<picky>Can we put the variable i inside of the for loop (for (var i = 0))
instead of what you have now? Since the variable isn't used anymore, it would
be most appropriate for it to go out of scope.

Comment 5

[R.J. Keller]

> +  var buttons = [

Once you move this outside the method, change it to sidebarButtons, too. buttons
is not a very descriptive name.

Comment 6

[[e-mail]]

I understand that Ctrl+[Shift+]Tab isn't available on the Mac so I would suggest
you support Ctrl+PgUp/Ctrl+PgDn as an alternative.

Also regarding adding sidebars it might be simpler if you just stepped though
the helpsidebar-box's alternate children, so selectedIndex would be an even
number being childNodes index of the help sidebar box's selected button.

Comment 7

[R.J. Keller]

> I understand that Ctrl+[Shift+]Tab isn't available on the Mac so I would suggest
> you support Ctrl+PgUp/Ctrl+PgDn as an alternative.

Instead of Ctrl+Tab? That wouldn't work because Ctrl+PgUp already has the
function in the <browser> area of help.

Comment 8

[R.J. Keller]

BTW, Neil mentioned on IRC that you should change "direction" parameter name to
"goForward" (then you can do selectedIndex += goForward ? 1 : -1;).

Thanks!

Comment 9

[Philip K. Warren]

(In reply to comment #6)
> I understand that Ctrl+[Shift+]Tab isn't available on the Mac so I would suggest
> you support Ctrl+PgUp/Ctrl+PgDn as an alternative.

Can you explain this a bit more (not too familiar with Macs)? Wouldn't Cmd+Tab
or Cmd+Shift+Tab work on Mac, or is there a conflict?

Comment 10

[Philip K. Warren]

Attachment: Patch v2

- I have changed the patch so it dynamically builds a list of the buttons which
are children of "helpsidebar-box", as Neil has described. This seems more
flexible than the previous patch and should work when new buttons are added.
- Renamed buttons array to sidebarButtons, even though it is still local to the
showRelativePanel function.
- Changed showRelativePanel to accept a boolean value and added a description
of the method.
- Moved variable 'i' within the loop.

Comment 11

[Aaron Leventhal]

Ctrl+tab and Ctrl+Shift+tab are available on Macs. See
http://www.mozilla.org/docs/end-user/moz_shortcuts.html#page_navigation
We already use that to switch tabs in the browser.

It's Cmd+tab and Cmd+Shif+tab that are not available on OS X. They're reserved
for switching among active apps.
http://www.mozilla.org/projects/ui/accessibility/mozkeylist.html#TAB

So, it's fine to use Ctrl+Tab and Ctrl+Shift+Tab. You can support
Ctrl+PgUp/Ctrl+PgDn as well if you want, since that is another shortcut that
often means to switch tabs. Since it's not as well known, and because you have
the accesskeys working, it's not a priority.

Comment 12

[Philip K. Warren]

Comment on attachment 147169 [details] [diff] [review]
Patch v2

Meant to change this to control instead of accel.

Comment 13

[Philip K. Warren]

Attachment: Patch v3

This is the correct one.

Comment 14

[R.J. Keller]

Comment on attachment 147171 [details] [diff] [review]
Patch v3

r=[e-mail],

Thanks for those changes! This patch looks great!

Comment 15

[[e-mail]]

(In reply to comment #11)
>Ctrl+tab and Ctrl+Shift+tab are available on Macs. See
>http://www.mozilla.org/docs/end-user/moz_shortcuts.html#page_navigation
>We already use that to switch tabs in the browser.
Does that mean that we need to get rid of this line?
http://lxr.mozilla.org/seamonkey/source/xpfe/global/resources/content/bindings/tabbrowser.xml#1430

Comment 16

[Philip K. Warren]

Comment on attachment 147171 [details] [diff] [review]
Patch v3

Requesting Mozilla 1.7 approval for a simple patch which enables keyboard
navigation of help.

Comment 17

[Philip K. Warren]

Fix checked in to trunk. Leaving open for 1.7 approval.

Checking in extensions/help/resources/content/help.js;
/cvsroot/mozilla/extensions/help/resources/content/help.js,v  <--  help.js
new revision: 1.54; previous revision: 1.53
done
Checking in extensions/help/resources/content/help.xul;
/cvsroot/mozilla/extensions/help/resources/content/help.xul,v  <--  help.xul
new revision: 1.49; previous revision: 1.48
done
Checking in extensions/help/resources/locale/en-US/help.dtd;
/cvsroot/mozilla/extensions/help/resources/locale/en-US/help.dtd,v  <--  help.dtd
new revision: 1.12; previous revision: 1.11
done
Checking in themes/classic/communicator/sidebar/sidebarBindings.xml;
/cvsroot/mozilla/themes/classic/communicator/sidebar/sidebarBindings.xml,v  <--
 sidebarBindings.xml
new revision: 1.3; previous revision: 1.2
done
Checking in themes/modern/communicator/sidebar/sidebarBindings.xml;
/cvsroot/mozilla/themes/modern/communicator/sidebar/sidebarBindings.xml,v  <-- 
sidebarBindings.xml
new revision: 1.12; previous revision: 1.11
done

Comment 18

[Asa Dotzler [:asa]]

Comment on attachment 147171 [details] [diff] [review]
Patch v3

a=asa (on behalf of drivers) for checkin to 1.7

Comment 19

[Asa Dotzler [:asa]]

When you check this into the branch, please make a posting to n.p.m.l10n
informing our localizers of this late-breaking change. Thanks.

Comment 20

[Philip K. Warren]

Checked into 1.7 branch.

Checking in extensions/help/resources/content/help.js;
/cvsroot/mozilla/extensions/help/resources/content/help.js,v  <--  help.js
new revision: 1.51.2.1; previous revision: 1.51
done
Checking in extensions/help/resources/content/help.xul;
/cvsroot/mozilla/extensions/help/resources/content/help.xul,v  <--  help.xul
new revision: 1.48.14.1; previous revision: 1.48
done
Checking in extensions/help/resources/locale/en-US/help.dtd;
/cvsroot/mozilla/extensions/help/resources/locale/en-US/help.dtd,v  <--  help.dtd
new revision: 1.11.14.1; previous revision: 1.11
done
Checking in themes/classic/communicator/sidebar/sidebarBindings.xml;
/cvsroot/mozilla/themes/classic/communicator/sidebar/sidebarBindings.xml,v  <--
 sidebarBindings.xml
new revision: 1.2.88.1; previous revision: 1.2
done
Checking in themes/modern/communicator/sidebar/sidebarBindings.xml;
/cvsroot/mozilla/themes/modern/communicator/sidebar/sidebarBindings.xml,v  <-- 
sidebarBindings.xml
new revision: 1.11.88.1; previous revision: 1.11
done

Comment 21

[Aaron Leventhal]

I still have a problem with this in that I can Ctrl+tab to one of the help
panels, or hit Alt+letter to get there -- but then I can't down arrow into the
tree/list view of items in the panel. It should automatically select the first
item if another item isn't already selected.

Comment 22

[Aaron Leventhal]

Reopening because it still isn't keyboard accessible unless I can arrow into
those tree views/lists. Am I doing something wrong?

Comment 23

[Stefan [:stefanh]]

It would be nice to have these shortcuts in the Help Window Shortcuts section of
the Help Viewer.

Comment 24

[Tracy Walker [:tracy]]

these keyboard shortcuts work for me with recent builds:
alt+g Glossary
alt+i Index
alt+s Search
alt+c Contents

But as Aaron pointed out, it is still not possible to get to the contents of the
selected tab via the keyboard

Can we give focus to the first item in the tab, then up and down arrows work.

Comment 25

[[e-mail]]

Attachment: Focus fix

Comment 26

[Aaron Leventhal]

Maybe this is an improvement, but there are still a number of problems.

1) There are dead items in the tab cycle, where focus is not visible
2) When a tab with a tree view is focused, it's not apparent until you down
arrow once. What should happen is that the first item gets selected if nothing
else is already selected.
3) It should only take one tab press to get out of a tree view and into the
content in the right pane. A shift+tab from there should get back to the tree view.

Comment 27

[[e-mail]]

Attachment: Improved focus fix

* Turns sidebar buttons into toolbarbuttons to stop them taking focus
* Adds class="focusring" to the trees to show focus in a theme-friendly way

Comment 28

[R.J. Keller]

(In reply to comment #27)
> Created an attachment (id=150097)
> Improved focus fix
> 
> * Turns sidebar buttons into toolbarbuttons to stop them taking focus
> * Adds class="focusring" to the trees to show focus in a theme-friendly way

Why are we having this class? The code doesn't seem to interact with it.

Also, why did you remove the second part of your original patch? I know that bug
244744 was checked in, but I'd still ilke to have it to prevent further logic
errors that may occur with other content packs.

Comment 29

[[e-mail]]

(In reply to comment #28)
>(In reply to comment #27)
>>* Adds class="focusring" to the trees to show focus in a theme-friendly way
> Why are we having this class? The code doesn't seem to interact with it.
It's something that themes support to make focussing trees more noticable.

Comment 30

[R.J. Keller]

Comment on attachment 150097 [details] [diff] [review]
Improved focus fix

> * Turns sidebar buttons into toolbarbuttons to stop them taking focus

Neil, I applied your patch and it doesn't fix this issue you noted in your
comment. The sidebar buttons cannot obtain focus ever and the trees tend to
take all the focus.

Comment 31

[[e-mail]]

The idea is to fix comment 26; parts 1 and 3 are dealt with by changing the
buttons to toolbarbuttons and part 2 should be covered by the focus ring.

Comment 32

[R.J. Keller]

Comment on attachment 150097 [details] [diff] [review]
Improved focus fix

OK, after talking with neil through email I decided to change to review+. This
patch looks good. Asking bryner for super-review.

Comment 33

[Aaron Leventhal]

Comment on attachment 150097 [details] [diff] [review]
Improved focus fix

Problems with this patch:
1. Ctrl+Tab and Ctrl+Shift+Tab no longer switch between help tabs.
2. Sometimes when you tab into the help content pane, then shift+tab back you
don't end up back in the list. For example, go to Index, arrow down to Account
Settings. Press Tab, then Shift+Tab. You should be back in account settings.
3. When you focus one of the tabs with a tree view for the first time, it's not
obvious you can arrow because nothing is highlighted yet. Why not follow my
previous suggestion and automatically select the first item if nothing else has
been selected yet? It is normal in a situation like this, to start selection on
the first item rather than have no obvious focus in the list.

Sorry, I'm putting r- (don't mean to step on rlk's toes), but it at least
should not break Ctrl+Tab and Ctrl+Shift+Tab, and would hopefully get this
finally completely right so we can put it to rest.

Comment 34

[R.J. Keller]

(In reply to comment #33)
> (From update of attachment 150097 [details] [diff] [review])
> Problems with this patch:
> 1. Ctrl+Tab and Ctrl+Shift+Tab no longer switch between help tabs.

Did it ever? I never knew it ever worked (I've never been able to get it to work
;)).

> 2. Sometimes when you tab into the help content pane, then shift+tab back you
> don't end up back in the list. For example, go to Index, arrow down to Account
> Settings. Press Tab, then Shift+Tab. You should be back in account settings.
> 3. When you focus one of the tabs with a tree view for the first time, it's not
> obvious you can arrow because nothing is highlighted yet. Why not follow my
> previous suggestion and automatically select the first item if nothing else has
> been selected yet? It is normal in a situation like this, to start selection on
> the first item rather than have no obvious focus in the list.

good catch!

> 
> Sorry, I'm putting r- (don't mean to step on rlk's toes), but it at least
> should not break Ctrl+Tab and Ctrl+Shift+Tab, and would hopefully get this
> finally completely right so we can put it to rest.
> 

No, feel free to change my reviews if I've made an error! I totally forgot to
test Ctrl+Shift+Tab.

Comment 35

[Aaron Leventhal]

Ctrl+Tab and Ctrl+Shift+Tab work in help for me:
Mozilla 1.8a2
Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8a2) Gecko/20040603

Comment 36

[[e-mail]]

(In reply to comment #33)
>(From update of attachment 150097 [details] [diff] [review])
>Problems with this patch:
>1. Ctrl+Tab and Ctrl+Shift+Tab no longer switch between help tabs.
Good catch. Fortunately it's a one line fix.

>2. Sometimes when you tab into the help content pane, then shift+tab back
>you don't end up back in the list.
It's always worked like this (at least, since 1.5 if not before)...

>3. When you focus one of the tabs with a tree view for the first time, it's
>not obvious you can arrow because nothing is highlighted yet. Why not follow
>my previous suggestion and automatically select the first item if nothing
>else has been selected yet?
Because selecting the first item will load it...

Comment 37

[Aaron Leventhal]

> >2. Sometimes when you tab into the help content pane, then shift+tab back
> >you don't end up back in the list.
> It's always worked like this (at least, since 1.5 if not before)...
Should we file a separate bug on that or do you want to fix it here? The reverse
focus cycle should be the exact reverse of the forward focus cycle. I need a
better way of saying that, but you know what I mean :)  

> >3. When you focus one of the tabs with a tree view for the first time, it's
> >not obvious you can arrow because nothing is highlighted yet. Why not follow
> >my previous suggestion and automatically select the first item if nothing
> >else has been selected yet?
> Because selecting the first item will load it...
That would be okay with me, because the first item in the Contents tab (which is
the tab that's selected when help opens) is "Help and Support Center" which is
the content that's already loaded. It would be fine if opening up the glossary
or index tab opened up a glossary or index page. When a list or tree view is
opened with nothing selected, a lot of users wouldn't know they could start
selecting by pressing down arrow. At least the first item should have a dotted
focus outline around it, even if it's not selected. Show me another accessible
app besides Mozilla that opens up a tree view or list with no items selected or
focused when the tree/list itself has focus.

Comment 38

[[e-mail]]

(In reply to comment #37)
>>>2. Sometimes when you tab into the help content pane, then shift+tab back
>>>you don't end up back in the list.
>>It's always worked like this (at least, since 1.5 if not before)...
>Should we file a separate bug on that
Do you really want tabbing into the help content pane to focus the first link in
the page, rather than the one that was just scrolled to?

>... the first item in the Contents tab ... is "Help and Support Center"
>which is the content that's already loaded.
Um... no. For a trivial example, Help/For IE Users.

Comment 39

[Aaron Leventhal]

(In reply to comment #38)
> (In reply to comment #37)
> >>>2. Sometimes when you tab into the help content pane, then shift+tab back
> >>>you don't end up back in the list.
> >>It's always worked like this (at least, since 1.5 if not before)...
> >Should we file a separate bug on that
> Do you really want tabbing into the help content pane to focus the first link in
> the page, rather than the one that was just scrolled to?
Ok, I see what the issue is. Strange. I never ran across this situation before,
so I'll have to think about it.

> 
> >... the first item in the Contents tab ... is "Help and Support Center"
> >which is the content that's already loaded.
> Um... no. For a trivial example, Help/For IE Users.
I'm not following the example -- that's the 3rd item in that tab. It wouldn't
automatically select the first item if that item was already selected. It would
only automatically select the first item when that tab gets focused, and nothing
else was selected.

Comment 40

[[e-mail]]

Actually I meant the browser's menu help -> for ie users...

Comment 41

[Aaron Leventhal]

For that one perhaps the initial selection should be on the relevant tree item
"For Internet Explorer Users". Wouldn't that be expected?

Comment 42

[[e-mail]]

Unfortunately in general it appears to be computationally expensive to determine
which entry in the tree corresponds to the loaded page.

Comment 43

[Aaron Leventhal]

(In reply to comment #42)
> Unfortunately in general it appears to be computationally expensive to determine
> which entry in the tree corresponds to the loaded page.

Thanks Neil. Are you going to post a new patch with Ctrl+Tab/Ctrl+Shift+Tab fixed?

Comment 44

[[e-mail]]

Attachment: Fixed fix

Comment 45

[Aaron Leventhal]

Comment on attachment 150276 [details] [diff] [review]
Fixed fix

Even though once you tab into content, it's hard to tab back to the
Glossary/Index/Search/Contents, using Tab or Shift+Tab, you can use F6 to get
back there. Also, the accesskeys work.

Anyway, this is a great improvement. We'll see how this all goes with end
users.

Thank you. r=aaronl

Comment 46

[Brian Ryner (not reading)]

Comment on attachment 150276 [details] [diff] [review]
Fixed fix

>--- help.js	17 May 2004 23:23:45 -0000	1.56
>+++ help.js	8 Jun 2004 15:55:57 -0000
>@@ -443,6 +443,7 @@ function showPanel(panelId) {
>   //add the selected style to the correct panel.
>   var theButton = document.getElementById(panelId + "-btn");
>   theButton.setAttribute("selected", "true");
>+  document.commandDispatcher.advanceFocusIntoSubtree(theButton);

does

theButton.focus();

not work?  or are you trying to focus something other than the button?

Comment 47

[[e-mail]]

(In reply to comment #46)
>(From update of attachment 150276 [details] [diff] [review])
>>+  document.commandDispatcher.advanceFocusIntoSubtree(theButton);
>
>does
>
>theButton.focus();
>
>not work?  or are you trying to focus something other than the button?
Correct, I want to focus the tree or search field depending on which panel is
exposed (in fact the toolbarbutton does not take focus).

Comment 48

[Aaron Leventhal]

Checked in for Neil. Did anyone file a bug on Firefox for this?

Checking in help.js;
/cvsroot/mozilla/extensions/help/resources/content/help.js,v  <--  help.js
new revision: 1.59; previous revision: 1.58
done
Checking in help.xul;
/cvsroot/mozilla/extensions/help/resources/content/help.xul,v  <--  help.xul
new revision: 1.51; previous revision: 1.50
done

Comment 49

[Aaron Leventhal]

Bug 251382 is the firefox bug.

Comment 50

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381