12849 - [blocker]GFX Select: event.target is an option, not the <select> element

Status: VERIFIED FIXED

(Core :: XUL, defect, P3)

Description

[Alec Flett]

Got another one for ya rod....
with the new GFX combo boxes that went in a few days ago, the event.target of a
select is now the option child rather than the select widget itself. When I
switch back to native widgets, the event target is back to being the the select
element.

This is majorly confusing the search dialog (again) because it expects
event.target to be the select widget. I couldn't find a spec for this, but I
would imagine that since the onselect= event handler is on the select and not
the option, that the select should be the event target.

Comment 1

[Alec Flett]

here's a good test:
<html:select onclick="selectClicked(event);">
<html:option>opt1</html:option>
<html:option>opt2</html:option>
<html:option>opt3</html:option>
</html:select>

and then the JS code looks like
function selectClicked(event) {
	dump("The target is " + event.target + "\n");
}

With GFX widgets, you'll see:
The target is [HTMLOptionElement]
with native widgets, you'll see:
The target is [HTMLSelectElement]

Comment 2

[rods (gone)]

Attachment: Demonstrates the wrong behavior

Comment 3

[rods (gone)]

This is a blocker for turning on GFX selects.
Tom, I think this would be your bug. I can't see at the moment what I
would/could do differently in the select code to change the target of the event
when the option gets selected so the target is the select instead of the
option for the :onselect: JS event. I am adding Eric P. and Kevin to the cc
list.

Comment 4

[rods (gone)]

Forgot to add myself to the cc list.

Comment 5

[joki (gone)]

Actually I'd say its yours for the same reasons.  The basic issue, though, is
that we now have multiple frames where we used to have just one.  So we used to
hit the SELECT frame and go no deeper.  Now we dig all the way down to the text
frames.  The gfx SELECT implementation is going to have to hide the existence of
those frames if it doesn't want the DOM to expose them via events.  I'll help
with this but its not mine.

As far as proper behavior, yes there is no spec for this per se.  Nav4 couldn't
go down this deep, IE does what we did with native widgets which is just give
back the SELECT.  I'd vote for just the SELECT since usually the reason you'd
want to know about the particular OPTION that was hit and pretty much that info
can be gotten after the onchange event.

Comment 6

[Alec Flett]

yes, I would say we need to be at least consistent with nav4...

Comment 7

[Alec Flett]

just discovered Nav4 doesn't have onclick/onselect (at least on linux)
so I guess consistency with IE is best..

Comment 8

[rods (gone)]

Tom and I talked about this and we think that it should return the option
element instead of the select element. The select can be easily gotten from the
event.target which is the parent of the returned option element. Also, it might
be helpful to get the option back, after all that was the DOM element that was
selected. I have a fix to check in.

Comment 9

[Alec Flett]

I really have to STRONGLY disagree on this one, for two major reasons:
1) the onclick= handler is put on the <select> element. why would event.target
return the option element?
2) most importantly - we are just making web developer's lives difficult if we
are intentionally different from IE when we could easily be the same. You may
not realize how difficult it already is to write cross-browser web applications,
but there's no reason to add one more gotcha.

But you said you had a fix....is the fix to return the select element or the
option element?

Comment 10

[Alec Flett]

As it turns out, onchange= is the HTML4 sanctioned event that we should be
talking about.

Nav4 has onchange= and it returns the select element, as it has since Nav2. we'd
even be inconsistent with Nav2-4 if we changed this.

Comment 11

[joki (gone)]

I initially was thinking about IE compatibility but I've changed my mind.  Our
goal is to make web developers lives easier by giving them standards support,
not by cloning IE.  Although the spec doesn't say anything about this specific
case, in general when you click on an element in a container you get the
lowest element, which in this case is the option.

Trust me, IE5 has such bad DOM support in general that we're not going to be
anywhere near compatible with them anyway.

Comment 12

[joki (gone)]

The onselect event is a separate issue.  The test case here talks about click
events which didn't work on these elements in Nav4.  This code would not change
the behavior of the onchange event.  The targetting of the onchange event isn't
mouse driven and happens through an entirely different mechanism.

Comment 13

[Alec Flett]

oh, ok, then this bug should be for the onchange= handler, because that is the
one that is busted.
 I have no idea how the onclick handler works, but choosing the option makes
sense. Sorry I wasn't explicit in my original bug report (whoops!)
Should I file a seperate bug or clarify the summary of this bug?

Comment 14

[Chris McAfee]

adding to 12902, gfx widgets tracker

Comment 15

[rods (gone)]

Ok, so forget about onselect.
Summary:
onchange for selects should have the select as the target
onclick for selects is going to have the option as the target
onclick for the option would (of coarse) of the option as the target

If the above is true, and the onchange happens outside of event processing then
this is an Eric Pollmann bug?

Comment 16

[Alec Flett]

that all sounds like the correct behavior to me!
Now the question is, what ACTUALLY happens in these three cases?

Comment 17

[rods (gone)]

onchange for selects return option as the target this WRONG
onclick for selects returns the option as the target CORRECT
onclick for the option returns the option as the target CORRECT

Reassigning to Eric

Comment 18

[rods (gone)]

Attachment: Better test case

Comment 19

[Eric Pollmann]

Checked in a fix.

Comment 20

[phillip]

verified fixed on

     1999-09-13-08-M10 RedHat Linux 6.0 (GNOME/enlightenment)
     1999-09-13-08-M10 WinNT 4.0 sp5