327203 - First item in pref pane gets focus even when OS pref says not to

Status: VERIFIED FIXED

(Camino Graveyard :: Preferences, defect)

Description

[Hixie (not reading bugmail)]

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.

Comment 1

[Chris Lawson (gone)]

Hixie's suggestion seems quite reasonable. I can take a look at this.

See also bug 326599.

cl

Comment 2

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

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.

Comment 3

[Chris Lawson (gone)]

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

Comment 4

[froodian (Ian Leue)]

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

Comment 5

[froodian (Ian Leue)]

[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

Comment 6

[froodian (Ian Leue)]

Attachment: A Solution

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 7

[Håkan Waara]

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 );
>     }

Comment 8

[froodian (Ian Leue)]

Attachment: Patch

(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 9

[Håkan Waara]

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

It's confusing, but if it works...

Comment 10

[Mike Pinkerton (not reading bugmail)]

+      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.

Comment 11

[froodian (Ian Leue)]

(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?

Comment 12

[Mike Pinkerton (not reading bugmail)]

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.

Comment 13

[froodian (Ian Leue)]

(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 14

[Mike Pinkerton (not reading bugmail)]

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.

Comment 15

[froodian (Ian Leue)]

Checked in on 1.8branch and trunk with mondo commenting.

Comment 16

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

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.