441828 - BookmarkToolbar's key view loop breaks easily; Cannot rely on automatic loop calculation.

Status: RESOLVED FIXED

(Camino Graveyard :: Accessibility, defect)

Description

[Sean Murphy]

The key view look of the BookmarkToolbar appears to work because, when the browser window first loads, a default geometric-position-based loop was auto generated by AppKit.  The key view loop is very fragile though, and will break down given the following facts:

Add a new BookmarkButton to the toolbar and try tabbing to it.  The existing loop was auto-generated upon the window's creation and will not account for any view that has appeared after this time.

Even worse: open a new window and delete a BookmarkButton.  Now you'll find
that the loop chain is broken, stopping at what is now the last button.  You
can never get past it into the browser view.  What happens is this last
button's nextKeyView winds up becoming nil, and then nextValidKey pointing back
to itself.

Due to the dynamic nature of the BookmarkToolbar, in that BookmarkButtons are frequently being added or removed, it cannot rely merely on the automatic key view loop calculation performed only once during initial creation.  It needs to properly manage its own key view loop.

I have a patch that will mimic the internal key view management behavior of an NSTabView.  The BookmarkToolbar's next key view can be set within IB, but then, in awakeFromNib, we store a reference to this view as the "original next key view."  We do this because whence buttons are populated, BookmarkToolbar's actual next key view should be the first button, otherwise all buttons would be skipped.  We then should set the nextKeyView of the final button to the original next key view reference, as set on the toolbar itself.

Comment 1

[Sean Murphy]

Attachment: Patch

Manually manages a key view loop for the BookmarkToolbar.

Comment 2

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

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

Håkan, since you've worked with this sort of code before, can you review Sean's patch?  It's much smaller than the one for bug 152987.

Comment 3

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

Comment on attachment 331849 [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 280963, or bug 152987 to land first).

Comment 4

[Stuart Morgan]

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

I'll look at this tonight to unblock bug 152987.

Comment 5

[Stuart Morgan]

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

>+  NSView*         mOriginalNextKeyView;

Call this mNextExternalKeyView to make the purpose clear (and update the comment about in later accordingly).

>   [self setNeedsDisplay:YES];
>+  [self setupKeyViewLoop];

reflowButtons: is way too often to call this method; that happens constantly during resizing, for example. Call it from rebuildButtonList, addButton, and removeButton (while it's overkill for the last two, it's probably not worth the extra code to special-case the fixup).

Comment 6

[Sean Murphy]

Attachment: Patch, v2

Updated to reflect review comments.

Comment 7

[Stuart Morgan]

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

>+  unsigned totalButtons = [mButtons count];
>...
>+  for (unsigned i = 0; i < totalButtons; i++) {

We generally prefer |unsigned int| to |unsigned|

>+    BookmarkButton* currentButton = [mButtons objectAtIndex:i];
>+    if (i == (totalButtons - 1))
>+      [currentButton setNextKeyView:mNextExternalKeyView];
>+    else
>+      [currentButton setNextKeyView:[mButtons objectAtIndex:(i + 1)]];
>+  }

Sorry, I missed this before; pull the last-button case out of the loop, as with the other patch.

sr=smorgan with those changes.

Comment 8

[Sean Murphy]

Attachment: Patch, for Check In

Comment 9

[Sean Murphy]

Attachment: Patch, for Check In

Forgot one of the unsigned int changes :).

Comment 10

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

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

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