336949 - Tabbing behaviour between location bar, search field, and page is very odd

Status: RESOLVED FIXED

(Camino Graveyard :: Toolbars & Menus, defect, P3)

Description

[Chris Lawson (gone)]

Since the debut of the new resizeable/collapsible search field (thanks pink!), our tabbing behaviour has gone all wonky.

If I launch Camino with a fresh profile, then put focus in the location bar, then hit tab, I get into the search field. A second tab gets me into the page. That's exactly what should happen.

If I collapse the search field and focus the location bar, then hit tab, the focus moves to the now-collapsed search field. I can prove this by typing "bob" and hitting return, upon which I get a Google search for "bob". A further tab keypress gets me into the page.

If I then un-collapse the search field, I have on occasion (not every time) been able to get the search field into a condition where it won't receive keyboard focus AT ALL. Cmd-L, then tab, does nothing at all, and a subsequent tab will then focus the page.

Smokey managed to get his Camino into a tab-loop where focus would alternate between the location bar and the search field, and refused to go into the page at all.

I suspect we have some issues with nextKeyView and the responder chain that need to be addressed in BrowserWindowController.mm (and possibly in the ExtendedSplitView class we implement).

Comment 1

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

Note that fixing some of this probably depends on fixing bug 152987 and bug 198153.

As Chris noted, I got somewhat different results; I could either end up in the page all the time (and never hit the search field) or cycle between location and search and never end up in the page; I could never reproduce his "#1" scenario.

Comment 2

[Chris Lawson (gone)]

For instance, in ExtendedSplitView:

|collapseSubviewAtIndex| should probably remove from the responder chain anything in the collapsing subview that can accept nextKeyView.

It also looks like we might need to revise |toolbarWillAddItem| and  |toolbarWillRemoveItem| in BrowserWindowController, as they appear to be referring to the old individual location and search items (which can no longer be manipulated independently).

cl

Comment 3

[Chris Lawson (gone)]

Oh, one more thing. If I collapse search, close the window, then open a new window, I don't seem to have a phantom search field in the nextKeyView chain any more. One tab makes the focus disappear (bug 198153) and a second tab gets me back into page content. So whatever we're doing in building a new window and/or tearing down the old one seems to be working, but whatever we're doing in the toolbar in an existing window is inadequate.

cl

Comment 4

[Håkan Waara]

Attachment: Fix

Hack to make the search textfield smart about our special ExtendedSplitView.

One more radical way to fix it would be to make a NSView subclass use poseAsClass: to override isHidden to check for this.

Comment 5

[Simon Fraser [no longer active]]

I think a better fix would be to override previous/nextKeyView or previous/nextValidKeyView in the ExtendedSplitView to skip views which are collapsed.

Comment 6

[Håkan Waara]

Comment on attachment 221929 [details] [diff] [review]
Fix

Thanks for the comment. Looking for a better way to solve this.

Comment 7

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

Since Håkan is not actively working on these right now, -> 1.2

Comment 8

[Håkan Waara]

It's possible that making our custom split view less hacky (hiding the hidden view, instead of setting its width to 0) would fix this.

Comment 9

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

Setting priority per 1.6 roadmap.

Comment 10

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

FWIW, the most common behavior I see now is (with the search field collapsed), tab from the location bar simply causes the text in the location bar to become selected (i.e., as if I had just tabbed into it).

Comment 11

[Stuart Morgan]

Attachment: fix

Fixes the issue by calling setHidden: on the collapsed view, which takes it out of the tab chain without us having to try to muck with the chain manually. I'd have liked to do this in ExtendedSplitView, but there doesn't seem to be anything I can override in NSSplitView that's called when I need this to be done.

This bug has nothing to do with any hackiness on our part, btw; it's just yet another way NSSplitView sucks.

Comment 12

[Steve Jacobs (irc: encro)]

"nextKeyView Order for Toolbar (Tabbing Focus Issue)"

I'd just been talking to CL about this tonight on IRC as there seems to be also an issue with the nextKeyView responder in BrowserWindow.nib

