From 2bc06fb4aea2e3e960064c3e5254a1fb6ed07a76 Mon Sep 17 00:00:00 2001 From: Markus Stange Date: Thu, 27 Jul 2023 01:43:21 +0000 Subject: [PATCH] Bug 1845381 - For begin/repeat/end SMIL animation events, only support "beginEvent"/"repeatEvent"/"endEvent" in addEventListener. r=dholbert,smaug Before this patch, we would have called listeners added with both addEventListener("end", ...) and addEventListener("endEvent", ...). Now we will just call listeners added with the latter and ignore the former. This change only affects listeners added with addEventListener. For attribute handlers and IDL property listeners, we still use onbegin/onrepeat/onend as before. The new behavior matches Blink, simplifies the code, and will let us improve performance by storing event listeners in a map keyed by event type atom (bug 1834370). Differential Revision: https://phabricator.services.mozilla.com/D184532 --- dom/base/nsContentUtils.cpp | 57 ++++---------- dom/base/nsContentUtils.h | 3 - dom/events/EventNameList.h | 33 ++++---- dom/svg/SVGAnimationElement.h | 27 +++++++ .../svg/smil/container/deferred-anim-1.xhtml | 2 +- .../svg/smil/container/deferred-tree-1.xhtml | 2 +- .../tests/svg/animations/custom-events.html | 75 +++++++++++++++++++ .../tests/svg/animations/event-listeners.html | 62 +++++++++++++++ 8 files changed, 201 insertions(+), 60 deletions(-) create mode 100644 testing/web-platform/tests/svg/animations/custom-events.html create mode 100644 testing/web-platform/tests/svg/animations/event-listeners.html diff --git a/dom/base/nsContentUtils.cpp b/dom/base/nsContentUtils.cpp index 8c7d6be0eba9..219e0fb7633a 100644 --- a/dom/base/nsContentUtils.cpp +++ b/dom/base/nsContentUtils.cpp @@ -976,20 +976,13 @@ static nsAtom* GetEventTypeFromMessage(EventMessage aEventMessage) { } } -// Because of SVG/SMIL we have several atoms mapped to the same -// id, but we can rely on MESSAGE_TO_EVENT to map id to only one atom. -static bool ShouldAddEventToStringEventTable(const EventNameMapping& aMapping) { - MOZ_ASSERT(aMapping.mAtom); - return GetEventTypeFromMessage(aMapping.mMessage) == aMapping.mAtom; -} - bool nsContentUtils::InitializeEventTable() { NS_ASSERTION(!sAtomEventTable, "EventTable already initialized!"); NS_ASSERTION(!sStringEventTable, "EventTable already initialized!"); static const EventNameMapping eventArray[] = { #define EVENT(name_, _message, _type, _class) \ - {nsGkAtoms::on##name_, _type, _message, _class, false}, + {nsGkAtoms::on##name_, _type, _message, _class}, #define WINDOW_ONLY_EVENT EVENT #define DOCUMENT_ONLY_EVENT EVENT #define NON_IDL_EVENT EVENT @@ -1010,11 +1003,9 @@ bool nsContentUtils::InitializeEventTable() { MOZ_ASSERT(!sAtomEventTable->Contains(eventArray[i].mAtom), "Double-defining event name; fix your EventNameList.h"); sAtomEventTable->InsertOrUpdate(eventArray[i].mAtom, eventArray[i]); - if (ShouldAddEventToStringEventTable(eventArray[i])) { - sStringEventTable->InsertOrUpdate( - Substring(nsDependentAtomString(eventArray[i].mAtom), 2), - eventArray[i]); - } + sStringEventTable->InsertOrUpdate( + Substring(nsDependentAtomString(eventArray[i].mAtom), 2), + eventArray[i]); } return true; @@ -4565,13 +4556,6 @@ nsAtom* nsContentUtils::GetEventMessageAndAtom( mapping.mMessage = eUnidentifiedEvent; mapping.mType = EventNameType_None; mapping.mEventClassID = eBasicEventClass; - // This is a slow hashtable call, but at least we cache the result for the - // following calls. Because GetEventMessageAndAtomForListener utilizes - // sStringEventTable, it needs to know in which cases sStringEventTable - // doesn't contain the information it needs so that it can use - // sAtomEventTable instead. - mapping.mMaybeSpecialSVGorSMILEvent = - GetEventMessage(atom) != eUnidentifiedEvent; sStringEventTable->InsertOrUpdate(aName, mapping); return mapping.mAtom; } @@ -4581,33 +4565,22 @@ EventMessage nsContentUtils::GetEventMessageAndAtomForListener( const nsAString& aName, nsAtom** aOnName) { MOZ_ASSERT(NS_IsMainThread(), "Our hashtables are not threadsafe"); - // Because of SVG/SMIL sStringEventTable contains a subset of the event names - // comparing to the sAtomEventTable. However, usually sStringEventTable - // contains the information we need, so in order to reduce hashtable - // lookups, start from it. + // Check sStringEventTable for a matching entry. This will only fail for + // user-defined event types. EventNameMapping mapping; - EventMessage msg = eUnidentifiedEvent; - RefPtr atom; if (sStringEventTable->Get(aName, &mapping)) { - if (mapping.mMaybeSpecialSVGorSMILEvent) { - // Try the atom version so that we should get the right message for - // SVG/SMIL. - atom = NS_AtomizeMainThread(u"on"_ns + aName); - msg = GetEventMessage(atom); - } else { - atom = mapping.mAtom; - msg = mapping.mMessage; - } + RefPtr atom = mapping.mAtom; atom.forget(aOnName); - return msg; + return mapping.mMessage; } - // GetEventMessageAndAtom will cache the event type for the future usage... - GetEventMessageAndAtom(aName, eBasicEventClass, &msg); - - // ...and then call this method recursively to get the message and atom from - // now updated sStringEventTable. - return GetEventMessageAndAtomForListener(aName, aOnName); + // sStringEventTable did not contain an entry for this event type string. + // Call GetEventMessageAndAtom, which will create an event type atom and + // cache it in sStringEventTable for future calls. + EventMessage msg = eUnidentifiedEvent; + RefPtr atom = GetEventMessageAndAtom(aName, eBasicEventClass, &msg); + atom.forget(aOnName); + return msg; } static nsresult GetEventAndTarget(Document* aDoc, nsISupports* aTarget, diff --git a/dom/base/nsContentUtils.h b/dom/base/nsContentUtils.h index 9e2f4d5f5391..3fa2d3e42f2e 100644 --- a/dom/base/nsContentUtils.h +++ b/dom/base/nsContentUtils.h @@ -242,9 +242,6 @@ struct EventNameMapping { int32_t mType; mozilla::EventMessage mMessage; mozilla::EventClassID mEventClassID; - // True if mAtom is possibly used by special SVG/SMIL events, but - // mMessage is eUnidentifiedEvent. See EventNameList.h - bool mMaybeSpecialSVGorSMILEvent; }; namespace mozilla { diff --git a/dom/events/EventNameList.h b/dom/events/EventNameList.h index 680d520496cd..6c9119d5f9e2 100644 --- a/dom/events/EventNameList.h +++ b/dom/events/EventNameList.h @@ -435,24 +435,31 @@ NON_IDL_EVENT(systemstatusbarclick, eXULSystemStatusBarClick, EventNameType_XUL, NON_IDL_EVENT(SVGLoad, eSVGLoad, EventNameType_None, eBasicEventClass) NON_IDL_EVENT(SVGScroll, eSVGScroll, EventNameType_None, eBasicEventClass) -// Only map the ID to the real event name when MESSAGE_TO_EVENT is defined. -#ifndef MESSAGE_TO_EVENT -EVENT(begin, eSMILBeginEvent, EventNameType_SMIL, eBasicEventClass) -#endif +// The three SMIL animation events. We mark these as NON_IDL_EVENT even though +// there exist IDL properties for them, because the IDL properties have +// different names (onbegin/onend/onrepeat rather than +// onbeginEvent/onendEvent/onrepeatEvent). +// And we use EventNameType_None because we don't want IsEventAttributeName to +// return true for onbeginEvent etc. NON_IDL_EVENT(beginEvent, eSMILBeginEvent, EventNameType_None, eSMILTimeEventClass) -// Only map the ID to the real event name when MESSAGE_TO_EVENT is defined. -#ifndef MESSAGE_TO_EVENT -EVENT(end, eSMILEndEvent, EventNameType_SMIL, eBasicEventClass) -#endif NON_IDL_EVENT(endEvent, eSMILEndEvent, EventNameType_None, eSMILTimeEventClass) -// Only map the ID to the real event name when MESSAGE_TO_EVENT is defined. -#ifndef MESSAGE_TO_EVENT -EVENT(repeat, eSMILRepeatEvent, EventNameType_SMIL, eBasicEventClass) -#endif NON_IDL_EVENT(repeatEvent, eSMILRepeatEvent, EventNameType_None, eSMILTimeEventClass) +#ifndef MESSAGE_TO_EVENT +// Repeat the SMIL animation events once more without the Event suffix, +// so that IsEventAttributeName() will return the right thing for these events. +// We use eUnidentifiedEvent here so that we don't accidentally treat these +// as alternate event names for the actual events. +// See bug 1845422 for cleaning this up. +NON_IDL_EVENT(begin, eUnidentifiedEvent, EventNameType_SMIL, + eSMILTimeEventClass) +NON_IDL_EVENT(end, eUnidentifiedEvent, EventNameType_SMIL, eSMILTimeEventClass) +NON_IDL_EVENT(repeat, eUnidentifiedEvent, EventNameType_SMIL, + eSMILTimeEventClass) +#endif + NON_IDL_EVENT(MozAfterPaint, eAfterPaint, EventNameType_None, eBasicEventClass) NON_IDL_EVENT(MozScrolledAreaChanged, eScrolledAreaChanged, EventNameType_None, @@ -530,7 +537,7 @@ EVENT(webkitTransitionEnd, eWebkitTransitionEnd, EventNameType_All, // These are only here so that IsEventAttributeName() will return the right // thing for these events. We could probably remove them if we used // Element::GetEventNameForAttr on the input to IsEventAttributeName before -// looking it up in the hashtable... +// looking it up in the hashtable... see bug 1845422. EVENT(webkitanimationend, eUnidentifiedEvent, EventNameType_All, eAnimationEventClass) EVENT(webkitanimationiteration, eUnidentifiedEvent, EventNameType_All, diff --git a/dom/svg/SVGAnimationElement.h b/dom/svg/SVGAnimationElement.h index 7f41d623fdff..156bf8f9e43a 100644 --- a/dom/svg/SVGAnimationElement.h +++ b/dom/svg/SVGAnimationElement.h @@ -71,6 +71,33 @@ class SVGAnimationElement : public SVGAnimationElementBase, public SVGTests { void EndElement(ErrorResult& rv) { EndElementAt(0.f, rv); } void EndElementAt(float offset, ErrorResult& rv); + // Manually implement onbegin/onrepeat/onend IDL property getters/setters. + // We don't autogenerate these methods because the property name differs + // from the event type atom - the event type atom has an extra 'Event' tacked + // on at the end. (i.e. 'onbegin' corresponds to an event whose name is + // literally 'beginEvent' rather than 'begin') + + EventHandlerNonNull* GetOnbegin() { + return GetEventHandler(nsGkAtoms::onbeginEvent); + } + void SetOnbegin(EventHandlerNonNull* handler) { + EventTarget::SetEventHandler(nsGkAtoms::onbeginEvent, handler); + } + + EventHandlerNonNull* GetOnrepeat() { + return GetEventHandler(nsGkAtoms::onrepeatEvent); + } + void SetOnrepeat(EventHandlerNonNull* handler) { + EventTarget::SetEventHandler(nsGkAtoms::onrepeatEvent, handler); + } + + EventHandlerNonNull* GetOnend() { + return GetEventHandler(nsGkAtoms::onendEvent); + } + void SetOnend(EventHandlerNonNull* handler) { + EventTarget::SetEventHandler(nsGkAtoms::onendEvent, handler); + } + // SVGTests SVGElement* AsSVGElement() final { return this; } diff --git a/layout/reftests/svg/smil/container/deferred-anim-1.xhtml b/layout/reftests/svg/smil/container/deferred-anim-1.xhtml index 0df307526fe6..369f5720457a 100644 --- a/layout/reftests/svg/smil/container/deferred-anim-1.xhtml +++ b/layout/reftests/svg/smil/container/deferred-anim-1.xhtml @@ -25,7 +25,7 @@ // In the pass case, we'll get an end event almost immediately. // In the failure case, wait 30s before giving up. timeoutID = window.setTimeout(giveUp, 30000); - anim.addEventListener('end', finish, true); + anim.addEventListener('endEvent', finish, true); } function giveUp() { diff --git a/layout/reftests/svg/smil/container/deferred-tree-1.xhtml b/layout/reftests/svg/smil/container/deferred-tree-1.xhtml index 1b700f180a93..ac83d5334093 100644 --- a/layout/reftests/svg/smil/container/deferred-tree-1.xhtml +++ b/layout/reftests/svg/smil/container/deferred-tree-1.xhtml @@ -26,7 +26,7 @@ // In the pass case, we'll get an end event almost immediately. // In the failure case, wait 30s before giving up. timeoutID = window.setTimeout(giveUp, 30000); - anim.addEventListener('end', finish, true); + anim.addEventListener('endEvent', finish, true); } function giveUp() { diff --git a/testing/web-platform/tests/svg/animations/custom-events.html b/testing/web-platform/tests/svg/animations/custom-events.html new file mode 100644 index 000000000000..1c49106b5108 --- /dev/null +++ b/testing/web-platform/tests/svg/animations/custom-events.html @@ -0,0 +1,75 @@ + +Custom events with the names "end" and "endEvent" and their effects on various types of event listeners + + + + + + + + + + diff --git a/testing/web-platform/tests/svg/animations/event-listeners.html b/testing/web-platform/tests/svg/animations/event-listeners.html new file mode 100644 index 000000000000..ca2b9b72f5bc --- /dev/null +++ b/testing/web-platform/tests/svg/animations/event-listeners.html @@ -0,0 +1,62 @@ + +Event handling of endEvent with various types of event listeners + + + + + + + + + + +