1845381 - Different behavior than Chrome with SMIL animation event names, e.g. "end" vs "endEvent"

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Markus Stange [:mstange]]

In Firefox, a SMIL animation end event will notify both "end" and "endEvent" listeners:

el.addEventListener("end", () => log("end listener fired"));
el.addEventListener("endEvent", () => log("endEvent listener fired"));

But in Chrome, only the endEvent listener is notified.

Firefox's current behavior is causing problems for me in bug 1834370, where I want to look up listeners based on the event type atom, and onend and onendEvent are two different atoms.
We could map them both to the same event atom, but this would change behavior for el.dispatchEvent(new CustomEvent("end" /* or "endEvent" */)), where we only want to notify one of the listeners (and where Firefox's and Chrome's behavior currently matches).

Can we switch to Chrome's behavior and only notify endEvent listeners?

The two other types with the same problem are beginEvent and repeatEvent.
I only know of two tests which rely on Firefox's current behavior: deferred-anim-1.xhtml and deferred-tree-1.xhtml both use addEventListener("end", ...). I haven't found uses of addEventListener("begin", ...) or addEventListener("repeat", ...).

Comment 1

[Robert Longson [:longsonr]]

I suspect you just need to delete these 3 lines and see what happens:

https://searchfox.org/mozilla-central/source/dom/svg/SVGElement.cpp#1438

Comment 2

[Markus Stange [:mstange]]

(In reply to Robert Longson [:longsonr] from comment #1)

https://searchfox.org/mozilla-central/source/dom/svg/SVGElement.cpp#1438

Thanks for the pointer! However I think this is one of the few location I want to leave untouched: This is what makes <animate onend="handler"> work, and that's the part that I'd like to keep. It's just addEventListener that I want to change.

Also, the IDL properties should have still the name onbegin / onrepeat / onend. This might be the root of the trouble. I'm currently looking into it.

Comment 3

[Markus Stange [:mstange]]

Attachment: testcase with addEventListener and IDL listeners

Comment 4

[Markus Stange [:mstange]]

Attachment: testcase with addEventListener and attribute handlers

Comment 5

[Markus Stange [:mstange]]

These testcases show another difference between Firefox and Chrome, about how dispatchEvent(new CustomEvent("...")) is treated:
In Firefox, dispatchEvent(new CustomEvent("end")) calls onend IDL listeners but not onend attribute handlers.
In Chrome, dispatchEvent(new CustomEvent("end")) calls neither onend IDL listeners nor onend attribute handlers.
In Firefox, dispatchEvent(new CustomEvent("endEvent")) does not call onend IDL listeners, but calls onend attribute handlers.
In Chrome, dispatchEvent(new CustomEvent("endEvent")) calls both onend IDL listeners and onend attribute handlers.

Both browsers call both onend IDL listeners and onend attribute handlers for the native SMIL animation end event.

Neither browser supports onendEvent as an IDL property or an attribute handler name.

Comment 6

[Markus Stange [:mstange]]

Attachment: Bug 1845381 - For begin/repeat/end SMIL animation events, only support "beginEvent"/"repeatEvent"/"endEvent" in addEventListener. r=smaug!,#webcompat-reviewers

Before this patch, we would have called listeners added with both
addEventListener("end", ...) and addEventListener("endEvent", ...).

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
store event listeners in a map keyed by event type atom (bug 1834370).

Comment 7

[Markus Stange [:mstange]]

https://treeherder.mozilla.org/jobs?repo=try&revision=99a5a17ee49cdcb37a5b7044496e23f6f78ca15c

Comment 8

[Markus Stange [:mstange]]

Attachment: some notes I wrote for myself when reading nsContentUtils event mapping code

Comment 9

[Daniel Holbert [:dholbert]]

(In reply to Markus Stange [:mstange] from comment #0)

In Firefox, a SMIL animation end event will notify both "end" and "endEvent" listeners:
[...]
Can we switch to Chrome's behavior and only notify endEvent listeners?

The two other types with the same problem are beginEvent and repeatEvent.

Yeah, it looks to me like this should be fine.

I'm not finding any spec support for begin, end, or repeat as event names. I suspect we support them by accident as a result of the fact that the on*** attributes for these events have no Event suffix. (See e.g. https://www.w3.org/TR/SVG2/interact.html which pairs up beginEvent with onbegin, etc. At some point in the past, I'll bet the only [or most-straightforward] way to support onbegin was to support begin as an event name, and that wart stuck around.)

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #9)

At some point in the past, I'll bet the only [or most-straightforward] way to support onbegin was to support begin as an event name, and that wart stuck around.

Or maybe it was just a mistake. :) It looks like this dates back to https://bugzilla.mozilla.org/show_bug.cgi?id=572270 which added both begin and beginEvent, and it's not obvious that we really had to add both. The commit there was:
https://hg.mozilla.org/mozilla-central/rev/f73e5032cfadb8b8d575b8d1d40814700e038530#l2.13
(Note, the commit message there has a typo'd bug number, so if you click through to the bug from that commit, you'll end up somewhere else.)