When tabbing from the URL field across it seems to go somewhere else before moving to the NSSearch, there is no blue highlight but its likely the resize handle.

Some  users may want to be able to adjust the resize handle via the keyboard though but as far as I can see this doesnt currently appear to be possible.

A modified BrowserWindow.nib is attached with a ctrl drag from the url search field to the NSSearch and binding as nextKeyView

Comment 13

[Steve Jacobs (irc: encro)]

Attachment: nextKeyView Fixed version of BrowserWindow.nib

Comment 14

[Steve Jacobs (irc: encro)]

Scratch that, Stuart has already fixed this in https://bugzilla.mozilla.org/show_bug.cgi?id=330666

See also: https://bugzilla.mozilla.org/show_bug.cgi?id=358526

Comment 15

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

Comment on attachment 277246 [details] [diff] [review]
fix

With this patch applied, I can pretty consistently get in a situation where trying to expand the search field for the first time in a new window ends up not drawing the search field.  I've compared to a tinderbuild that has this morning's extra nib fix, so it's either this patch or my debug build being wonky ;)

STR:

1. Have the search field collapsed
2. Open a new window
3. Click on the splitter as if you're going to drag to size
--> The splitter jumps over to the default search field width before you start dragging
--> The search field is not drawn in the empty space
4. Click the splitter again, or actually manage to drag this time
--> Search field appears

I wonder if this is because the splitter isn't actually "moved" manually in this case?  If I do succeed in moving it before it springs away, the bug doesn't appear.

It's a minor thing AFAICT, so we can ignore it, or take it in follow-up bug, or whatever; I just wanted to mention it here because it seems that this patch causes the behavior.  

The patch does fix cl's "bob" behavior as intended.

Comment 16

[Stuart Morgan]

Clicking the divider doesn't expand it here; maybe they took that behavior out in 10.4? I can imagine why that might happen though, so I'll look for a possible fix.

Comment 17

[Håkan Waara]

Oh, I just r+:ed but realized you probably will want to fix Smokey's comment 15 before checking this in?

Comment 18

[Stuart Morgan]

Attachment: v2

I was able to repro Smokey's issue after all, and this fixes it. I was able to find a way to do it internally to ESV after all (and needed to, since that issue was related to ESV's view swapping).

Comment 19

[Håkan Waara]

Comment on attachment 278201 [details] [diff] [review]
v2

Ouch :)

Did Smokey test it? Maybe that would be a good idea, to make sure all cases are OK now.

Comment 20

[Håkan Waara]

Comment on attachment 278201 [details] [diff] [review]
v2

Oh, one thing: Wanna sprinkle some more comments over the if-statements? They are pretty heavy.

Comment 21

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

I did test the new patch, yes, and it continues to fix cl's "bob" problem as well as now fixing the view-showing bug I found earlier (hooray for frankenbuilds!)

Comment 22

[Stuart Morgan]

Comment on attachment 278201 [details] [diff] [review]
v2

Yeah, I meant to comment the inner section; I'll do that on checkin, or with any sr comments.

Comment 23

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 278201 [details] [diff] [review]
v2

sr=pink

Comment 24

[Stuart Morgan]

Landed on trunk and MOZILLA_1_8_BRANCH.

Comment 25

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

Setting this to FIXED per comment 24 and the "fixed1.8.1.8" keyword (1.8.1.8? shouldn't this be 1.8.1.7, since Gecko 1.8.1.7 hasn't shipped/etc.?)

If you have other issues, check bug 383871 first, and if the issue is not listed among those deps, file a new one.

Comment 26

[Stuart Morgan]

Oops, thanks.

(In reply to comment #25)
> (1.8.1.8? shouldn't this be 1.8.1.7, since Gecko 1.8.1.7 hasn't shipped/etc.?)

Probably; I just can't keep track. Changing.

Comment 27

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

No, you were right; Sam told me later that what we knew as 1.8.1.7 is being displaced to 1.8.1.8, and 1.8.1.7 is coming off of the 1.8.1.6 relbranch....  I'm out of the loop this time ;)