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

Status: NEW

(Core :: Layout: Form Controls, defect)

Description

[Mats Palmgren (inactive)]

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?

Comment 1

[Aaron Leventhal]

Would fixing this remove the need for esm::EnsureFocusSynchronization() ? The
blur handler itself could change focus, right?

Comment 2

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Did smaug's recent changes affect this?

Comment 3

[Olli Pettay [:smaug][[e-mail]]]

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.

Comment 4

[Mike Schroepfer]

Mats/Smaug - is this bug still relevant?

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Removing blocking status until we get feedback.

Comment 6

[BugBot [:suhaib / :marco/ :calixte]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.