Closed Bug 303620 Opened 15 years ago Closed 15 years ago

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

Categories

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

x86
All
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: Biesinger, Assigned: aaronlev)

References

()

Details

(Keywords: crash, testcase, Whiteboard: [ETA: as soon as trunk build testing shows this is satisfactory])

Attachments

(6 files, 3 obsolete files)

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.
I'm pretty sure this is being caused by the JS included in this page to disable
the <input>.
Attached file even tinier test case
The spontaneous find as you type behavuor of the testcase is dealt with in bug
258285.
Attachment #192375 - Flags: superreview?(bryner)
Attachment #192375 - Flags: review?(mats.palmgren)
Attachment #192375 - Flags: superreview?(bryner) → superreview+
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...
Attachment #192375 - Flags: review?(mats.palmgren) → review-
Attached file Testcase #4
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?
Severity: normal → critical
Flags: blocking1.8b4?
Keywords: crash, testcase
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?
Blocks: 300147
Flags: blocking1.8b4? → blocking1.8b4+
Whiteboard: [ETA: 8/25 -- Does not really have to block anymore, now that find as you type issue is cleared]
(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.

Attachment #192375 - Attachment is obsolete: true
Attachment #193459 - Flags: superreview?(bryner)
Attachment #193459 - Flags: review?(mats.palmgren)
Whiteboard: [ETA: 8/25 -- Does not really have to block anymore, now that find as you type issue is cleared] → [ETA: as soon as I get r+ and sr+ ]
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...
Attachment #193459 - Flags: superreview?(bryner)
Attachment #193459 - Flags: review?(mats.palmgren)
Attachment #193459 - Flags: review-
Attached file Testcase #5
(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().

Attached patch Address Mats' comments (obsolete) — Splinter Review
Attachment #193459 - Attachment is obsolete: true
Attachment #193572 - Flags: review?(mats.palmgren)
Attachment #193572 - Attachment is obsolete: true
Attachment #193572 - Flags: review?(mats.palmgren)
Attachment #193586 - Flags: review?(mats.palmgren)
Attachment #193586 - Attachment description: Correct patch → Correct patch (applies to trunk only)
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.
Attachment #193586 - Flags: superreview+
Comment on attachment 193586 [details] [diff] [review]
Correct patch (applies to trunk only)

Looks good. r=mats
Attachment #193586 - Flags: review?(mats.palmgren) → review+
Attachment #193586 - Flags: approval1.8b4?
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.
Attachment #193586 - Flags: approval1.8b4?
Fixed on trunk.
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Whiteboard: [ETA: as soon as I get r+ and sr+ ] → [ETA: as soon as trunk build testing shows this is satisfactory]
Maybe bug 305840 is regression of this.
Depends on: 305913
Depends on: 305840
Depends on: 305833
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.
We need to ensure that 305840 goes in on the branch if this one goes.
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.
Flags: blocking1.8b5+ → blocking1.8b5?
Flags: blocking1.8b5? → blocking1.8b5-
Flags: blocking-aviary2.0?
*** Bug 300147 has been marked as a duplicate of this bug. ***
Depends on: 505355
Component: Keyboard: Navigation → User events and focus handling
You need to log in before you can comment on or make changes to this bug.