198153 - Tabbing from Location Bar to content requires two or more tab presses

Status: RESOLVED FIXED

(Camino Graveyard :: Accessibility, defect)

Description

[Nathan Hamblen]

User-Agent:       Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.4a) Gecko/20030307 Chimera/0.7+
Build Identifier: Mozilla/5.0 (Macintosh; U; PPC Mac OS X Mach-O; en-US; rv:1.4a) Gecko/20030307 Chimera/0.7+

I wanted to get a patch attached to 152987 before it was closed but didn't make
it. I've got one ready that goes to content area in one tab press and doesn't
get stuck in the bookmarks bar.

Reproducible: Always

Steps to Reproduce:
1. Acquire focus in the location bar.
2. Press tab.
Actual Results:  
Either I end up in the bookmarks bar or I am in nowhere land where pressing the
cursor keys does nothing.

Expected Results:  
I should already be in the content area and able to scroll.

Comment 1

[Nathan Hamblen]

Attachment: another two line patch! Makes browserView the first responder.

By sending focus to the browserView (which we get from the browser wrapper
(which we get from the window controller)) we bypass that extra tabbing.

Q: Should we also try to make the focus go back to the location bar after
tabbing through the whole page, like Conrad mentioned in 152987?

Comment 2

[Greg K.]

Nathan, if your patch is ready for review, be sure to set the appropriate
attachment flags for it.

Comment 3

[Nathan Hamblen]

Comment on attachment 117687 [details] [diff] [review]
another two line patch! Makes browserView the first responder.

Oh, it's ready for review. I was just waiting to see if anyone said "I want
full-fledged tabbing in to, out of, and shift-tabbing back into the location
bar or nothing at all."

Comment 4

[Mike Pinkerton (not reading bugmail)]

so what happens when you have the system pref set to tab through all UI
controls? we should tab to the next button in the ui, or perhaps to the first
item in the personal toolbar? wouldn't this go directly to the browser? what
does safari do in that case?

and yes, we do want to go back to the urlbar when done tabbing through content
as conrad suggested. that should be pretty easy, it's just an interface with 2
methods. can you hook that up as well?

Comment 5

[Nathan Hamblen]

Comment on attachment 117687 [details] [diff] [review]
another two line patch! Makes browserView the first responder.

Removing my review request.  I'll post a more comprehensive patch at some point
(not sure when). If someone else starts working on this pls let me know.

Comment 6

[Nathan Hamblen]

It turns out Apple doesn't have its act together here. With "highlight: Any
control" selected in Full Keyboard Access (keyboard control panel), you should
be able to tab through all controls in the toolbar. Unfortunately, there is no
way to put the toolbar into the window's responder chain. Therefore the only way
to tab through all toolbar items (in any app) is to turn on full keyboard access
(different setting on same page) and press control F5. In Mail, this shifts
focus up to the toolbar, but as soon as you enter the search field, your next
tab is hardwired to go into the application window instead of the next toolbar
element. This breaks full keyboard access. In Safari, there is no way at all
that I can find to tab through toolbar elements (other than entry fields) or the
personal bookmarks toolbar. In both Mail and Safari the developers chose to
break full keyboard access to the toolbar in order to add specific toolbar
elements to the window's responder chain. That gives most users what they want:
tabbing between toolbar entry fields and the content window.

Having said ALL that, I think we should be doing about what Mail does. Hardwire
the location field (and search field when it's done) into the responder chain
(or a fake one). Until Apple improves the NSToolbar interface, there is no other
way to tab out of the toolbar. This will mean that you won't be able to tab
through every element of the toolbar; you'll get booted out once you get to the
location field. I will try to make it work so that the _personal_ toolbar is
properly tabable so that we are 5% more correct, and that much closer to
perfection whenever Apple gives us a way to tab out of the window toolbar.

I'll post something if I get anywhere with it.

Comment 7

[S Woodside]

maybe you can get it by wrapping around the other way?

Comment 8

[Nathan Hamblen]

Heh heh, yeah it is possible to avoid the death-trap select box in mail by
shift-tabbing, if that's what you mean, to wrap around the toolbar and get to
the buttons on the right. When you expect users to do that, though, I think you
drop any semblance of "accessibility." :\ I've convinced myself that the Mail
developers must have filed internal bugs with the Toolbar framework and that it
will be fixed eventually. It's a silly state of affairs right now.

Comment 9

[Nathan Hamblen]

Comment on attachment 117687 [details] [diff] [review]
another two line patch! Makes browserView the first responder.

The real patch is on its way, and I WILL get the toolbar to (virtually) be in
the window's key view loop, or die trying. When I'm done Camino will support
Apple's OS X accesibility features much better than any browser (and even
better than Mail!).

