303620 - Focus rectangle drawn at <select> when focus is actually on <input>

Status: RESOLVED FIXED

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

Description

[Christian :Biesinger (don't email me, ping me on IRC)]

On the Create Attachment page, if I select "?" from the dropdown list with the
keyboard and press tab, the focus is in the <input> (that's very good, and
didn't always use to be that way :-) )

But, the <select> below for the next flag gets a focus rectangle. That seems
incorrect since it doesn't actually have focus.

winxp, seamonkey, 2005070205; but also seen on linux for a long time.

Comment 1

[Adam Guthrie]

Attachment: minimized testcase with the JS included

Comment 2

[Adam Guthrie]

Attachment: minimized testcase without the JS included

I'm pretty sure this is being caused by the JS included in this page to disable
the <input>.

Comment 3

[Aaron Leventhal]

Attachment: even tinier test case

Comment 4

[Aaron Leventhal]

Attachment: Very sensible change -- same exact thing for button, select, input and textarea. Don't assume that focus stays what you set it to after a SetContentState(NS_EVENT_STATE_FOCUS)

The spontaneous find as you type behavuor of the testcase is dealt with in bug
258285.

Comment 5

[Mats Palmgren (inactive)]

Comment on attachment 192375 [details] [diff] [review]
Very sensible change -- same exact thing for button, select, input and textarea. Don't assume that focus stays what you set it to after a SetContentState(NS_EVENT_STATE_FOCUS)

Looks good but there are a couple more places...

Comment 6

[Mats Palmgren (inactive)]

Attachment: Testcase #4

Comment 7

[Mats Palmgren (inactive)]

The crash occurs here:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/base/src/nsGenericElement.cpp&root=/cvsroot&mark=2552-2558#2545

After the SetContentState() call the frame is gone:

Program received signal SIGSEGV, Segmentation fault.
[Switching to Thread 16384 (LWP 7472)]
0x40b36d79 in PresShell::ScrollFrameIntoView(nsIFrame*, int, int) const
(this=0x8697b50, aFrame=0x86c3c38, aVPercent=-2, aHPercent=-2)
    at nsPresShell.cpp:4523
4523        nsIDocument* document = content->GetDocument();
(gdb) bt 6
#0  0x40b36d79 in PresShell::ScrollFrameIntoView(nsIFrame*, int, int) const
(this=0x8697b50, aFrame=0x86c3c38, aVPercent=-2, aHPercent=-2)
    at nsPresShell.cpp:4523
#1  0x40d3b340 in nsGenericElement::SetFocus(nsPresContext*) (this=0x86bea60,
aPresContext=0xbfffd280) at nsGenericElement.cpp:2559
#2  0x40d76536 in nsEventStateManager::ChangeFocusWith(nsIContent*,
nsIEventStateManager::EFocusedWithType) (this=0x832e058, aFocusContent=0x86bea60,
    aFocusedWith=eEventFocusedByKey) at nsEventStateManager.cpp:3084
#3  0x40d76fc0 in nsEventStateManager::ShiftFocusInternal(int, nsIContent*)
(this=0x832e058, aForward=1, aStart=0x0) at nsEventStateManager.cpp:3348
#4  0x40d76836 in nsEventStateManager::ShiftFocus(int, nsIContent*)
(this=0x832e058, aForward=1, aStart=0x0) at nsEventStateManager.cpp:3185
#5  0x40d748c9 in nsEventStateManager::PostHandleEvent(nsPresContext*, nsEvent*,
nsIFrame*, nsEventStatus*, nsIView*) (this=0x832e058,
    aPresContext=0x8660fd0, aEvent=0xbfffde60, aTargetFrame=0x86bc950,
aStatus=0xbfffdad8, aView=0xbfffde60) at nsEventStateManager.cpp:2163
(More stack frames follow...)
(gdb) p *aFrame
$1 = {<nsISupports> = {_vptr.nsISupports = 0x0}, mRect = {x = -572662307, y =
-572662307, width = -572662307, height = -572662307},
  mContent = 0xdddddddd, mStyleContext = 0xdddddddd, mParent = 0xdddddddd,
mNextSibling = 0xdddddddd, mState = 3722304989}

You need add the same check that we are still focused and also to get the
primary frame again. Maybe we also need to keep a strong ref on presShell?

Comment 8

[Mats Palmgren (inactive)]

The other spot I found was this:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/dom/src/base/nsGlobalWindow.cpp&root=/cvsroot&mark=6682-6684,6690#6667

It's less important, but we probably should check that we are still focused
before assigning "didFocusContent = PR_TRUE" - so we would do the
"focusedWindow->Focus();" instead, I think that is desirable, no?

Comment 9

[Aaron Leventhal]

