The default bug view has changed. See this FAQ.

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

VERIFIED FIXED

Status

Camino Graveyard
Preferences
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: Hixie (not reading bugmail), Assigned: froodian (Ian Leue))

Tracking

({fixed1.8.1.1, polish})

Details

Attachments

(1 attachment, 1 obsolete attachment)

1.74 KB, patch
Håkan Waara
: review+
Mike Pinkerton (not reading bugmail)
: superreview+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
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

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

See also bug 326599.

cl
Assignee: mikepinkerton → bugzilla
Keywords: polish

Updated

11 years ago
Hardware: PC → Macintosh
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.
QA Contact: preferences

Comment 3

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

Comment 4

11 years ago
this happens at http://lxr.mozilla.org/mozilla/source/camino/src/preferences/MVPreferencesController.mm#269
Blocks: 325880
(Assignee)

Comment 5

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

Comment 6

11 years ago
Created attachment 244411 [details] [diff] [review]
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").
Assignee: bugzilla → stridey
Status: NEW → ASSIGNED
Attachment #244411 - Flags: review?(hwaara)

Comment 7

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

Comment 8

11 years ago
Created attachment 244494 [details] [diff] [review]
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.
Attachment #244411 - Attachment is obsolete: true
Attachment #244494 - Flags: review?(hwaara)
Attachment #244411 - Flags: review?(hwaara)

Comment 9

11 years ago
Comment on attachment 244494 [details] [diff] [review]
Patch

It's confusing, but if it works...
Attachment #244494 - Flags: review?(hwaara) → review+
(Assignee)

Updated

11 years ago
Attachment #244494 - Flags: superreview?(mikepinkerton)
+      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.
(Assignee)

Comment 11

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

Comment 13

10 years ago
(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.
Attachment #244494 - Flags: superreview?(mikepinkerton) → superreview+
(Assignee)

Comment 15

10 years ago
Checked in on 1.8branch and trunk with mondo commenting.
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Keywords: fixed1.8.1.1
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.