Bug 46949 – Event names should always match the registered handler

• Component: DOM
• Status: RESOLVED DUPLICATE

[Enrica Casucci]

When firing DOM Level 3 focusin/focusout events, we also fire DOM Level 2 DOMFocusIn/DOMFocusOut, but we fail to reflect it in the event object.
The breaks any client who checks event.type.

[Enrica Casucci]

Created attachment 69407 [details]
Patch

[Adele Peterson]

Comment on attachment 69407 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69407&action=review

> WebCore/dom/EventTarget.cpp:310
> +            event->useAliasedType();

Would it make sense to have event->type() have this logic?  Are there any other places where using the regular type for an event with an aliased type would cause problems?

[Ryosuke Niwa]

Comment on attachment 69407 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69407&action=review

> WebCore/dom/EventTarget.cpp:310
> +        if (result != d->eventListenerMap.end()) {
> +            // When firing the event listener for the aliased event, the event type should
> +            // match the registered handler.
> +            event->useAliasedType();

It seems like alias is only used for focusin / focusout right now but are we sure we want to use the aliased type whenever an event is aliased?  i.e. if we are adding another aliased event in the future, do we also want to use the aliased type for that?

> LayoutTests/fast/events/focusinout.html:15
> -    writePass('result1');
> +    if (event.type == "focusin")
> +        writePass('result1');

Would it be possible to rename result1...result4 to something more meaningful?  It'll be also nice if we could print out what failed.

[Enrica Casucci]

(In reply to comment #2)
> (From update of attachment 69407 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69407&action=review
> 
> > WebCore/dom/EventTarget.cpp:310
> > +            event->useAliasedType();
> 
> Would it make sense to have event->type() have this logic?  Are there any other places where using the regular type for an event with an aliased type would cause problems?

I don't believe it is possible. Where we call useAliasedType is the only place we know we are firing an aliased type.

Comment on attachment 69407 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=69407&action=review

>>> WebCore/dom/EventTarget.cpp:310
>>> +            event->useAliasedType();
>> 
>> Would it make sense to have event->type() have this logic?  Are there any other places where using the regular type for an event with an aliased type would cause problems?
> 
> It seems like alias is only used for focusin / focusout right now but are we sure we want to use the aliased type whenever an event is aliased?  i.e. if we are adding another aliased event in the future, do we also want to use the aliased type for that?

I think it is wrong for the first target to change the event’s reported type for all subsequent targets that might handle the same event.

[Enrica Casucci]

(In reply to comment #5)
> (From update of attachment 69407 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=69407&action=review
> 
> >>> WebCore/dom/EventTarget.cpp:310
> >>> +            event->useAliasedType();
> >> 
> >> Would it make sense to have event->type() have this logic?  Are there any other places where using the regular type for an event with an aliased type would cause problems?
> > 
> > It seems like alias is only used for focusin / focusout right now but are we sure we want to use the aliased type whenever an event is aliased?  i.e. if we are adding another aliased event in the future, do we also want to use the aliased type for that?
> 
> I think it is wrong for the first target to change the event’s reported type for all subsequent targets that might handle the same event.

I'm not sure I understand your concern. We are always firing the Level 3 event with its name for all the targets, then we change to the name to the aliased and we fire the aliased handlers with the aliased name.
I apologize if I'm missing the point.

[Enrica Casucci]

(In reply to comment #6)
> (In reply to comment #5)
> > (From update of attachment 69407 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=69407&action=review
> > 
> > >>> WebCore/dom/EventTarget.cpp:310
> > >>> +            event->useAliasedType();
> > >> 
> > >> Would it make sense to have event->type() have this logic?  Are there any other places where using the regular type for an event with an aliased type would cause problems?
> > > 
> > > It seems like alias is only used for focusin / focusout right now but are we sure we want to use the aliased type whenever an event is aliased?  i.e. if we are adding another aliased event in the future, do we also want to use the aliased type for that?
> > 
> > I think it is wrong for the first target to change the event’s reported type for all subsequent targets that might handle the same event.
> 
> I'm not sure I understand your concern. We are always firing the Level 3 event with its name for all the targets, then we change to the name to the aliased and we fire the aliased handlers with the aliased name.
> I apologize if I'm missing the point.

The concept of aliasedType was introduced to continue to support old style events even when using the DOM Level 3 ones. I believe it is correct for the event type to match the name of the registered handler.
If a client registers a generic handler function and relies on the event.type  to distinguish which event was received, it will fail if we don't have handler and event type in sync.

[Ryosuke Niwa]

(In reply to comment #7)
> (In reply to comment #6)
> > > I think it is wrong for the first target to change the event’s reported type for all subsequent targets that might handle the same event.
> > 
> > I'm not sure I understand your concern. We are always firing the Level 3 event with its name for all the targets, then we change to the name to the aliased and we fire the aliased handlers with the aliased name.
> > I apologize if I'm missing the point.
> 
> The concept of aliasedType was introduced to continue to support old style events even when using the DOM Level 3 ones. I believe it is correct for the event type to match the name of the registered handler.
> If a client registers a generic handler function and relies on the event.type  to distinguish which event was received, it will fail if we don't have handler and event type in sync.

I think Dan is more concerned about the timing in which we change the event type.  Maybe we can do this where we call the first fireEventListeners?

[Darin Adler]

Longer term we definitely need to eliminate this concept of aliased types. I think it was not the right solution for the problem.

[Darin Adler]

There was another bug where we were trying to fix this same issue, and I had made a lot of comments there.

[Darin Adler]

*** This bug has been marked as a duplicate of bug 42580 ***