STEPS TO REPRODUCE 1. Open Preferences 2. Click a tab 3. Examine the first UI component in that pane ACTUAL RESULTS It has a focus ring. EXPECTED RESULTS No focus ring unless the OS pref to make such things focussable is set.

Hixie's suggestion seems quite reasonable. I can take a look at this. See also bug 326599. cl

I recall us doing this for a reason when we redid the prefs, but I don't recall what that reason was. If the first item is a text field, of course, it should have a focus ring under all conditions.

When we fix this, we'll need to fix the spacing across all our pref panes, too. Right now it's wildly inconsistent (mostly in the vertical, where controls are spaced in all manner of arbitrary ways). cl

this happens at http://lxr.mozilla.org/mozilla/source/camino/src/preferences/MVPreferencesController.mm#269

[10:50pm] <Wevah> basically, you hook up the nextKeyView of the view itself to the first control [10:50pm] <Wevah> and then instead of focussing that no matter what, you check [view nextValidKeyView] [10:50pm] <Wevah> which automatically takes FKA into account, returning nil if there's nothing that should be focussed

This is kinda hackish, but what we have now is also kinda hackish, and in contrast this fixes the bug. ;) The comment pretty much covers what's going on - we need to test whether the first element is a validKeyView or not, and the only way to do that is approach it from one side or the other. Approaching from the view ("above") is out, so I approach from the next element ("below").

Comment on attachment 244411 [details] [diff] [review] A Solution >Index: src/preferences/MVPreferencesController.mm >=================================================================== >RCS file: /cvsroot/mozilla/camino/src/preferences/MVPreferencesController.mm,v >retrieving revision 1.25 >diff -u -8 -r1.25 MVPreferencesController.mm >--- src/preferences/MVPreferencesController.mm 25 Oct 2006 19:54:23 -0000 1.25 >+++ src/preferences/MVPreferencesController.mm 2 Nov 2006 08:43:32 -0000 >@@ -265,18 +265,25 @@ > [[NSNotificationCenter defaultCenter] > postNotificationName: MVPreferencesWindowNotification > object: self > userInfo: [NSDictionary dictionaryWithObjectsAndKeys:mWindow, @"window", nil]]; > > [mCurrentPaneIdentifier autorelease]; > mCurrentPaneIdentifier = [identifier copy]; > >- [mWindow setInitialFirstResponder:[pane initialKeyView]]; >- [mWindow makeFirstResponder:[pane initialKeyView]]; >+ NSView* initialKeyView = [pane initialKeyView]; >+ [mWindow setInitialFirstResponder:initialKeyView]; Why do you need this? I don't suppose it's even used, if we explicitly set the focus. >+ // Ideally we'd hook up the view's nextKeyView to the first element in the pane and set focus >+ // to |nextValidKeyView|, but we can't, since that view is in a different nib from the prefpanes. >+ // So to validate the first element (for FKA) we call |previousValidKeyView| on the second element >+ NSView* firstValidKeyView = [[initialKeyView nextKeyView] previousValidKeyView]; >+ if ([firstValidKeyView isEqual:initialKeyView]) >+ [mWindow makeFirstResponder:firstValidKeyView]; >+ It's a confusing fix... Isn't there some way you can test the first view if it's a valid keyview? Also, wouldn't [[initialKeyView previousKeyView] nextValidKeyView] make more sense, if you really want one of the first things to get focus (rather than, accidently, something at the very end)? If my memory serves me, there's something called selectNextValidKeyView, or something like that. Please also look around in the family of those methods. > [[mWindow toolbar] setSelectedItemIdentifier:mCurrentPaneIdentifier]; > } > else > { > NSRunCriticalAlertPanel( NSLocalizedString( @"Preferences Error", nil ), > [NSString stringWithFormat:NSLocalizedString( @"Could not load %@", nil ), > [self labelForPane:identifier]], nil, nil, nil ); > }

(In reply to comment #7) > > [mCurrentPaneIdentifier autorelease]; > > mCurrentPaneIdentifier = [identifier copy]; > > > >- [mWindow setInitialFirstResponder:[pane initialKeyView]]; > >- [mWindow makeFirstResponder:[pane initialKeyView]]; > >+ NSView* initialKeyView = [pane initialKeyView]; > >+ [mWindow setInitialFirstResponder:initialKeyView]; > > Why do you need this? I don't suppose it's even used, if we explicitly set the > focus. Yeah, good point. Lost. > It's a confusing fix... Isn't there some way you can test the first view if > it's a valid keyview? If so, the documentation is not forthcoming. > Also, wouldn't [[initialKeyView previousKeyView] > nextValidKeyView] make more sense, if you really want one of the first things > to get focus (rather than, accidently, something at the very end)? Conceptually yes, but in practice it doesn't work. I'm not really sure why, but if you want to play at home, selecting a pane when FKA is on won't select anything (but a single tab will select the correct item) and selecting a pane with space will do various inconsistent things to the focus. > If my memory serves me, there's something called selectNextValidKeyView, or > something like that. Please also look around in the family of those methods. Yeah, it's |selectKeyViewFollowingView|. Given that we have to manually check to make sure we're looking at the right item for validity (otherwise, we end up giving focus to the prefpane buttons when FKA is off), we might as well just call |makeFirstResponder| directly.

Comment on attachment 244494 [details] [diff] [review] Patch It's confusing, but if it works...

+ NSView* firstValidKeyView = [[initialKeyView nextKeyView] previousValidKeyView]; what if there is only 1 thing in the pane? what will nextKeyView give you? As long as it doesn't return nil, you're fine. You should double-check that case.

(In reply to comment #10) > As > long as it doesn't return nil, you're fine. You should double-check that case. Yeah, it does return nil (so it just doesn't focus anything in that case). Any suggestions for a better solution, or is this something we're willing to live with?

do we have any pref panels that might fit this bill? what would such a worst-case pane look like? we should document that downside if we do want to live with it so at least we know about it up front.

(In reply to comment #12) > do we have any pref panels that might fit this bill? No. It'd need to be just a single checkbox, or text field, or something. The closest we come (History) is 3 elements. > what would such a worst-case pane look like? Worst case is that (in the hypothetical single-item pane) the first element isn't selected with FKA on. For users with FKA off, they'd never notice any difference.

Comment on attachment 244494 [details] [diff] [review] Patch sr=pink comment the heck out of this limitation, and i guess i'm ok with it.

Checked in on 1.8branch and trunk with mondo commenting.

Just for the record, one consequence of this patch is that when you enable FKA while viewing in a sheet in one of the prefPanes (e.g., Cookies Exceptions) and then close the sheet (with FKA still enabled), nothing is focused in the prefPane. Tab does get you to the first item, though, so it's an odd and minor thing, but one that I don't feel warrants a bug.