Comment 10

[Nathan Hamblen]

Attachment: Patches nsEventStateManager to fix a problem with nsIWebBrowserChromeFocus

The affected file is in mozilla/content/events. I'm posting this now because I
know the change will be needed for Camino focusing to work properly and because
I expect it to take longer to be reviewed since it's not Camino's code. Does
anyone know who needs to review and possibly sr this?

The change is necessary because after gecko calls our
nsIWebBrowserChromeFocus::FocusPrevElement() or FocusNextElement() method, it
calls nsEventStateManager::SetContentState(null, NS_EVENT_STATE_FOCUS) to
register that the content no longer has focus. The patched line was calling
SendFocusBlur with a flag to raise the window. Later on that flag triggered a
call to nsChildView::SetFocus(PR_TRUE). That call undoes the focus change that
our listener has already performed, setting the focus back to the content view.
Without this patch (or something similar in gecko) I don't see a way that we
can make use of nsIWebBrowserChromeFocus to get focus back from gecko. (It took
a LOT of gdb-ing to even figure out how the focus change was being thwarted). 

I've tested this patch on a mach-o build of Mozilla and it caused no change in
Mozilla's focusing behavior. Furthermore, from my reading of the file I can't
see any reason why you would want or need to raise the window at that point.
It's safe and can be checked in at any time to prepare for the rest of the fix
for this bug.

Comment 11

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 119567 [details] [diff] [review]
Patches nsEventStateManager to fix a problem with nsIWebBrowserChromeFocus

asking for review from bryner, he'd know best on this.

Comment 12

[Brian Ryner (not reading)]

cc'ing aaronl, who added that... aaron, any thoughts on this patch?

Comment 13

[Aaron Leventhal]

Before I added that, it always focused the window if it needed to. I added the
parameter for my FocusElementButNotDocument() method, which is needed when the
find dialog is up, and a link is found. The link gets focused within the content
document but the find dialog retains window focus.

The only other SendFocusBlur() call in Mozilla is the one you're patching in
SetContentState(). I assume that's for a very good reason that it has always
focused the new window of the content, if it's different.

Comment 14

[Nathan Hamblen]

It's not focusing the window of newly focused content. It's focusing the window
of the content that is losing focus. At least in the case I was testing there is
no new content being focused; it is leaving content for a focus change to the
browser window controls. I could be wrong but I don't think the line I changed
is reached under other circumstances.

Comment 15

[Nathan Hamblen]

<a
href="http://lxr.mozilla.org/seamonkey/source/embedding/browser/webBrowser/nsWebBrowser.cpp#1758">Big
fat unfinished embedding code.</a>
Hey... not funny! The setFocusAtFirstElement() / setFocusAtLastElement()
functions are just just stubs (despite some cheerful comments in
nsIWebBrowserFocus.idl). No wonder they weren't working. What that means is that
when you shift-tab from the location bar to the browser you will not get focus
on the last element form element of the page the way that you should. It's not a
HUGE problem, but it is a little disorienting. When it's working proprely
(mozilla, phoenix...) the whole focusing cycle feels a lot smoother. I'll
include the calls to these stubs in my patch (which is almost ready to post here).

Comment 16

[Nathan Hamblen]

Attachment: All kinds of focusing fixes

For this to work very well you want to also apply the state manager patch,
which does require a real "make -f client.mk [build]" to take effect. There
aren't any nib changes because appkit does a good job of building a sensible
key view chain on its own. This patch excludes a few superfluous NSImageViews,
etc, from becoming firstResponder, and adds to the chain the toolbar item views
that it is able to get to. The result is that tabbing and shift tabbing work
like they should. You can foul it up by editing the toolbar using cmnd-drag
(without opening the customize toolbar sheet), but as soon as you switch away
from the window and come back it rights itself. It's about as good as it can
get, I believe, until Apple gives us a more flexible interface to NSToolbars.

Also, works nicely with the new search field!

Comment 17

[Brian Ryner (not reading)]

nsEventStateManager's mCurrentFocus is overloaded; there are two things it can
mean when it's null:

