9086 - [FIX] Focus not switched to body after click in scrollbar

Status: RESOLVED WORKSFORME

(Core :: DOM: UI Events & Focus Handling, defect, P4)

Description

[cpratt]

Build ID: 1999063008
Platform: Windows NT, Linux
(Haven't tried Mac OS yet - apprunner is still launching)

To reproduce:
- Load a page that is long (http://www.macintouch.com works well).
- Click in the Location: control to change focus away from the body.
- Click in the scrollbar to scroll the page.
- Press the down arrow key.

Result: The text insertion cursor moves to the right in the Location: control.

Expected result: By clicking in the scrollbar, focus should be switched to the
body of the document; pressing the Down arrow key should scroll the document
down.

Comment 1

[cpratt]

Same problem on Mac OS 8.6.

Comment 2

[Erin Pierce]

Followed reproduction directionsin description, acheived the same results using
a different page. Test case from description is accurate.

Comment 3

[Erin Pierce]

Attachment: text test case

Comment 4

[Erin Pierce]

Attachment: page that needs scrolling to use for test

Comment 5

[ekrock's old account (dead)]

Moving M17.

Comment 6

[ekrock's old account (dead)]

Bulk moving [testcase] code to new testcase keyword. Sorry for the spam!

Comment 7

[Jesse Ruderman]

Also, clicking a scrollbar for a frame should switch focus to that frame.

Comment 8

[joki (gone)]

Chris, I think this might fall into your zone

Comment 9

[saari (gone)]

Ah yes, this is a known bug, with a currently unknown solution. Maybe we can add 
an event handler to the scrollbar XBL to set focus to the document. We have to 
becareful to not kill the current focus if it is already in the correct document.

Comment 10

[saari (gone)]

nominating nsbeta3 because several people seem to get bitten by this... it is 
non-intuitive that focus isn't in the document you just scrolled.

Comment 11

[saari (gone)]

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

Comment 12

[saari (gone)]

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

Comment 13

[ckritzer (gone)]

Mass update:  changing qacontact to [e-mail]

Comment 14

[Loco]

Updating QA Contact.

Comment 15

[Loco]

saari-you think this should be nominated for RTM?  I'm not quite clear on the
problem here, so I'm having a hard time figuring out what is or is not supposed
to be going on here.  Thanks. -d

Comment 16

[Daniel Roberts]

I vote that this get fixed before RTM, because 1) this is a problem which 
potentially has a very wide audience, and 2) this works correctly in other 
products (like IE and Opera).  I agree it is a relatively minor annoyance, but 
it is still something that quite a lot of users will notice, and the perception 
of product quality by END USERS, who are used to this working CORRECTLY with 
products like IE or Opera will NOT be good.

I don't mean to be nitpicky, but I do agree this should be --'d if this were a 
rarely noticed problem, but it is not.  Little things like this (IMHO) 
contribute VERY much to the overall perception of product quality by the 
average end user.  For this reason, I think this bug MUST be fixed.

Dan Lorca: Did you read the testcase and the bug description here?  The problem 
is that the scrollbar is accepting focus, and not returning it to the body when 
it is clicked on.  Incidently, not only does this mean that the down arrow key 
does not work, but it also means the mouse wheel does not work.  You might want 
to check out bug 32344 (which is quite rightly a duplicate of this bug now) for 
more details on this aspect of it.  For this reason, I am cc'ing the mouse 
wheel expert, Brian Ryner, since he originally filed that bug.

Comment 17

[Jesse Ruderman]

Should this apply to all scrollbars, or just some/most?  When I read mail (OE), 
I use the keyboard to select messages and the mouse to scroll within the 
current message.  If scrolling the message moved focus to the pane with the 
message, my strategy wouldn't work.  (Mozilla has a keyboard shortcut to go to 
the next message, and so does OE, as I just realized.)

Comment 18

[saari (gone)]

The scrollbar is *not* accepting focus, which is correct. The incorrect part is
that it doesn't toss focus into the associated content document. As I stated
previously, we have to be a little careful with the logic so we don't kill the
focus if it is already in the correct document.

Since you wanted this rtm nominated, I've added the rtm keyword.

Comment 19

[Peter Trudelle]

rtm-

Comment 20

[saari (gone)]

Targeting Mozilla 1.0, would be good to fix

Comment 21

[Matthew T (active 1999-2002)]

See also bug 33012.

Comment 22

[saari (gone)]

Pretty annoying bug, target mozilla 0.9

Comment 23

[Loco]

Has this been improved at all since the fixes went in for scrolling mice?  It 
would seem that is has-marking

WORKSFORME.

Comment 24

[saari (gone)]

No, this is still very much present. Try the very first example with
macintouch.com. Reopening

Comment 25

[Hixie (not reading bugmail)]

Reassigning QA Contact for all open and unverified bugs previously under Lorca's
care to Gerardo as per phone conversation this morning.

Comment 26

[Erik Jacobsen]

Build: 2001020808
Pltfm: Linux

I suppose this is the same bug or highly related:

On opening a new window on a link via the middle mouse button,
the window will not scroll via the keyboard until the body or scrollbar is
clicked. Actually, the window acts as if it doesn't have keyboard focus
at all in this case. Same with Ctrl-N, unless I use the Location box.

Comment 27

[Jesse Ruderman]

Erik: if focus is ending up in the location bar after opening a new window (as 
opposed to on the page), that's bug 37638 or bug 53549.

Comment 28

[Erik Jacobsen]

Hmmm, thanks for the bug-pointer, <a
href="http://bugzilla.mozilla.org/show_bug.cgi?id=37638">Bug 37638</a> seems
most relevant and very similar. However, the behavior I have here is that the
new window does not get any keyboard events at all (using [a-zA-Z0-9] plus arrow
keys), including the Location box (and the previous window remains responsive to
the keyboard). So, to scroll the body in the new window, I first need to click
the page body, which is a bit jarring for me. I suppose I'm used to the 4.7x
experience, i.e.: "open link in new window" -> read visible portion of page ->
scroll down via keyboard. This is fvwm2, btw, using mouse focus.

Comment 29

[Jesse Ruderman]

Erik: hmm, I'd suggest you file another bug then.  I don't think the problem 
you're having is very closely related to this bug.

Comment 30

[Jesse Ruderman]

Btw, Erik, do you have the Sidebar enabled (but possibly collapsed) with the 
Search panel open?  If you turn off the Sidebar (F9) and then open another 
window, does your problem go away?  If so, your problem is probably related to 
the fix for bug 61417, and you should comment on that bug.

Comment 31

[Erik Jacobsen]

Jesse, thanks. Yes, I had the sidebar enabled but collapsed, with the search
panel open, so now I see where the keyboard focus was. Disabling the sidebar
leads immediately to bug 37638 ... the keyboard focus for the new window is
still not the page body, but the location box (and using an arrow key clears
that box!).

Comment 32

[joki (gone)]

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

Comment 33

[Madhur Bhatia]

QA contact updated

Comment 34

[saari (gone)]

->1.1

Comment 35

[Jesse Ruderman]

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

Comment 36

[Ed Lana]

The mail client also has this focus problem.

When using the mail client in three-pane mode the focus is not on message text
unless you specifically click in the message detail pane.  The scroll bar for
the message pane does not move the focus to the message.  Only clicking on the
text will bring the focus to the message detail.  The message and the scroll bar
should be linked.

Steps to reproduce this:

1. Open mail/news in three-pane mode.
2. Select a message thread.

At this point the focus is in the message thread pane.  If you press
the "down arrow" key the next message is selected and displayed in the message
detail pane.

(The fix related to bug 33507 is an exception to this because pressing the space
bar does not scroll the thread pane but instead the message detail scrolls.)

3. Move the message scroll bar and the message scrolls but the focus does not
move to the message detail it stays with the thread pane.

If you try to scroll through the message using the "down arrow" key the message
does not scroll.  Instead the next item in the thread pane is selected and the
text in the message pane changes.

4.  If you specifically click on the "message text" focus will then move to the
message detail pane and the "down arrow" key will scroll through the text.

I can imagine someone using the arrow key to scan through their messages.  So, I
don't think the focus should be changed from the thread pane simply because a
message is visible.  However if someone uses the scroll bar to scroll through
the message it seems reasonable that the focus should shift to message at that
time.  Then the arrow keys would scroll through the message rather than the
thread pane. Clicking on a new message in the thread pane would again return
focus to the thread pane.

It seems a little strange that the space bar would work on one pane while the
arrow keys are moving things in a different one, but that is a different bug.

Activating or Deactivating the sidebar with F9 does not have any effect on the
focus.

I have duplicated this on both:
Mozilla 1.1beta Build (2002072104) Windows 98 (Modern Theme) 
Mozilla 1.1beta (2002072203) Mac OS(X) 10.1 (Classic Theme)

Comment 37

[Aaron Leventhal]

This is really not that hard to fix. A member of the community should step
forward and take this on so they can learn about Mozilla.

Comment 38

[Mats Palmgren (inactive)]

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

Comment 39

[Asa Dotzler [:asa]]

This affects Firebird too. Not sure whether or not the fixes will be
independent. Ben, do we need an additional bug for Firebird or will both apps
require the same fix?

Comment 40

[Asa Dotzler [:asa]]

Bryner, can you take a look at this and see if it's something easy to fix?

Comment 41

[Asa Dotzler [:asa]]

no patch. not likely for 1.6. 

Comment 42

[Martyr]

This does effect end-user perception of the product. I had a friend over for a
few days who had never used Moz before, and it annoyed her. Confirmed on Moz
1.6b, OS X, Mac.

Comment 43

[Ruben van Engelenburg]

Attachment: This patch makes sure the focus is being set to the body whenever the scrollbar is being used.

This is my first contribution to Mozilla, so I hope I did everything according
to the protocol.

Comment 44

[chris hofmann]

think its getting too late for 1.7a, try for reviews in beta

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I don't think this patch is correct. You need to focus some DOM content, you
can't just switch the focus to another widget. This is really up bryner's alley,
he's the one you want to talk to.

Comment 46

[Brian Ryner (not reading)]

Comment on attachment 140842 [details] [diff] [review]
This patch makes sure the focus is being set to the body whenever the scrollbar is being used.

I don't think this is correct, and it will probably also slow down scrolling.

It seems like the right way to go about this would be to call window.focus()
from an event handler (click or mouseup) on the scrollbar.  Perhaps via an XBL
event handler on the <scrollbar> (see scrollbar.xml).

Comment 47

[[e-mail]]

(In reply to comment #46)
>It seems like the right way to go about this would be to call window.focus()
>from an event handler (click or mouseup) on the scrollbar.  Perhaps via an
>XBL event handler on the <scrollbar> (see scrollbar.xml).
Not from XBL, because JavaScript might be disabled in content.

Comment 48

[chris hofmann]

no patch yet.  bryner still looking for a way to do this.

Comment 49

[Mats Palmgren (inactive)]

Taking bug, patch coming up...

Comment 50

[Mats Palmgren (inactive)]

Attachment: Patch rev. 1

Find the frame that we are scrolling and focus it.
If its scrolled view is different from the current keyboard
scrolled view (nsPresShell::GetViewToScroll) then also move the
caret there (which will make nsPresShell target it for kbd scroll).

I added the code to the nsScrollbarButtonFrame class since it
already had a couple of static methods that are used from the Slider/
ScrollbarFrame classes.

A side-effect of the patch is also that we now get focus-outlines
for overflow:scroll boxes. I don't know if this is desirable or not.
(I kind of like it myself).

This fix depends on nsPresShell::GetViewToScroll() from bug 251986,
but I have included a copy of it here so you can test the patch.

Here are some URLs that should cover all possible use cases:
http://java.sun.com/j2se/1.3/docs/api/index.html  (FRAMESET)
http://bugzilla.mozilla.org/attachment.cgi?id=153476  (overflow:scroll)
http://bugzilla.mozilla.org/attachment.cgi?id=153477  (IFRAME with above)
http://bugzilla.mozilla.org/attachment.cgi?id=153478  (FRAMEs with above)
(the last three are the "complex 1" testcases from bug 97283)

Comment 51

[Brian Ryner (not reading)]

Comment on attachment 154883 [details] [diff] [review]
Patch rev. 1

>--- layout/xul/base/src/nsScrollbarButtonFrame.cpp	29 May 2004 00:09:04 -0000	1.37
>+++ layout/xul/base/src/nsScrollbarButtonFrame.cpp	31 Jul 2004 23:30:13 -0000
>@@ -275,3 +279,69 @@
>   nsRepeatService::GetInstance()->Stop();
>   return nsButtonBoxFrame::Destroy(aPresContext);
> }
>+
>+nsIScrollableView*
>+PresShell_GetViewToScroll(nsIPresContext* aPresContext)
>+{
>+  nsCOMPtr<nsIEventStateManager> esm = aPresContext->EventStateManager();

This doesn't need to be a COMPtr

>+  nsIFrame* selectionFrame = nsnull;
>+  nsIScrollableView* scrollView = nsnull;
>+  nsCOMPtr<nsIContent> selectionContent, endSelectionContent;  // NOT USED
>+  PRUint32 selectionOffset; // NOT USED
>+  esm->GetDocSelectionLocation(getter_AddRefs(selectionContent),
>+                               getter_AddRefs(endSelectionContent),
>+                               &selectionFrame,
>+                               &selectionOffset);
>+  if (selectionFrame) {
>+    nsCOMPtr<nsIScrollableViewProvider> svp = do_QueryInterface(selectionFrame);

Neither does this, since it's a frame (use CallQueryInterface).

>+    if (svp) {
>+      svp->GetScrollableView(aPresContext, &scrollView);
>+    } else {
>+      nsIView* selectionView = selectionFrame->GetClosestView();
>+      if (selectionView){
>+        scrollView = nsLayoutUtils::GetNearestScrollingView(selectionView);
>+      }
>+    }
>+  }
>+  if (!scrollView) {
>+    nsIViewManager* viewManager = aPresContext->GetViewManager();
>+    if (viewManager) {
>+      viewManager->GetRootScrollableView(&scrollView);
>+    }
>+  }
>+  return scrollView;
>+}
>+
>+void
>+nsScrollbarButtonFrame::FocusScrolledFrame(nsIPresContext* aPresContext,
>+                                           nsIFrame* aFrame)
>+{
>+  // Find the frame that we are scrolling and focus it.
>+  // If its scrolled view is different from the current keyboard
>+  // scrolled view (nsPresShell::GetViewToScroll) then also move the
>+  // caret there (which will make nsPresShell target it for kbd scroll).
>+  for ( ; aFrame; aFrame = aFrame->GetParent()) {
>+    nsCOMPtr<nsIScrollableViewProvider> svp = do_QueryInterface(aFrame);

Same as above.

>Index: layout/xul/base/src/nsScrollbarFrame.cpp
>===================================================================
>RCS file: /cvsroot/mozilla/layout/xul/base/src/nsScrollbarFrame.cpp,v
>retrieving revision 1.50
>diff -u -r1.50 nsScrollbarFrame.cpp
>--- layout/xul/base/src/nsScrollbarFrame.cpp	18 Apr 2004 14:30:36 -0000	1.50
>+++ layout/xul/base/src/nsScrollbarFrame.cpp	31 Jul 2004 23:30:13 -0000
>@@ -163,6 +163,7 @@
>                      nsGUIEvent*     aEvent,
>                      nsEventStatus*  aEventStatus)
> {
>+  nsScrollbarButtonFrame::FocusScrolledFrame(aPresContext, this);
>   return NS_OK;
> }
> 

You may want to patch nsNativeScrollbarFrame as well, since it's used in place
of nsScrollbarFrame on Mac.

Overall though, I'm still not convinced this is right.	If I have keyboard
focus in a scrollable iframe on the page, and I then grab the outer page
srcollbar and drag it, I'd like for the focus to remain in the iframe.	Why do
we actually want this behavior in the first place?  I can't find any other
browser that does this (tested IE6 and Safari).

Comment 52

[Wayne Mery (:wsmwk)]

Ryner in comment #51:
> Overall though, I'm still not convinced this is right.	If I have keyboard
> focus in a scrollable iframe on the page, and I then grab the outer page
> srcollbar and drag it, I'd like for the focus to remain in the iframe.	Why do
> we actually want this behavior in the first place?  I can't find any other
> browser that does this (tested IE6 and Safari).

On the contrary, I see current IE 6 scrolls the outer page after touching the scroll bar
https://bugzilla.mozilla.org/attachment.cgi?id=153477
http://java.sun.com/j2se/1.3/docs/api/index.html

Comment 53

[Ed Lana]

> Overall though, I'm still not convinced this is right.  If I have keyboard
> focus in a scrollable iframe on the page, and I then grab the outer page
> srcollbar and drag it, I'd like for the focus to remain in the iframe.  Why do
> we actually want this behavior in the first place?  I can't find any other
> browser that does this (tested IE6 and Safari).

The focus issue effects the Mozilla mail client. (see comment #36)

Comment 54

[Wayne Mery (:wsmwk)]

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

Comment 55

[Peter Reynolds]

I'm wondering whether my problem might be the same as this or whether anyone knows where I should look.  Can someone tell me please?  I'm using Thunderbird Trunk nightly.  Every day I paste a piece of text/HTML into an email.  The cursor is there at the end of what I have pasted, and I can type or backspace just fine.  But when I press CTRL-Home to go up to the top, it doesn't work until I use the mouse to click somewhere in the window.  I'm pretty sure this is a new problem in the last month or two.

Comment 56

[[e-mail]]

Isn't that bug 394264?

Comment 57

[Anne (:annevk)]

This seems to work fine these days.