280963 - Tabs are not focusable (when full keyboard access is on)

Status: RESOLVED FIXED

(Camino Graveyard :: Accessibility, defect, P3)

Description

[Asaf Romano (gone)]

Not as either native / safari's tabs, the new tab widget is not focusable when
"full keyboard access" is on.

Try the following in Safari, Camino 0.8.x branch and recent Camino trunk builds:
1. Turn on "Full Keyboard Access" is System Preferences->Keyboard
2. Try to tab to the tabbar (from the searchbar or something) and move between
tabs using the left/right arrows.

It should be pretty easy to draw a focus ring around the tab rect (at least in
Carbon ; ) )

Comment 1

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

There are a couple of other open issues with tabbing/focus issues that may or
may not be related: bug 152987 and bug 198153.

Comment 2

[Josh Aas]

tough call on whether or not to block the release for this. I think it'll take a
while to get it fixed (the fix might not be bad, but finding someone to do it
will  be). For now I don't think we should hold up the release, but we should
definitely get this into a minor update after 0.9 it it doesn't make it into 0.9.

Comment 3

[Nick Kreeger]

Taking

Comment 4

[Samuel Sidler (old account; do not CC)]

This would be great to have for 1.0, but is it practical? How hard is the fix going to be? I say move to 1.1.

Comment 5

[Simon Fraser [no longer active]]

This shouldn't be too hard, should it?

Comment 6

[Samuel Sidler (old account; do not CC)]

-> 1.1

Comment 7

[froodian (Ian Leue)]

-> hwaara, per big SoC meeting

Comment 8

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Mass un-setting milestone per 1.6 roadmap.

Filter on RemoveRedonkulousBuglist to remove bugspam.

Developers: if you have a patch in hand for one of these bugs, you may pull the bug back to 1.6 *at that point*.

Comment 9

[Sean Murphy]

Attachment: Patch

Key view loop support for the tab bar.

Comment 10

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 335084 [details] [diff] [review]
Patch

This functions well in my testing, even without the nib change for bug 152987, which I gather is needed for making the full loop work.  (That is, this patch does successfully insert the tabs, their close buttons, the scroll arrows, and the All Tabs menu successfully into our current dysfunctional loop, and the focus/key loop works as expected in the tab bar within the context of that known dysfunction.)

Comment 11

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Comment on attachment 335084 [details] [diff] [review]
Patch

We chatted about this at the meeting, and the patch does require and should be tested with the nib on bug 152987, but it doesn't require the patch from 152987 (that is, the nib lands with whichever bug of this, bug 441828, or bug 152987 to land first).

Comment 12

[Stuart Morgan]

Comment on attachment 335084 [details] [diff] [review]
Patch

>+  NSView*           mOriginalNextKeyView; // The next key view after the tab bar, as set from within Interface Builder.

Same name change here as in the bookmark bar bug.

>+  TabButtonView* firstTabButton = [(BrowserTabViewItem*)[mTabView tabViewItemAtIndex:0] buttonView];

It can happen that we get here in initial setup with firstTabButton being nil. That used to be okay because we'd call this method later even when the tab bar is hidden and it would be fixed up, but with my changes in bug 454030 that's no longer the case. You'll need a
  // If we don't have a tab button yet, just keep the current nextKeyView.
  if (!firstTabButton)
    return;
here.

>+  for (int currentButtonIndex = 0; currentButtonIndex < numberOfTabs; currentButtonIndex++) {
> [...]
>+    if (currentButtonIndex == (numberOfTabs - 1)) {
> [...]
>+    }
>+    else {

Rather than constantly checking to see if you are at the end, loop to one before the end and move the whole contents of the if block outside the loop

It's weird that the tab bar is reaching into the tab view's subviews to hook up the tab chain, but I can see where it could be messy to do the other way (and that's hardly the worst encapsulation problem in our tab system), so we can call this good for now.

>+  if ([[theEvent characters] isEqualToString:@" "] &&
>+      (firstResponder == mOverflowLeftButton ||
>+       firstResponder == mOverflowRightButton))
>+  {
>+    [super keyDown:theEvent];
>+    if ([firstResponder acceptsFirstResponder])
>+      [[self window] makeFirstResponder:firstResponder];
>+  }
>+  else {
>+    [super keyDown:theEvent];
>+  }

Move the [super keyDown:] to just above the |if| (since you are testing against the saved firstResponder, that will still work) and get rid of the else block.

Also, as a minor nit: right now the #pragma mark separates the drag-and-drop handling in this file from everything else, so put your new methods above to maintain that (otherwise the mark is really random).


With all the various patches brought together and the one fix above, the behavior looks great!

Comment 13

[Sean Murphy]

Attachment: Patch, v2

Updated to address review comments.

Comment 14

[Stuart Morgan]

Comment on attachment 338648 [details] [diff] [review]
Patch, v2

sr=smorgan

Comment 15

[Smokey Ardisson (offline for a while; not following bugs - do not email)]

Landed on cvs trunk along with bug 152987 and bug 441828; thanks Sean!

If you see a regression or, more likely, a case where the new loop breaks, please file a follow-up bug.