1. focus has moved out of the document (which is the case you're looking at)
2. focus is on the document but not on any element

It sounds like in the case where (1) happens we definitely don't want to set
widget focus to the document.  However, I don't think we want to change the
behavior of (2).  There's a comment about this in
nsEventStateManager::ShiftFocusInternal().  I think if you do something similar
here and only pass TRUE to SendFocusBlur if the docshell has focus, that will
work for both cases.  saari, aaronl, sound ok to you?

Comment 18

[Nathan Hamblen]

I added in code to get the docshell and call docShell->GetHasFocus(&docHasFocus) but it's claiming that the document does have focus even in case (1).

Comment 19

[Brian Ryner (not reading)]

Hm.  What if you check whether the focus controller is active?

Comment 20

[Nathan Hamblen]

Attachment: Patches nsEventStateManager to conditionally raise the window on blur event

Brian: tried what you said, and it works. The focus controller is active when
tabbing between page elements but inactive when switching out the content area.

Comment 21

[Brian Ryner (not reading)]

Comment on attachment 121208 [details] [diff] [review]
Patches nsEventStateManager to conditionally raise the window on blur event

Sorry for the flag juggling.  I'll sr and let saari r=.

Comment 22

[Nathan Hamblen]

Attachment: Same as previous, but checks for null mDocument

Hold it, the last patch had a crasher. mDocument is sometimes null (maybe only
on HTTP redirects), and I wasn't checking for that. I've added a null check and
all is well now.

Comment 23

[Mike Pinkerton (not reading bugmail)]

saari's on sabbatical, should i r= this?

Comment 24

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 121244 [details] [diff] [review]
Same as previous, but checks for null mDocument

r=pink. now i gotta send email to get driver's approval. let the paperwork
begin!

Comment 25

[Brendan Eich [:brendan]]

Comment on attachment 121244 [details] [diff] [review]
Same as previous, but checks for null mDocument

Drive-by nit-picking: don't initialize nsCOMPtrs to nsnull, they auto-construct
to that; don't test 'if (fc)' outside the "then" part of the 'if (mDocument)'
test, since it can't be non-null if (!mDocument).

/be

Comment 26

[S Woodside]

ummm... you mean it CAN be non-null if !mDocument???

Comment 27

[Brendan Eich [:brendan]]

> ummm... you mean it CAN be non-null if !mDocument???

No.

+      // see comments in ShiftFocusInternal on mCurrentFocus overloading
+      PRBool fcActive = PR_FALSE;
+      nsCOMPtr<nsIFocusController> fc = nsnull;
+      if (mDocument)
+        fc = getter_AddRefs(GetFocusControllerForDocument(mDocument));
+      if (fc)
+        fc->GetActive(&fcActive);

is badly written. It should be:

+      // see comments in ShiftFocusInternal on mCurrentFocus overloading
+      PRBool fcActive = PR_FALSE;
+      if (mDocument) {
+        nsCOMPtr<nsIFocusController> fc;
+        fc = getter_AddRefs(GetFocusControllerForDocument(mDocument));
+        if (fc)
+          fc->GetActive(&fcActive);
+      }

/be

Comment 28

[Brendan Eich [:brendan]]

I perpetrated a double-negative, which is always confusing (even danm-moz was
waylaid).  Rephrase "it can't be non-null if (!mDocument)" to "it can be only 
null (i.e., it must be null) if !mDocument".  But I was right!

/be

Comment 29

[Mike Pinkerton (not reading bugmail)]

sigh. i was going to clean this up when i landed it. for the love of god will
someone give me an a=?!

Comment 30

[Brendan Eich [:brendan]]

pink: I'm tempted to be equally pissy/desperate in reply: "Poor planning on your
part doesn't make a crisis on my part."  It's not as if this patch couldn't have
landed a lot earlier than when 1.5b is being wrapped up, is already overdue, and
is impinging on 1.5 final.  That's not the best time to get back to this bug,
note that it should be landed, and expect instant gratification from drivers.

But I'm a nice guy (if sometimes pissy too ;-).  So I'll approve the cleaned-up
patch here to land as soon as 1.5b is done, for 1.5 final.

/be

Comment 31

[Mike Pinkerton (not reading bugmail)]

nathan, i'm ready to drive this in, but i'm unsure how to test it. it doesn't
seem to actually fix the problem in the summary. should it?

Comment 32

[S Woodside]

Comment #16: "For this to work very well you want to also apply the state
manager patch, which does require a real "make -f client.mk [build]" to take
effect." I don't know what the "state manager patch" is although the second
patch here "patches nsEventStateManager".

Comment 33

[Nathan Hamblen]

Sorry for being out of touch, I've been paddling around glacier bay all week.
Yes Mike, the combined patches are supposed to fix the problem in the summary. I
was afraid that it may have become out-of-date, so perhaps it has. "The state
manager patch" is patch #121244 and it is required for patch #120261 to work.
For the time being I don't have a build to work with and can't look at this, but
I know that patch #121244 fixes broken code in mozilla and should be checked in
to the trunk even if patch #120261 needs more work to fix the problem in the
summary.

Comment 34

[Mike Pinkerton (not reading bugmail)]

ah-ha, i missed the camino part of this patch. no wonder it didn't work. i'll
try to get this in asap. sorry.

Comment 35

[Mike Pinkerton (not reading bugmail)]

landed the ESM patch, the camino patch doesn't apply cleanly. i'll take a look
in a day or two.

Comment 36

[Jesse Ruderman]

The ESM patch might have caused a regression: bug 218995, "accesskeys only
half-focus page textboxes if address bar has focus".

Comment 37

[Asa Dotzler [:asa]]

Comment on attachment 120261 [details] [diff] [review]
All kinds of focusing fixes

We have a new Camino request flag which can be set multiple times for a patch.
Moving old review requests to the new flag. Sorry for the spam.

Comment 38

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 120261 [details] [diff] [review]
All kinds of focusing fixes

this patch fails to apply. can someone please fix it up to work with an updated
version of the trunk?

Comment 39

[Ludovic Hirlimann [:Usul]]

Taken

Comment 40

[Ludovic Hirlimann [:Usul]]

Attachment: updated patch to our new trunk

mike this one applies and compiles.

Comment 41

[Ludovic Hirlimann [:Usul]]

Attachment: greeee, patch without garbage. Applies on Camino current

Comment 42

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 134784 [details] [diff] [review]
greeee, patch without garbage. Applies on Camino current

|-updateToolbarKeyViews| duplicates code and won't work if there are > 2 custom
toolbar items. I think it needs to be rewritten

add a newline at the end of CHBrowserListener.mm

Comment 43

[Ludovic Hirlimann [:Usul]]

Attachment: updated patch

Comment 44

[Mike Pinkerton (not reading bugmail)]

Comment on attachment 134849 [details] [diff] [review]
updated patch

this patch is much better than the previous version because it allows you to
tab through buttons on the toolbar if "focus all buttons" is enabled.

however, if you have the url bar and the search bar on the toolbar and focus
the url bar, hitting tab doesn't focus the search bar.

isn't that the point of this patch?

Comment 45

[Jasper]

Ludo could you update?

Comment 46

[Ludovic Hirlimann [:Usul]]

need more time to work on it.

Comment 47

[Ludovic Hirlimann [:Usul]]

Don't have the memory to debug this one rasigning to default owner.

Comment 48

[Jasper]

Ludo there is no escaping this bug anymore with your new PowerBook :) Let's fix
this baby ones and for all.

