70616 - search sidebar steals dialog focus.

Status: RESOLVED FIXED

(SeaMonkey :: Search, defect)

Description

[andreww]

windows build 20011022605

Steps to repeat:

start mozilla

make sure that the search sidebar is "topmost"

quit mozilla

launch mozilla

immediately hit ctrl+l to open an "open location" dialog.  You must do this
quickly before the search sidebar fully loads.

do nothing until the search sidebar finishes loading.

The search sidebar will steal cursor focus from the open location dialog and you
will be unable to type in that dialog unless you click again into the dialog
field.  

This may seem sorta fringe, but I've had this happen to me 3 times now in the
past few days and I think it falls into the "polish" and correctness category...

Comment 1

[Jesse Ruderman]

The search sidebar code was modified to focus its textbox as soon as it's 
created in bug 61417.  Due to bug 41813 and bug 50665 (recently marked as dups 
of bug 28467, I'm not sure why), this focus change isn't local to the search 
sidebar but instead grabs focus from any other part of the Mozilla window or 
even another Mozilla window.

The dialog problem will stick around even after the textbox.focus() problem is 
fixed, though, because it's likely that there will still be a way to steal 
focus from other windows (perhaps window.focus()).

Comment 2

[Jesse Ruderman]

I filed bug 76621 for the problem with sidebars stealing focus, so the 
remaining problem here is that the parent window of a modal dialog can steal 
focus from the dialog.

Comment 3

[Jon Granrose]

this is very annoying.  I never even use search in my sidebar, so it's about to
get unselected in my sidebar, but every time I open a new browser window and
search sidebar is open, it grabs focus, so I can't use space to page down in the
main window, or type in the url bar to go to a new site.  This is in 2001091203
on win98.

do we have a target milestone for this or is this dependent on some other bug?

Comment 4

[Jesse Ruderman]

Granrose, that's covered by bug 76621.  This bug is about things outside of 
dialogs being able to steal focus from dialogs.

Comment 5

[Samir Gehani]

bryner,
Could you have look at this?
-- sidebar triage team

Comment 6

[Doug Turner (:dougt)]

Attachment: focus patch

Comment 7

[matt]

last attachment was by [e-mail] on Cathleens machine.

Comment 8

[Brian Ryner (not reading)]

Would a better way to fix this be to check whether the sidebar tab is the active
window, and then focus the textbox if it is?  This way you don't lose the
auto-focus, but it won't steal focus either.  This is currently being done in
navigator.js for the content/urlbar focusing.... cc'ing jag to see if he has any
comments on that.

Comment 9

[jag (Peter Annema)]

That sounds reasonable. The code would indeed be very similar to the code that
currently prevents us from stealing focus with backgrounded windows:

http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=navigator.js&root=/cvsroot&subdir=mozilla/xpfe/browser/resources/content&command=DIFF_FRAMESET&rev1=1.373&rev2=1.374

Comment 10

[Jesse Ruderman]

Setting focus within a window (or within a content area) should be one command, 
called focus().  Setting focus should not require having separate cases for 
when the window has focus and when it doesn't, and it certainly shouldn't 
require having enough privileges to access nsIWindowWatcher.  See bug 77675, 
bug 97067, bug 76621, and bug 41813 for some of the problems caused by the 
current way of handling focus.

Comment 11

[jag (Peter Annema)]

The problem is that not all platforms provide a |focus()| that doesn't activate
the window, so a similar logic (check whether the element's window is active and
if so just call focus, otherwise set focusedElement or focusedWindow) would need
to be put in all |focus()| implementations.

And then of course one would want a way to explicitely raise/activate a window
(or do we have that yet?)

Comment 12

[Jesse Ruderman]

>The problem is that not all platforms provide a |focus()| that doesn't activate
>the window, so a similar logic ... would need to be put in all |focus()| 
>implementations.

I'd rather that logic reside in the implementation of focus() rather than at 
each point where code needs to move focus within a window.

>And then of course one would want a way to explicitely raise/activate a window
>(or do we have that yet?)

I think most pop-under ads use window.focus() to bring the main page back to 
the front.  (In other words, some sites use window.focus() to hide the fact 
that the site is littering your desktop with ads.)  I'm not sure what you're 
supposed to use for "focus this frame" or "focus the content area, rather than 
the sidebar or the location bar" or "focus the content area rather than this 
text field, so the user can scroll the page with the arrow keys".

Comment 13

[Brian Ryner (not reading)]

This logic already exists for HTML text fields, see:
http://lxr.mozilla.org/seamonkey/source/content/html/content/src/nsHTMLInputElement.cpp#713

Perhaps we should just duplicate this for xul text fields?

Comment 14

[Brian Ryner (not reading)]

Not gonna make it for 0.9.7.

Comment 15

[Robert Ginda]

This affects chatzilla users with default channels too.  They start chatzilla,
which takes a few minutes to connect to the irc server and various initial
channels.  As each initial channel is successfully joined, the chatzilla input
box is focused, which of course, steals focus from whatever the user had been doing.

Comment 16

[sujay]

Claudius is search QA.

Also Samir, how come this is isn't assigned to you?

Comment 17

[basic]

isn't the xul text field using the html text field?

Comment 18

[basic]

Attachment: patch -- overide focus() method to use html:input focus() method

This should do the trick. Not sure if there are any bad things that might be
going on ;-). I've tested it with chatzilla, works like a charm.