Comment 11

[Brian Birtles (:birtles)]

I'm trying to remember why the code ended up like that. I feel like there was some requirement in our code at that time for attribute handlers to match event names. However, SMIL defined "beginEvent" and onbegin so we had to add the "begin" event as well in order to satisfy that requirement.

Comment 12

[Markus Stange [:mstange]]

Hi Brian! That makes sense. My first patch indeed broke attribute handlers, and I had to re-introduce entries to EventNameList.h so that the attribute handlers would work. I've filed bug 1845422 for a better fix of that part.
The crucial bit seems to be that the "fake" event names for attribute handlers need to map to eUnidentifiedEvent - this will prevent addEventListener from mapping the fake event names to the event message of the real event.

(In reply to Markus Stange [:mstange] from comment #7)

https://treeherder.mozilla.org/jobs?repo=try&revision=99a5a17ee49cdcb37a5b7044496e23f6f78ca15c

This push had the broken attribute handlers. Push with fixed attribute handlers:
https://treeherder.mozilla.org/jobs?repo=try&revision=e1ad4f9c16af4e280197f6e59febf5e6fa6b24fd

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e104c452e06d
For begin/repeat/end SMIL animation events, only support "beginEvent"/"repeatEvent"/"endEvent" in addEventListener. r=dholbert,smaug

Comment 14

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/41198 for changes under testing/web-platform/tests

Comment 15

[Noemi Erli[:noemi_erli]]

Backed out changeset e104c452e06d (Bug 1845381) for causing failures in href-animate-element.html CLOSED TREE

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&group_state=expanded&collapsedPushes=842382&selectedTaskRun=WeDAbFWJSDu7fUk_ORCakQ.0&resultStatus=testfailed%2Cbusted%2Cexception%2Cretry%2Cusercancel&searchStr=linux%2C18.04%2Cx64%2Cwebrender%2Cdebug%2Cweb%2Cplatform%2Ctests%2Cwith%2Csoftware%2Cwebrender%2Cenabled%2Cwithout%2Cfission%2Cenabled%2Ctest-linux1804-64-qr%2Fdebug-web-platform-tests-swr-nofis%2Cwpt16&revision=e104c452e06d5696c7ffa9033bfa60a8a2a4f349

Log: https://treeherder.mozilla.org/logviewer?job_id=424029216&repo=autoland&lineNumber=20057

Backout: https://hg.mozilla.org/integration/autoland/rev/e17bc7b1f2b1f219643100d1ce3d73b47853873f

Comment 16

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR was closed without merging

Comment 17

[Markus Stange [:mstange]]

(In reply to Noemi Erli[:noemi_erli] from comment #15)

Backed out changeset e104c452e06d (Bug 1845381) for causing failures in href-animate-element.html CLOSED TREE

href-animate-element.html was added in bug 1245751 and uses waitEvent(animate, 'begin') and, as expected, fails in other browsers:
https://wpt.fyi/results/svg/linking/scripted/href-animate-element.html?label=experimental&label=master&aligned

I'll just change it to use beginEvent.

Comment 18

[Markus Stange [:mstange]]

I've updated the patch. This is ready to re-land as soon as the soft-freeze ends.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a9569d599736
For begin/repeat/end SMIL animation events, only support "beginEvent"/"repeatEvent"/"endEvent" in addEventListener. r=webcompat-reviewers,dholbert,smaug,twisniewski

Comment 20

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/a9569d599736

Comment 21

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot