36573 - [DOM] onchange -> event.target is wrong element

Status: VERIFIED FIXED

(Core :: DOM: Events, defect)

Description

[Joe Hewitt]

When an input element's onchange event fires, the target of the event should be 
a reference to the input element itself.  However, instead the target is the 
input's FORM element, or if the input element has no siblings, the HTML element.

Comment 1

[Joe Hewitt]

Attachment: simple test case

Comment 2

[Asa Dotzler [:asa]]

confirming

Comment 3

[joki (gone)]

This is valid though the bug actually seems to be that the target of the current 
mouse event is being taken for the onchange event target.  I can see how that 
might happend.  I'll keep looking at it.

Comment 4

[joki (gone)]

Adding [DOM] prefix to bugs related to DOM Level 0, 1, or 2 
compatibility/compliance.

Comment 5

[ckritzer (gone)]

Per heikki, saari, ckritzer:
This bug has been marked "future" because the original netscape engineer working 
on this is over-burdened. If you feel this is an error, that you or another
known resource will be working on this bug,or if it blocks your work in some way 
-- please attach your concern to the bug for reconsideration.

Comment 6

[Steve Halasz]

Dragging and dropping a file onto a text input produces and event with an 
undefined target. The following INPUT field types do not work:
Text
File (Dropping on the 'Browse' button *does* work)
TEXTAREA

The following do work:
Submit
Checkbox
SELECT

Comment 7

[Fabian Guisset]

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

Comment 8

[Guy Alfandary]

Please change the target to 0.9 I tried doing it myself as per:
http://www.mozilla.org/roadmap.html "how you can help"/"Mozilla embedders"
but, apparently I'm not "sufficiently empowered" :-(

Comment 9

[Guy Alfandary]

After much digging I found the problem and a solution.
The problem is as follows:
nsEventStateManager has a target mCurrentTarget which is set
by the mouse press event the problem is that for change event
this target is incorrect e.g.: the user enters text in the input
and than presses outside of the text input - the resulting target
is whatever the user pressed on.
The solution:
Set mCurrentTarget before dispatching the event, and reset it afterwards.
The code:
In nsEventStateManager::PreHandleEvent at the top add:
///////////////////////////
  if (!aPresContext && !aEvent && !aStatus && !aView && aTargetFrame)
    {
      mCurrentTarget = aTargetFrame;
      return NS_OK;
    }
////////////////////////////
In our case the only preprocessing needed is to set mCurrentTarget

In nsGfxTextControlFrame2::CallOnChange
add #include "nsIEventStateManager.h"
replace ... mContent->HandleDOMEvent ...
with:
/////////////////////////////
      {
        nsIFrame* oldTarget;
        nsCOMPtr<nsIEventStateManager> manager;
        context->GetEventStateManager(getter_AddRefs(manager));
        if (manager)
          {
            manager->GetEventTarget(&oldTarget);
            manager->PreHandleEvent(nsnull, nsnull, this, nsnull, nsnull);
          }
        nsresult result = mContent->HandleDOMEvent(context, &event, nsnull,
                                                   NS_EVENT_FLAG_INIT, &status);
        if (manager)
          manager->PreHandleEvent(nsnull, nsnull, oldTarget, nsnull, nsnull);
        return result;
      }
/////////////////////////////

Can someone please check this fix and submit it if OK ?

Comment 10

[Fabian Guisset]

joki can you please take a look at the suggestion offered by Guy?

Comment 11

[Vladimir Ermakov]

QA Contact Update

Comment 12

[Asa Dotzler [:asa]]

jst, can you take a look at this patch please? thanks.

Comment 13

[Johnny Stenback (:jst)]

This is really joki's code, I'd prefere if he'd have a look once he has a
chance. Back to joki.

Comment 14

[John Straw]

Attachment: G. Alfandary's fix as patch

Comment 15

[saari (gone)]

->0.9.6 for Tom to evaluate the patch

Comment 16

[Jonadab the Unsightly One (Nathan Eady)]

Too late for 0.9.6, this needs retargeting.

Comment 17

[joki (gone)]

0.9.6 has passed, moving to 0.9.7.  Load balancing 0.9.7 list shortly

Comment 18

[joki (gone)]

Moving bugs from 0.9.7 for triaging in 0.9.8

Comment 19

[Bob Clary [:bc] (inactive)]

Looking at the sequence of events as you tab out of the testcase's input field
to the select box gives:

================= begin event =================
Event called on obj [object HTMLInputElement]
[element[INPUT]]
evt[type]=change
evt[target]=[object HTMLSelectElement][element[SELECT]]
evt[currentTarget]=[object HTMLInputElement][element[INPUT]]
evt[eventPhase]=2
evt[bubbles]=true
evt[cancelable]=true
evt[view]=[object Window]
evt[originalTarget]=[object HTMLSelectElement][element[SELECT]]
evt[button]=65535
evt[rangeParent]=[object HTMLFormElement][element[FORM]]
evt[rangeOffset]=9
================= end event ===================

Which agrees with the earlier analysis. It seems to me that AT_TARGET, target ==
currentTarget should be an invariant.  What is the meaning of originalTarget?
Does this value of originalTarget==SELECT when tabbing from the INPUT to SELECT
make sense?

Getting event targets wrong is a bad thing.

Comment 20

[Heikki Toivonen (remove -bugzilla when emailing directly)]

Major corporations depend on eapp bugs, and need them to be fixed before they
can recommend Mozilla-based products to their customers. Adding nsbeta1+ keyword
and making sure the bugs get re-evaluated if they are targeted beyond 1.0.

Comment 21

[Bob Clary [:bc] (inactive)]

eapp was incorrectly used to change this to nsbeta1+. Resetting to nsbeta1 to
nominate. This is an important issue and deserves to be nsbeta1+ by the ADT.

Comment 22

[Bob Clary [:bc] (inactive)]

Note that after typing into the input field in the test case and either tab or
the mouse is used to transfer focus to the select box, the blur event fires on
the imput element but does not bubble before the focus event fires on the select
box.

Comment 23

[joki (gone)]

The patch is definitely the right idea but there's actually a mechanism in
nsPresShell that achieves it more cleanly.  Posting new patch.

Comment 24

[joki (gone)]

Attachment: Patch for bug

Comment 25

[saari (gone)]

Comment on attachment 70056 [details] [diff] [review]
Patch for bug

r=saari

Comment 26

[Johnny Stenback (:jst)]

Comment on attachment 70056 [details] [diff] [review]
Patch for bug

sr=jst

Comment 27

[joki (gone)]

Fix checked in.

Comment 28

[Vladimir Ermakov]

verifying on build 2002-02-22-03-trunk windows 98

Comment 29

[Todd A. Fisher]

If this is working can someone update
http://www.mozilla.org/docs/dom/mozilla/bug-events.html

Comment 30

[Gérard Talbot]

http://www.mozilla.org/docs/dom/mozilla/bug-events.html
 
has been updated accordingly.