Comment 19

[Alex Vincent [:WeirdAl]]

I used this patch, which is syntactically equivalent to basic's patch:

--- xpfe/global/resources/content/bindings/textbox.xml.bak	Wed Dec 19 03:58:08 2001
+++ xpfe/global/resources/content/bindings/textbox.xml	Sat Jan 12 12:01:18 2002
@@ -55,7 +55,7 @@
           this.inputField.select();
         </body>
       </method>
-
+      <method name="focus" action="this.inputField.focus();"/>
       <property name="controllers"    readonly="true" onget="return
this.inputField.controllers"/>
       <property name="textLength"     readonly="true" onget="return
this.inputField.textLength;"/>
       <property name="selectionStart" onset="this.inputField.selectionStart =
val; return val;"

Started Mozilla again (set to start mail/news).  Right around when the prompt
comes up to ask for my password, CRASH:  TB1571231Y, TB1570833H, TB1570821X.

Comment 20

[basic]

Attachment: patch -- overide focus() method to use html:input focus() method

sorry... that was bad. This should work.

Comment 21

[Alex Vincent [:WeirdAl]]

That time I just lost the textbox I was supposed to type my password into.  The
prompt came up fine, but there wasn't anything I could type into.

Mozilla/5.0 (Windows; U; Win98; en-US; rv:0.9.7+) Gecko/20020110

Comment 22

[basic]

Attachment: patch -- overide focus() method to use html:input focus() method

patch -- overide focus() method to use html:input focus() method

lets do this again, it better work this time or Alex will kill me.

Comment 23

[Alex Vincent [:WeirdAl]]

This time, I had no major regressions.  Repeating the reporter's testcase, I
observed the search panel has a blinking caret, and the dialog box had a
blinking caret and focus.  Chatzilla also refused for the first time to take
focus back when I switched to browse or look at my JS console.  Very nice.

Guess I might as well hang up on the coroner...

Comment 24

[basic]

I've been using moz with the last patch without any problems.
Brian: can you take a look at the patch?

Comment 25

[sujay]

--> search/claudius

Comment 26

[basic]

note that the patch I attached has nothing to do with sidebar as it changes the
xul <textbox> to use the <html:input>'s focus() method when it's focus() method
is called.

Comment 27

[Brian Ryner (not reading)]

Comment on attachment 64687 [details] [diff] [review]
patch -- overide focus() method to use html:input focus() method

r=bryner, but long-term we should re-examine the way that we want focus and
activation to work.

Comment 28

[David Hyatt]

This is tricky, IMO too tricky to consider for 0.9.8.  There will need to be a 
lot of testing of this patch before it should go in to make sure there aren't 
regressions from it.  (Textfield focus in XUL is very tricky, i.e., very hacky.)

Comment 29

[basic]

I thought that the tricky parts were handled by <html:input>, oh well guess I'll
have to suffer with chatzilla stealing focus every time I start/restart moz

Comment 30

[Brian Ryner (not reading)]

This is an edge case, moving out...

(claudius or sairuh, would you have time to help with regression testing on this
patch?)

Comment 31

[Jesse Ruderman]

I don't think it's an edge case to leave the search sidebar panel open.

Comment 32

[Brian Ryner (not reading)]

I think this will have been fixed by my checkin for bug 76621.  Can someone
please test on a current trunk or branch build and close this if it is fixed?

Comment 33

[Brian Ryner (not reading)]

yes, this is now fixed.