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

RESOLVED FIXED in Camino2.0

Status

RESOLVED FIXED
11 years ago
10 years ago

People

(Reporter: murph, Assigned: murph)

Tracking

(Blocks: 1 bug)

unspecified
Camino2.0
All
Mac OS X
Dependency tree / graph

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

11 years ago
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.
(Assignee)

Comment 1

10 years ago
Created attachment 331849 [details] [diff] [review]
Patch

Manually manages a key view loop for the BookmarkToolbar.
Attachment #331849 - Flags: review?
Status: NEW → ASSIGNED
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.
Attachment #331849 - Flags: review? → review?(hwaara)
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).
(Assignee)

Updated

10 years ago
Blocks: 152987

Updated

10 years ago
Attachment #331849 - Flags: superreview?(stuart.morgan+bugzilla)

Comment 4

10 years ago
Comment on attachment 331849 [details] [diff] [review]
Patch

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

Comment 5

10 years ago
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).
Attachment #331849 - Flags: superreview?(stuart.morgan+bugzilla)
Attachment #331849 - Flags: superreview-
Attachment #331849 - Flags: review?(hwaara)
(Assignee)

Comment 6

10 years ago
Created attachment 338652 [details] [diff] [review]
Patch, v2

Updated to reflect review comments.
Attachment #331849 - Attachment is obsolete: true
Attachment #338652 - Flags: superreview?(stuart.morgan+bugzilla)

Comment 7

10 years ago
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.
Attachment #338652 - Flags: superreview?(stuart.morgan+bugzilla) → superreview+
(Assignee)

Comment 8

10 years ago
Created attachment 338655 [details] [diff] [review]
Patch, for Check In
Attachment #338652 - Attachment is obsolete: true
(Assignee)

Comment 9

10 years ago
Created attachment 338656 [details] [diff] [review]
Patch, for Check In

Forgot one of the unsigned int changes :).
Attachment #338655 - Attachment is obsolete: true
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.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Camino2.0
You need to log in before you can comment on or make changes to this bug.