235522 - Here, Focus is coming at more than one places at a time

Status: RESOLVED FIXED

(SeaMonkey :: General, defect)

Description

[Neelesh K Sharma]

User-Agent:       
Build Identifier: 

In this bug, Focus is not coming at appropriate place.

Reproducible: Always
Steps to Reproduce:
1. Load the page win1.htm into the browser, ensure that the browser does not 
occupy the whole screen.             
2.  Click in the textfield, click outside the browser window, then click onto 
the combo box. Both will now appear to have focus.
3.Now press the tab key until the window has focus, then click outside browser 
window, click on the textarea, click outside the browser window and then click 
on the combo box. The window, textarea and combo box will all now appear to 
have focus. 
4. Reload the page win1.htm into the browser 
5. clicking on the combo box and then clicking on the textarea, it will take 
two mouse clicks to actually set the focus in the text area

Comment 1

[Neelesh K Sharma]

I have checked this problem in open source mozilla 1.1, 1.2, 1.3 and  1.4 on 
os2. Problem is occuring everywhere. It is occuring partially in mozilla 1.5 
and 1.6 on os2 and windows.

 Regards, Neelesh
 Mozilla Support, IBM India

Comment 2

[Neelesh K Sharma]

Attachment: Testcase

Comment 3

[Boris Zbarsky [:bzbarsky]]

mkaply, can you reproduce this?  I can't seem to on Linux, but the focus model
is very different....

Comment 4

[Mike Kaply [:mkaply]]

I can't reproduce this. When I click on the combobox, it pops down (on Windows
and OS/2)

It sounds like the version you are using is missing my final fix in
nsFrameWindow.cpp.

http://lxr.mozilla.org/seamonkey/source/widget/src/os2/nsFrameWindow.cpp#435

Comment 5

[aaronr]

ok, this is what I see:

-- As late as mozilla 1.4 if you have the cursor flashing in the textfield,
click outside the frame, then click on the combobox, the cursor will still flash
on the textfield, yet the combobox will still be hashed.
-- Starting on mozilla 1.5, this behavior doesn't occur anymore.
-- If you tab around the page win1.htm until the client area of the fame has a
hashed border, no amount of clicking on the widgets inside the page or switching
focus to the desktop and then back to a widget in the page will cause the client
area to lose its hashed border.  This is recreatable on RedHat 9, Windows 2000
and OS/2.
-- Mike Kaply and I have confirmed that the fix to nsFrameWindow.cpp that he
mentioned is indeed in Mozilla 1.4 for OS/2.

So I think that this bug should now be centered around getting the proper
behavior across all platforms for the hashing and unhashing of the client area.

Comment 6

[Brian Ryner (not reading)]

Attachment: patch

Here's one way we could fix this.  Right now, the tabbing code takes care of
removing the canvas focus ring when you tab from the document into the content.
 This wasn't getting hit when you click instead of tab.  This patch pulls the
check into SetContentState, which is hit for all focus transitions.

This probably needs a little more testing to cover all of the cases where the
canvas focus ring should be removed.  That includes these basic tests:

- using f6 or ctrl+tab to switch between frameset frames
- tabbing into/out of iframes
- using ctrl+L to focus the URL bar (document focus ring should go away)

and for more fun:

- script focusing a control on a timer, with no keyboard or mouse event
involved
- focusing a control which doesn't accept focus (document focus ring should
remain)
- focusing a control which focuses something else in its focus handler

Also, just so that it's not counted as a bug... the fact that the document
focus ring does not appear when you _click_ in the page is intentional.  We
felt that since the focus ring is mainly wanted for keyboard
navigation/accessibility, there's no point in showing it for click-focus and
uglifying sites where a frameset with no borders is used to achieve the layout.

Comment 7

[Brian Ryner (not reading)]

Attachment: patch with null check

The previous patch crashed when displaying the profile manager.  This fixes
that by null-checking mDocument.

I tested all of the scenarios I mentioned before and found no problems.  I
tested the frameset on http://msdn.microsoft.com/library/ and the iframe on
http://tinderbox.mozilla.org/showbuilds.cgi?tree=SeaMonkey.  I'll attach a
testcase I used for the trickier tests.

Comment 8

[Brian Ryner (not reading)]

Attachment: additional testcases

Comment 9

[Darin Fisher]

Comment on attachment 142872 [details] [diff] [review]
patch with null check

>+        nsCOMPtr<nsIDocShell> docShell =
>+          do_QueryInterface(nsCOMPtr<nsISupports>(mDocument->GetContainer()));

it's too bad there isn't a way to avoid QI'ing twice here (once for the
QueryReferent and once for the explicit QI).  QueryReferent does take
an IID, so maybe we should write a version of GetContainer that takes
an IID (or uses template magic)?

that's just a quick comment... i haven't really reviewed the patch yet ;)

Comment 10

[Dan M]

Comment on attachment 142872 [details] [diff] [review]
patch with null check

>+ nsCOMPtr<nsIDocShell> docShell =
>+   do_QueryInterface(nsCOMPtr<nsISupports>(mDocument->GetContainer()));
>+

Mmmmmmm, refelicious. aghhhhhhhghghghgghhh

Code looks fine and I noticed no regressions when I took it out for a spin. But
there was this funky bit:

>+      nsCOMPtr<nsIContent> oldFocus = mCurrentFocus;
>+

oldFocus isn't referenced again before it goes out of scope. Is it a
kungFuDeathGrip? It doesn't seem to be. I say ditch it or rename it.

PS ditto comment 4: I can't reproduce the original testcase on my WinXP box,
patched or not. (The second testcase is problematic without the patch.)

Comment 11

[Brian Ryner (not reading)]

(In reply to comment #10)
> there was this funky bit:
> 
> >+      nsCOMPtr<nsIContent> oldFocus = mCurrentFocus;
> >+
> 
> oldFocus isn't referenced again before it goes out of scope. Is it a
> kungFuDeathGrip? It doesn't seem to be. I say ditch it or rename it.

Yep, I changed my mind about how to do this and forgot to remove that part. 
Will do that before checking in.

> PS ditto comment 4: I can't reproduce the original testcase on my WinXP box,
> patched or not. (The second testcase is problematic without the patch.)
> 

Same.

Comment 12

[Darin Fisher]

Comment on attachment 142872 [details] [diff] [review]
patch with null check

sr=darin

Comment 13

[Brian Ryner (not reading)]

checked in.

Comment 14

[Neelesh K Sharma]

I have ported this patch over IWB2.03la, which is nothing but mozilla 1.6, on 
os2. Problem related to 5th step in recreation scenario can be still seen. I 
found the same result with mozilla 1.7b on windows XP.