Closed Bug 198153 Opened 19 years ago Closed 13 years ago

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

Categories

(Camino Graveyard :: Accessibility, defect)

PowerPC
macOS
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: nathan, Assigned: murph)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(2 files, 6 obsolete files)

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.
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?
Nathan, if your patch is ready for review, be sure to set the appropriate
attachment flags for it.
Summary: follow on to 152987: tabbing from location bar to content requires two or more tab presses → (Follow-on to 152987:) Tabbing from Location Bar to content requires two or more tab presses
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."
Attachment #117687 - Flags: review?(pinkerton)
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?
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Component: URL Bar & Autocomplete → Accessibility
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.
Attachment #117687 - Flags: review?(pinkerton)
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.
maybe you can get it by wrapping around the other way?
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 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!).
Attachment #117687 - Attachment is obsolete: true
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 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.
Attachment #119567 - Flags: review?(bryner)
cc'ing aaronl, who added that... aaron, any thoughts on this patch?
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.
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.
<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).
Attached patch All kinds of focusing fixes (obsolete) — Splinter Review
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!
Attachment #120261 - Flags: review?(pinkerton)
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?
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).
Hm.  What if you check whether the focus controller is active?
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.
Attachment #119567 - Attachment is obsolete: true
Attachment #121208 - Flags: review?(bryner)
Attachment #121208 - Flags: review?(bryner) → review+
Attachment #119567 - Flags: review?(bryner)
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=.
Attachment #121208 - Flags: superreview+
Attachment #121208 - Flags: review?(saari)
Attachment #121208 - Flags: review+
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.
Attachment #121208 - Attachment is obsolete: true
Attachment #121244 - Flags: superreview?(bryner)
Attachment #121244 - Flags: review?(saari)
Attachment #121208 - Flags: review?(saari)
Attachment #121244 - Flags: superreview?(bryner) → superreview+
saari's on sabbatical, should i r= this?
Target Milestone: --- → Camino0.8
Attachment #121244 - Flags: review?(saari) → review?(pinkerton)
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!
Attachment #121244 - Flags: review?(pinkerton) → review+
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
ummm... you mean it CAN be non-null if !mDocument???
> 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
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

sigh. i was going to clean this up when i landed it. for the love of god will
someone give me an a=?!
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
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 #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".
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.
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.
landed the ESM patch, the camino patch doesn't apply cleanly. i'll take a look
in a day or two.
The ESM patch might have caused a regression: bug 218995, "accesskeys only
half-focus page textboxes if address bar has focus".
Whiteboard: haspatch
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.
Attachment #120261 - Flags: review?(pinkerton) → review?
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?
Taken
Assignee: pinkerton → qa-mozilla
Status: ASSIGNED → NEW
Attached patch updated patch to our new trunk (obsolete) — Splinter Review
mike this one applies and compiles.
Attachment #120261 - Attachment is obsolete: true
Attachment #120261 - Flags: review?
Attachment #134784 - Flags: review?
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
Attachment #134784 - Flags: review? → review-
Attached patch updated patchSplinter Review
Attachment #134784 - Attachment is obsolete: true
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?
Attachment #134849 - Flags: review-
Ludo could you update?
need more time to work on it.
Don't have the memory to debug this one rasigning to default owner.
Assignee: qa-mozilla → pinkerton
Ludo there is no escaping this bug anymore with your new PowerBook :) Let's fix
this baby ones and for all.
Summary: (Follow-on to 152987:) Tabbing from Location Bar to content requires two or more tab presses → [patch] (Follow-on to 152987:) Tabbing from Location Bar to content requires two or more tab presses
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. 
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?
Target Milestone: Camino0.8 → Camino0.9
This bug seems to be in a messy state. Can you update this, Ludo?
*** Bug 270684 has been marked as a duplicate of this bug. ***
Bug 236596, which apparently is caused by this fix, is causing us quite some 
pain. Please-please, do something to resolve it.
Any chance to back this patch out? Seems that very troublesome bug 236596 is
caused by this patch.
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.
FYI, the change in attachment 119567 [details] [diff] [review] is the problem.
Bug 152987 has regressed and is targeted for 1.0, so I doubt this will make 0.9....
Target Milestone: Camino0.9 → Camino1.0
As it stands, this won't make 1.0.
Target Milestone: Camino1.0 → Camino1.1
Summary: [patch] (Follow-on to 152987:) Tabbing from Location Bar to content requires two or more tab presses → (Follow-on to 152987:) Tabbing from Location Bar to content requires two or more tab presses
Depends on: 152987
QA Contact: bugzilla → accessibility
Summary: (Follow-on to 152987:) Tabbing from Location Bar to content requires two or more tab presses → Tabbing from Location Bar to content requires two or more tab presses
Whiteboard: haspatch
Giving to hwaara since he's working on this stuff anyway.
Assignee: mikepinkerton → hwaara
I'm sorry, I'm not (now).
Assignee: hwaara → nobody
Since Håkan is not actively working on these right now, -> 1.2
Target Milestone: Camino1.1 → Camino1.2
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*.
Target Milestone: Camino1.6 → ---
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
Assignee: nobody → murph
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.