Open Bug 304751 Opened 15 years ago Updated 12 years ago

Calling FireOnChange() inside nsComboboxControlFrame::SetFocus() is bad

Categories

(Core :: Layout: Form Controls, defect)

x86
All
defect
Not set

Tracking

()

People

(Reporter: mats, Assigned: mats)

Details

(Whiteboard: [dbaron-1.9:RsCe])

From bug 258285 comment 213:

As far as I can see this part of the patch is only necessary to fix the
testcase "Tiny testcase.." (attachment 192321 [details]). The reason is that
when we first blur the SELECT we end up here:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/content/src/nsHTMLSelectElement.cpp&rev=1.232&root=/cvsroot&mark=1759-1772#1731

and on the "forced" frame blur on line 1767 we end up here:
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/forms/nsComboboxControlFrame.cpp&rev=1.324&root=/cvsroot&mark=509,519#508

and then the fun begins since we have onchange="something_else.focus()" which
will move the focus and then "on the way back" we end up blurring what was
just focused (in nsHTMLSelectElement::HandleDOMEvent line 1770).

The placement of "FireOnChange()" inside "SetFocus()" is bad for other
reasons as well and I was planning on moving it. Hopefully this solves the
combobox blur problem too.

The rest of the patch looks good, except maybe for the removal of the
null-check of |mDocument| in one place - why do we not need it there?
Would fixing this remove the need for esm::EnsureFocusSynchronization() ? The
blur handler itself could change focus, right?
Flags: blocking1.9a1?
Did smaug's recent changes affect this?
Flags: blocking1.9a1? → blocking1.9+
My fix prevented the crash if display type was changed during 'change' event.
Bug 234455 may have changed things too.

I'm not quite sure what this bug is about.
Mats/Smaug - is this bug still relevant?
Whiteboard: [dbaron-1.9:RsCe]
Removing blocking status until we get feedback.
Flags: blocking1.9+
You need to log in before you can comment on or make changes to this bug.