Comment 49

[Martijn Wargers (dead)]

The ESM patch has possibly caused also - likewise as in comment 36 - more
regressions: bug 241942 and bug 236596.
Backing out the ESM patch seems to fix these bugs. 

Comment 50

[Mike Pinkerton (not reading bugmail)]

i landed some changes to allow tabbing to the search field and then into the
content area. we still need to land a bunch of these fixes, but with the
1.7branch locked down, it's probably not the best time. let's push this to 0.9.

can someone put together an updated patch with the gecko and CHBrowserView
changes we need?

Comment 51

[Josh Aas]

This bug seems to be in a messy state. Can you update this, Ludo?

Comment 52

[Jasper]

*** Bug 270684 has been marked as a duplicate of this bug. ***

Comment 53

[steffen.kuhnert]

Bug 236596, which apparently is caused by this fix, is causing us quite some 
pain. Please-please, do something to resolve it.

Comment 54

[Raoul Nakhmanson-Kulish]

Any chance to back this patch out? Seems that very troublesome bug 236596 is
caused by this patch.

Comment 55

[Thomas Winzig]

PLEASE, is anyone interested in the regressions this has caused???

If what everyone is saying is true, it has made XML/XSLT transformations useless
for any pages dealing with forms. This bug is killing us! That fact that it is
in 1.0 is very sad.

Comment 56

[Simon Fraser [no longer active]]

FYI, the change in attachment 119567 [details] [diff] [review] is the problem.

Comment 57

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

Bug 152987 has regressed and is targeted for 1.0, so I doubt this will make 0.9....

Comment 58

[Samuel Sidler (old account; do not CC)]

As it stands, this won't make 1.0.

Comment 59

[Chris Lawson (gone)]

Giving to hwaara since he's working on this stuff anyway.

Comment 60

[Håkan Waara]

I'm sorry, I'm not (now).

Comment 61

[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 62

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

Mass un-setting milestone per 1.6 roadmap.

Filter on RemoveRedonkulousBuglist to remove bugspam.

Developers: if you have a patch in hand for one of these bugs, you may pull the bug back to 1.6 *at that point*.

Comment 63

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

With FKA off and the search field hidden (or FKA on and the bookmark bar and search field hidden), one tab sends focus from the location bar into content following bug 152987 --> FIXED