(In reply to comment #8)
> The other spot I found was this:
>
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/dom/src/base/nsGlobalWindow.cpp&root=/cvsroot&mark=6682-6684,6690#6667
> 
> It's less important, but we probably should check that we are still focused
> before assigning "didFocusContent = PR_TRUE" - so we would do the
> "focusedWindow->Focus();" instead, I think that is desirable, no?

I don't think so, I think the point there is that SetContentState() will handle
geting the window focused for whatever content is focused, via SendFocusBlur(),
so that that focusing the window is only necessary if there was no child content
at all getting focused.

I'm not sure why Brian didn't just let SetContentState(nsnull,
NS_EVENT_STATE_FOCUS) do the window focusing, and just have one case there.

Comment 10

[Aaron Leventhal]

Attachment: Handle last few cases, do it more conveniently via return value from SetContentState()

Comment 11

[Mats Palmgren (inactive)]

Comment on attachment 193459 [details] [diff] [review]
Handle last few cases, do it more conveniently via return value from SetContentState()

Index: content/events/public/nsIEventStateManager.h
+   * @return  Whether the content was able to change all states. Returns
PR_FALSE
+   *		       if a resulting DOM event causes a different content
+   *		       node to change states, instead of the content node
passed in.
+   */
+  virtual PRBool SetContentState(nsIContent *aContent, PRInt32 aState) = 0;


The comment is unclear on the return value when we fail in such a way
that we end up with 'mCurrentFocus' being NULL (I assume it's PR_FALSE also?)

Regarding the change of return type - do we need to bump the IID for
the interface? (I guess 'nsresult' and 'PRBool' are most likely compatible
but you never know...).


Index: content/base/src/nsGenericElement.cpp
-  if (frame && frame->IsFocusable()) {
+  if (frame && frame->IsFocusable() &&
     aPresContext->EventStateManager()->SetContentState(this,
-							 NS_EVENT_STATE_FOCUS);
+							 NS_EVENT_STATE_FOCUS))
{
     presShell->ScrollFrameIntoView(frame, NS_PRESSHELL_SCROLL_IF_NOT_VISIBLE,
				    NS_PRESSHELL_SCROLL_IF_NOT_VISIBLE);


You can't trust that 'frame' is alive after SetContentState,
you need to call 'GetPrimaryFrameFor' again.
Try the upcoming testcase which crashes with the current patch...

Comment 12

[Mats Palmgren (inactive)]

Attachment: Testcase #5

Comment 13

[Aaron Leventhal]

(In reply to comment #12)
> Created an attachment (id=193492) [edit]
> Testcase #5

Cool testcase.

I wonder why we really need separate impls for nsHTMLAnchorElement::SetFocus()
and nsHTMLAreaElement::SetFocus() apart from nsGenericHTMLElement::SetFocus(). I
see that they FlushPendingNotifications(Flush_Layout) which seems like it should
be done for everything or not at all. The anchor impl has checks for a link
handler but the generic SetFocus() already does a better IsFocusable().

Comment 14

[Aaron Leventhal]

Attachment: Address Mats' comments

Comment 15

[Aaron Leventhal]

Attachment: Correct patch (applies to trunk only)

Comment 16

[Brian Ryner (not reading)]

Comment on attachment 193586 [details] [diff] [review]
Correct patch (applies to trunk only)

>--- content/events/src/nsEventStateManager.h	16 Aug 2005 17:54:28 -0000	1.138
>+++ content/events/src/nsEventStateManager.h	23 Aug 2005 17:19:44 -0000
>@@ -113,11 +113,11 @@ public:
>   NS_IMETHOD GetEventTarget(nsIFrame **aFrame);
>   NS_IMETHOD GetEventTargetContent(nsEvent* aEvent, nsIContent** aContent);
>   NS_IMETHOD GetEventRelatedContent(nsIContent** aContent);
> 
>   NS_IMETHOD GetContentState(nsIContent *aContent, PRInt32& aState);
>-  NS_IMETHOD SetContentState(nsIContent *aContent, PRInt32 aState);
>+  PRBool SetContentState(nsIContent *aContent, PRInt32 aState);

The compiler will make this virtual since it's virtual on the base class, but
please write out "virtual PRBool ...." here for clarity.

Looks ok otherwise.

Comment 17

[Mats Palmgren (inactive)]

Comment on attachment 193586 [details] [diff] [review]
Correct patch (applies to trunk only)

Looks good. r=mats

Comment 18

[Asa Dotzler [:asa]]

Comment on attachment 193586 [details] [diff] [review]
Correct patch (applies to trunk only)

please get this in on the trunk and request approval when it's proved itself.
Thanks.

Comment 19

[Aaron Leventhal]

Fixed on trunk.

Comment 20

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Maybe bug 305840 is regression of this.

Comment 21

[Asa Dotzler [:asa]]

We want this on the branch but we need to work out the regressions first. Please
request approval when all regressions have been cleared up. thanks.

Comment 22

[Mike Schroepfer]

We need to ensure that 305840 goes in on the branch if this one goes.

Comment 23

[Aaron Leventhal]

This was 1.8b4+, but I recommend we minus it because it's caused too many
problems. Too risky for branch at this point.

The most important part of the bug was fixed with bug 258285 -- where find as
you type was coming up in the wrong situations.

It's not a regression. Same behavior in firefox 1.0.

Comment 24

[Boris Zbarsky [:bzbarsky]]

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