383867 - Bookmark bar receives keyboard focus even when hidden

Status: RESOLVED FIXED

(Camino Graveyard :: Toolbars & Menus, defect)

Description

[Sean Murphy]

With full keyboard access enabled, items in the bookmark toolbar will accept key-focus.  This will happen, for example, when tabbing out of the last NSToolbar item.  Even if the bookmark toolbar is hidden, it will still accept key-focus on each item.

STR:

1. Enable OS X's full keyboard access (^F7 by default).
2. In Camino, show the bookmark toolbar.
3. Throw some pages onto the bookmark toolbar.
4. Observe how those bookmarks can accept key-focus, by first highlighting the search toolbar field and then tabbing out of it.
5. Now, hide the bookmark toolbar.
6. Repeat step 4, and notice that focus should have jumped directly from the search field into the browser content, not stopping off in the bookmark toolbar because it's not visible.
7. While focus still appears to be lost somewhere (but is actually still in the bookmark toolbar), show the toolbar again and you can see proof that one of those bookmark items will contain the key-view highlight indication.

When the bookmark toolbar view is hidden, it should be removed from the key-view loop.

Comment 1

[Sean Murphy]

Just to clarify: s/"bookmark toolbar"/"bookmark bar" above.

Comment 2

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

Related to bug 198153?

Comment 3

[Sean Murphy]

(In reply to comment #2)
> Related to bug 198153?

Hmm, even though the summary over there isn't aptly named, part of the latest patch on that bug does seem to be an attempt to fix this issue.  In addition, even more code in the patch steps on the toes of bug 152987 and attempts to fix that particular problem as well.

Comment 4

[Sean Murphy]

Attachment: patch

I am providing a patch here, as this bug now seems like the most appropriate place for fixing the exact problem mentioned in the summary.  Even after preventing the bookmark bar from participating in the key-view loop after it's hidden, there is still one mysterious tab stop between the toolbar and browser content (seems like a ChildView) which is what should be fixed in bug 198153.

The way we hide the bookmark bar is not to remove it from the view hierarchy but rather make it's visible rect zero, effectively making it invisible.  This is why it's buttons were still in the loop.

Ideally, I would have liked to do the following:

- (BOOL)acceptsFirstResponder
{
  return mIsShowing;
}

But doing this still allows the bookmark button subviews to participate in the key-view loop.  One other way would be to iterate over each button upon hiding, and call setEnabled:NO (as is done in the canidate patch on bug 198153).  I chose instead to augment our bookmark bar hiding process by simply performing a setHidden:YES on it.  Hiding a view implicitly hides that view's subviews, and in doing so, they are ignored during keyboard navigation.

Comment 5

[Stuart Morgan]

Comment on attachment 267868 [details] [diff] [review]
patch

Essentially all the current hiding code is now legacy, since we are 10.3+ and can use setHidden:. Rather than just add it and leave the old stuff, go ahead and remove most of the implementation entirely and replace it with a setHidden: and a resizeSubviewWithOldSize:, as well as turning isVisible into an isHidden passthrough (don't worry about whether we should change the interface just yet, that's something that will span more classes and we can handle later), and update BrowserContentViews to check for isHidden and do size calculations accordingly.

I'm planning on making the corresponding status bar change to BCV shortly as part of switching the status bar to setHidden: so we can add the menu toggle for it.

Comment 6

[Stuart Morgan]

Oh, and you'll need to adjust MainController's toggleBookmarksToolbar to check isVisible rather than using the frame info directly (which it really should always have been doing). It's possible there are other places that implementation knowledge has leaked, so you'll want to do a quick audit.

Comment 7

[Sean Murphy]

(In reply to comment #5)
> Essentially all the current hiding code is now legacy, since we are 10.3+ and
> can use setHidden:. Rather than just add it and leave the old stuff, go ahead
> and remove most of the implementation entirely and replace it with a setHidden:
> and a resizeSubviewWithOldSize:, as well as turning isVisible into an isHidden
> passthrough (don't worry about whether we should change the interface just yet,
> that's something that will span more classes and we can handle later), and
> update BrowserContentViews to check for isHidden and do size calculations
> accordingly.

Sounds good to me, and I'd feel much better about that approach.  Thanks for the quick review.

(In reply to comment #6)
> Oh, and you'll need to adjust MainController's toggleBookmarksToolbar to check
> isVisible rather than using the frame info directly (which it really should
> always have been doing). It's possible there are other places that
> implementation knowledge has leaked, so you'll want to do a quick audit.

Yeah, I actually did want to modify this along with my patch, but shied away since the -[BookmarkToolbar isVisible] property had always been available and I was afraid there was some reason we weren't toggling based upon that status already.

Comment 8

[Sean Murphy]

Attachment: removed legacy hiding

> -  BOOL            mIsShowing;

I know you said not to modify BookmarkToolbar's interface now (and under this bug), but I assumed you were more concerned with changes such as replacing isVisible/setVisible.  As a direct result of relying on isHidden/setHidden, the mIsShowing instance variable is no longer useful.

Comment 9

[Stuart Morgan]

Comment on attachment 268235 [details] [diff] [review]
removed legacy hiding

ivars aren't part of the interface, so no problem there.

>+  if (isVisible) {
>     [self reflowButtons];
>     [self setNeedsDisplay:YES];
>   }

This is all redundant to having called resizeSubviewsWithOldSize: after toggling hidden off.

>-    bmToolbarHeight = NSHeight([mBookmarksToolbar frame]);
>+    if (![mBookmarksToolbar isHidden])
>+      bmToolbarHeight = NSHeight([mBookmarksToolbar frame]);
>   }

I'm not seeing any work in the outer |if| block that makes sense to do while the bar is hidden; is there something I'm missing that prevents doing the isHidden check on the entire block?

Comment 10

[Sean Murphy]

Attachment: updated

Updated to reflect review comments.

Comment 11

[Stuart Morgan]

Comment on attachment 268312 [details] [diff] [review]
updated

So... because I wsan't thinking through some of the behaviors of resizeWithOldSuperviewSize:/resizeSubviewsWithOldSize:, everything about my last review except for removing |[self setNeedsDisplay:YES];| was wrong; sorry about that. (Feel free to argue with me when I'm wrong in the future ;) )

r=me on your previous patch without the setNeedsDisplay.

Comment 12

[Sean Murphy]

Attachment: patch 2, minus setNeedsDisplay

No problem Stuart.  I apologize on my part because I failed to notice the problem with those changes myself.  Testing resize behavior with the bar hidden somehow slipped my mind during that wave of updates.

Comment 13

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 268456 [details] [diff] [review]
patch 2, minus setNeedsDisplay

sr=pink

Comment 14

[Eiichi]

Comment on attachment 268456 [details] [diff] [review]
patch 2, minus setNeedsDisplay

It seems to be O.K to check in.
Miss to land?

Comment 15

[Stuart Morgan]

Nothing is landing until we've had time to address the build failure.

Comment 16

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

(In reply to comment #15)
> Nothing is landing until we've had time to address the build failure.

By which he means bug 386653.

Comment 17

[Stuart Morgan]

Landed on trunk and MOZILLA_1_8_BRANCH.