Add onanimation* event handlers, and ontransitionend (and webkit versions)

RESOLVED FIXED in Firefox 51

Status

()

RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: smaug, Assigned: mantaroh)

Tracking

({dev-doc-complete})

unspecified
mozilla51
dev-doc-complete
Points:
---

Firefox Tracking Flags

(firefox51 fixed)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(5 attachments)

(Reporter)

Description

5 years ago
Looks like Blink just got them
https://chromiumcodereview.appspot.com/23583032/

I don't know which spec actually defines them.
The relevant spec would be http://dev.w3.org/csswg/css-animations/ .  If it doesn't define them and should, I'd need to find out the right way to define them.  (Is this actually something we want?)
Do we want these for new events?
Flags: needinfo?(annevk)

Comment 3

4 years ago
You could maybe get HTML to define them.

I think we want these since we don't really have any better convenient shorthand and this helps feature testing.
Flags: needinfo?(annevk)
(Reporter)

Comment 4

3 years ago
bbirtles, perhaps you have some input to this?
Flags: needinfo?(bbirtles)
I think we should do it. It's on my list of things to do but it's pretty far down the list. If we start encountering content with this then I'd be happy to prioritize it.
Flags: needinfo?(bbirtles)
Chiko is willing to work this bug, the event handlers are not yet specced though.
Assignee: nobody → chikoski
I started working on the spec for this. Anne, does this look right?

  https://github.com/w3c/csswg-drafts/compare/animation-event-handlers

Once we work out the right approach, we need to do the same for transitionend.
Flags: needinfo?(annevk)

Comment 8

3 years ago
Yeah that seems fine.
Flags: needinfo?(annevk)
We discussed this at TPAC and Apple, in particular, seemed keen to remove this feature. They agreed to hide it from Web content and Google said their usage counters are very low so they seemed ok with removing it too.

Some very sparse notes here:
https://lists.w3.org/Archives/Public/www-style/2015Oct/0226.html (see item 2)
(Reporter)

Comment 10

3 years ago
I wonder why people want to remove the handlers from the web. And blink certainly still supports them in their latest dev build.
This is now something we should decide soon, see Bug 1236979.
Dean, what was your concern with these event handlers? Have you removed them from WebKit yet?
Flags: needinfo?(dino)
(Side note: if we do end up needing to add these <div onanimation*=..."> attribute-based event handlers, we'll probably want to add webkit-prefixed versions as well, and we'll want to make sure we behave like webkit-based browsers do if both the prefixed & unprefixed attributes are specified.  See bug 1236979 comment 24 & other comments on that bug for more info.)
I spoke to Dean (Apple) today and he said they won't be removing the onanimation* handlers (or onwebkitanimation* handlers) after all.
Flags: needinfo?(dino)
(Reporter)

Comment 14

3 years ago
So we probably should add them too then (both prefixed and unprefixed).
Flags: needinfo?(dholbert)
Keywords: dev-doc-needed
Brian, do you know what changed Dean's mind?

In any case -- if Apple & Google are keeping them forever, then I suppose we'll want to add them as well (both prefixed & unprefixed), because we're likely to run into compat issues in the future (though we haven't yet).

I don't think this floats to the top of my webcompat priority-list at the moment, though, so I won't assign myself to this for now.  I'll get to it eventually, or others are free to take it, too.

(I suspect the code-change here isn't too hard; bug 1236979 comment 24 suggests it's a one-line change for each type of event.  Writing tests will be a bit more involved, though.)
Flags: needinfo?(dholbert) → needinfo?(bbirtles)
>  I'll get to it eventually

(Sorry, didn't notice this already had an assignee -- looks like Chiko may be taking this, which is great!)
(Broadening summary to cover ontransitionend & webkit variants as well. Hope that's OK.)
Summary: Add onanimation* event handlers → Add onanimation* event handlers, and ontransitionend (and webkit versions
(In reply to Daniel Holbert [:dholbert] from comment #15)
> Brian, do you know what changed Dean's mind?

No. It was the last session of TPAC so I think he was just expressing his preference without really considering the compat issues. He said they'd remove support from webkit for Web content (but presumably not for regular webviews).

When I spoke to Dean yesterday he seemed to think they wouldn't get away with removing them after all and was surprised he had promised to remove them. Simon Fraser and Ted (Edward O'Connor) were there and seemed to concur.
Flags: needinfo?(bbirtles)
(Assignee)

Comment 19

2 years ago
Hi chiko.
Will you have time to work on this bug?
(I'd like to steal this bug.)
Status: NEW → ASSIGNED
Flags: needinfo?(chikoski)
(Assignee)

Comment 20

2 years ago
Current each browser's behavior is as follow.

Test page: output.jsbin.com/geriwik

                  onAnimationXXX | onWebkitAnimationXX | onTransitionEnd | onWebkitTransitionEnd
------------------------------------------------------------------------------------------------
Firefox(50)         not work     |       not work      |    not work     |    not work
Chromium(54)          work       |         work        |    not work     |      work
Safari(9.1.2)         work       |         work        |      work       |      work
Edge(25.10586.0.0)  not work     |       not work      |    not work     |    not work
------------------------------------------------------------------------------------------------

I think we need to add unprefix event handler.
However I'm not sure we should add prefixed event handler.


Hi Daniel,
In comment #18, webkit won't remove prefix event handler. And Webkit/Blink support this.
So I think it's not particularly a problem that we add prefixed event handler for Web-Compat.

How you do you feel about it?
Flags: needinfo?(dholbert)
As long as there's no webcompat demand/requirements for these attributes, we should discuss adding the unprefixed versions to the CSS Animations & CSS Transitions specs before (or while) we implement them.  (as a followup to the mailing list post from comment 9 with earlier discussion)

With spec support, we'll be more likely to get buy-in from Edge on implementing these as well (and from Blink on implementing unprefixed "onTransitionEnd"), so that we actually end up with useful interop.

> In comment #18, webkit won't remove prefix event handler.

I'm not sure that's what Brian was saying in comment 18. I thought he was just talking about these "on$XYZ" attributes *in general*, not specifically about the prefixed versions.  And I think it seems conceivable that Apple/WebKit might keep around e.g. onAnimationXXX but remove onWebkitAnimationXXX if usage is low enough, in light of their policy at https://webkit.org/blog/6131/updating-our-prefixing-policy/ .  Not sure though.

Anyway, I'm neutral on whether it's worth adding the prefixed versions.  Since there's not much usage / webcompat demand, I guess the main supporting argument for adding onWebkitAnimationXXX is simply *consistency* (with onAnimationXXX, and with the fact that we do also fire webkit-prefixed versions of the event itself (as of bug 1236979)).  So, I'm on board with supporting the prefixed versions of these attributes from that perspective, I think.
Flags: needinfo?(dholbert)
Yes, Dean's comment was specifically about removing the onanimationstart, onanimationend, onanimationiteration, and ontransitionend event handlers. I didn't ask specifically about onwebkitanimationstart etc.

I'm not too concerned about whether or not we add the webkit equivalents but I think I agree with Daniel that if we're firing webkitanimationstart, we should probably call onwebkitanimationstart too.

Updated

2 years ago
Assignee: chikoski → nobody
Status: ASSIGNED → NEW
(Assignee)

Comment 23

2 years ago
https://treeherder.mozilla.org/#/jobs?repo=try&revision=841a62ba37cc6e7c4a3c1074f2751c18e5521a19
Assignee: nobody → mantaroh
Status: NEW → ASSIGNED
Flags: needinfo?(chikoski)
(Assignee)

Comment 24

2 years ago
Thanks Daniel, Brian.

Brian,
I have a question about prefix event handler. Please let me confirm.

In WebKit, they defined prefix event handler like "webkitAnimationStart".[1]
However, we compare lower case character of event name when adding content attribute event handler.[2] So we can't add an event handler using content attribute.

[1] https://github.com/WebKit/webkit/blob/f20c9674d3fff188aa6fbf78393c8b2abbee49a7/Source/WebCore/dom/EventNames.h#L242
[2] https://dxr.mozilla.org/mozilla-central/rev/4c05938a64a7fde3ac2d7f4493aee1c5f2ad8a0a/dom/base/nsContentUtils.cpp#3734

I think that we need to compare as mixed-case.

How do you think about this?
Flags: needinfo?(bbirtles)
Sorry, I don't understand. If you are talking about content attributes, then the HTML parser lowercases them.
Flags: needinfo?(bbirtles)
While updating CSS transitions I went to check other browsers' support and it seems to be as follows:

                                          Safari   Chrome   Edge
----------------------------------------+--------+--------+-------
ontransitionend content attribute       |   o    |   o    | 
onwebkittransitionend content attribute |   o    |   o    |
ontransitionend IDL attribute           |   o    |        |
onwebkittransitionend IDL attribute     |   o    |        |

Browsing the chromium source, it appears the same is true for animations.

The current CSS Animations spec requires user agents to support *both* the content attribute and IDL attribute. I'm surprised that Chrome only supports the content attribute. Still, I suppose for consistency we should support the IDL attribute too.
Creating PR for specifying ontransitionend handlers in CSS Transitions: https://github.com/w3c/csswg-drafts/pull/344
(Assignee)

Comment 28

2 years ago
Created attachment 8775391 [details]
Bug 911987 part 1 - Add onanimation** and ontransitionend event handlers.

This event handler is defined CSS-Animation and CSS-Transition.
https://drafts.csswg.org/css-animations-1/#event-animationevent
https://github.com/w3c/csswg-drafts/pull/344

Review commit: https://reviewboard.mozilla.org/r/67562/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67562/
Attachment #8775391 - Flags: review?(masayuki)
Attachment #8775392 - Flags: review?(bbirtles)
(Assignee)

Comment 29

2 years ago
Created attachment 8775392 [details]
Bug 911987 part 2 - Add the onanimation** and ontransition end tests.

Review commit: https://reviewboard.mozilla.org/r/67564/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/67564/
(Assignee)

Comment 30

2 years ago
Thanks brian,

(In reply to Brian Birtles (:birtles) from comment #26)
> The current CSS Animations spec requires user agents to support *both* the
> content attribute and IDL attribute. I'm surprised that Chrome only supports
> the content attribute. Still, I suppose for consistency we should support
> the IDL attribute too.
As discussed with you, we should implement two nsGkAtom for EventListener and content attribute.

As the first of this work, I created the patch of no-prefixed event handlers.

Masayuki, 
Could you please review patch of event handler part?
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #29)
> Created attachment 8775392 [details]
> Bug 911987 part 2 - Add the onanimation** and ontransition end tests.

Though I am not reviewer of this patch, I had an idea about the test of this bug, so I'd like to leave comments here.

1) We should check that the callback is surely called just *once*.
2) We should check that the event received in onanimation** callback equals to the event received in callback of addEventListener().
Comment on attachment 8775391 [details]
Bug 911987 part 1 - Add onanimation** and ontransitionend event handlers.

Looks good to me, but webidl change needs smaug's review too.
Attachment #8775391 - Flags: review?(masayuki)
Attachment #8775391 - Flags: review?(bugs)
Attachment #8775391 - Flags: review+
(Assignee)

Comment 34

2 years ago
Comment on attachment 8775392 [details]
Bug 911987 part 2 - Add the onanimation** and ontransition end tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67564/diff/1-2/
(In reply to Mantaroh Yoshinaga[:mantaroh] from comment #34)
> Comment on attachment 8775392 [details]
> Bug 911987 part 2 - Add the onanimation** and ontransition end tests.

This test can be passed even if onanimation* callback is called *twice* and then the callback of addEventListner is called.

You can make contentAttributeEvent and idlAttributeEvent array and push each event on each callbacks and check the length of each arrays.

Also, resolving retPromise in compareEvent() is not a good idea to me because it will make accidentally the test success even if one more callback is called after resolving the promise.
(Reporter)

Comment 36

2 years ago
Comment on attachment 8775391 [details]
Bug 911987 part 1 - Add onanimation** and ontransitionend event handlers.

https://reviewboard.mozilla.org/r/67562/#review64764

I assume the idea is that also SVG elements supports the event handlers (which the patch does). r+.
Attachment #8775391 - Flags: review?(bugs) → review+
(Assignee)

Updated

2 years ago
Attachment #8775392 - Flags: review?(bbirtles)
(Assignee)

Comment 37

2 years ago
Comment on attachment 8775391 [details]
Bug 911987 part 1 - Add onanimation** and ontransitionend event handlers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67562/diff/1-2/
Attachment #8775391 - Flags: review+
Attachment #8775392 - Flags: review?(bbirtles)
(Assignee)

Comment 38

2 years ago
Comment on attachment 8775392 [details]
Bug 911987 part 2 - Add the onanimation** and ontransition end tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67564/diff/2-3/
Comment on attachment 8775392 [details]
Bug 911987 part 2 - Add the onanimation** and ontransition end tests.

https://reviewboard.mozilla.org/r/67564/#review65270

These tests should be rewritten and added to layout/style/test/test_animations.html (or a file in the same directory if it's too difficult to add it to that same file, e.g. see test_animations_event_order.html).

dom/animation/tests is for Web Animations tests and dom/animation/tests/css-animations is for tests that cover the integration between Web Animations and CSS Animations.
Attachment #8775392 - Flags: review?(bbirtles)
(Assignee)

Comment 41

2 years ago
Comment on attachment 8775391 [details]
Bug 911987 part 1 - Add onanimation** and ontransitionend event handlers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67562/diff/2-3/
Attachment #8775392 - Flags: review?(bbirtles)
(Assignee)

Comment 42

2 years ago
Comment on attachment 8775392 [details]
Bug 911987 part 2 - Add the onanimation** and ontransition end tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67564/diff/3-4/
(Assignee)

Comment 43

2 years ago
Thanks Brian.

(In reply to Brian Birtles (:birtles) from comment #39)
> Comment on attachment 8775392 [details]
> Bug 911987 part 2 - Add the onanimation** and ontransition end tests.
> 
> https://reviewboard.mozilla.org/r/67564/#review65270
> 
> These tests should be rewritten and added to
> layout/style/test/test_animations.html (or a file in the same directory if
> it's too difficult to add it to that same file, e.g. see
> test_animations_event_order.html).
I moved this test to layout/style/tests.

Could you please confirm this patch?
Comment on attachment 8775392 [details]
Bug 911987 part 2 - Add the onanimation** and ontransition end tests.

https://reviewboard.mozilla.org/r/67564/#review65810

This is better, but could be much simpler.

::: layout/style/test/test_animations_event_handler_attribute.html:9
(Diff revision 4)
> +https://bugzilla.mozilla.org/show_bug.cgi?id=911987
> +-->
> +<head>
> +  <meta charset=utf-8>
> +  <title>Test for CSS Animation and Transition event handler
> +         attribute.(Bug 911987)</title>

Very minor nit: s/attribute/attributes/

(Very very very minor nit: Please add a space before "(Bug 911987)")

::: layout/style/test/test_animations_event_handler_attribute.html:32
(Diff revision 4)
> +<pre id="test">
> +<script type="application/javascript">
> +'use strict';
> +
> +var gReceivedAttributeEvents = [];
> +var gReceivedListenerEvents = [];

I'm not sure that gReceivedListenerEvents really adds any value? Do we need it?

::: layout/style/test/test_animations_event_handler_attribute.html:59
(Diff revision 4)
> +function clearReceivedEvents() {
> +  gReceivedAttributeEvents = [];
> +  gReceivedListenerEvents = [];
> +}
> +
> +function checkReceivedEvents(eventName, eventsCount) {

So it looks like we always set eventsCount to 1 and we always call clearReceivedEvents() after.

We should drop |eventsCount| and combine clearReceivedEvents(). Perhaps just call this checkEvent()?

::: layout/style/test/test_animations_event_handler_attribute.html:62
(Diff revision 4)
> +}
> +
> +function checkReceivedEvents(eventName, eventsCount) {
> +  var compareEvents = function(expected, target) {
> +    for (var prop in expected) {
> +      if (prop == "elapsedTime") {

Nit: ===

::: layout/style/test/test_animations_event_handler_attribute.html:64
(Diff revision 4)
> +function checkReceivedEvents(eventName, eventsCount) {
> +  var compareEvents = function(expected, target) {
> +    for (var prop in expected) {
> +      if (prop == "elapsedTime") {
> +        ok(Math.abs(expected.elapsedTime - target.elapsedTime) < 0.000002,
> +           "elapsedTime. expected[" + expected[prop] + "],target[" +

Nit: Space between ']' and ',target['

::: layout/style/test/test_animations_event_handler_attribute.html:95
(Diff revision 4)
> +// Take over the refresh driver right from the start.
> +advance_clock(0);
> +
> +// 1. TEST FOR EVENT HANDLER(animation).
> +
> +// 1.a Content attribtue.

Minor nit: 1a.
(Here and below)

::: layout/style/test/test_animations_event_handler_attribute.html:99
(Diff revision 4)
> +
> +// 1.a Content attribtue.
> +
> +var div = document.createElement("div");
> +gDisplay.appendChild(div);
> +addHandlers(div, true);

We try to avoid passing boolean parameters when possible since they are hard to understand at the call site.

So, here we could either add an enum parameter (see animation_utils.js where we have the RunningOn and ExpectComparisonTo enums), *or* we could simply register both at once and test together. E.g. have gReceivedContentAttributeEvents and gReceivedIDLAttributeEvents then in checkEvent("animationstart") report which one failed. That would save repeating the same code twice.
Attachment #8775392 - Flags: review?(bbirtles)
(Assignee)

Comment 45

2 years ago
Comment on attachment 8775391 [details]
Bug 911987 part 1 - Add onanimation** and ontransitionend event handlers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67562/diff/3-4/
Attachment #8775392 - Flags: review?(bbirtles)
(Assignee)

Comment 46

2 years ago
Comment on attachment 8775392 [details]
Bug 911987 part 2 - Add the onanimation** and ontransition end tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67564/diff/4-5/
(Assignee)

Comment 47

2 years ago
https://reviewboard.mozilla.org/r/67564/#review65810

Thanks Brian,

I addressed the patch of test part.
Comment on attachment 8775392 [details]
Bug 911987 part 2 - Add the onanimation** and ontransition end tests.

https://reviewboard.mozilla.org/r/67564/#review66230

I think this is fine, but I think this could be more simple (and I think we should aim to make our tests as simple as possible since we need to maintain them).

::: layout/style/test/test_animations_event_handler_attribute.html:16
(Diff revision 5)
> +  .transition-div {
> +    margin-left: 0px;
> +    transition: margin-left 100ms;
> +  }

Nit: This would be slightly easier to understand if you simply included these declarations in the test code, e.g.

  div.style.marginLeft = '0';
  div.style.transition = 'margin-left 100ms';

::: layout/style/test/test_animations_event_handler_attribute.html:68
(Diff revision 5)
> +  var compareEvents = function(expected, target) {
> +    for (var prop in expected) {
> +      if (prop === "elapsedTime") {
> +        ok(Math.abs(expected.elapsedTime - target.elapsedTime) < 0.000002,
> +           "elapsedTime. expected[" + expected[prop] + "], target[" +
> +           target[prop] + "]");
> +      } else {
> +        ok(expected[prop] == target[prop],
> +           "property[" + prop + "]. expected[" + expected[prop] +
> +          "], target[" + target[prop] + "]");
> +      }
> +    }
> +  };

Why would the events be different?

I think we can make this test a lot simpler by just testing event handler registration. Isn't that all we need to test in this bug?

e.g. what about something like:

  // Create div elements with event handlers.
  //
  // We need two div elements: one with the content attribute specified and one
  // with the IDL attribute specified since we can't set these independently.
  function createTargetWithHandlers() {
    var contentAttributeElement = document.createElement('div');
    var idlAttributeElement     = document.createElement('div');

    document.body.appendChild(contentAttributeElement);
    document.body.appendChild(idlAttributeElement);

    // Add handlers
    [ 'onanimationstart',
      'onanimationiteration',
      'onanimationend',
      'ontransitionend' ].forEach(event => {

      contentAttributeElement.setAttribute(event, 'handleEvent(event)');
      contentAttributeElement.handlerType = 'content attribute';

      idlAttributeElement[event] = handleEvent;
      idlAttributeElement.handlerType = 'IDL attribute';
    });

    return [contentAttributeElement, idlAttributeElement];
  }

  function handleEvent(event) {
    // We expect each event to be checked then cleared after it is received.
    if (event.target.receivedEvent) {
      ok(false, `Received ${event.type} event but we have yet to clear previous
                {event.target.receivedEvent} event`);
      return;
    }
    event.target.receivedEvent = event.type;
  }

  function checkReceivedEvent(elems, eventType) {
    elems.forEach(elem => {
      is(elem.receivedEvent, eventType,
        `Expected to receive '${eventType}', got '${elem.receivedEvent}',
          for event handler registered using ${elem.handlerType}`);
      elem.receivedEvent = undefined;
    });
  }

  // Take over the refresh driver right from the start.
  advance_clock(0);

  // 1. Test for CSS Animation event handlers

  var targets = createTargetWithHandlers();
  targets.forEach(target => {
    target.style.animation = 'anim 100ms 2';
  });

  advance_clock(0);
  checkReceivedEvent(targets, 'animationstart');

  advance_clock(100);
  checkReceivedEvent(targets, 'animationiteration');

  advance_clock(200);
  checkReceivedEvent(targets, 'animationiteration');

  targets.forEach(target => target.remove());

  ...

I haven't run this code but I think you get the idea. It seems like that would be sufficient? What do you think?
Attachment #8775392 - Flags: review?(bbirtles)
(Assignee)

Comment 49

2 years ago
Created attachment 8777660 [details]
Bug 911987 part 3 - Add onwebkit prefixed event handler tests.

Review commit: https://reviewboard.mozilla.org/r/69128/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69128/
Attachment #8777660 - Flags: review?(bbirtles)
Attachment #8777661 - Flags: review?(masayuki)
Attachment #8775392 - Flags: review?(bbirtles)
(Assignee)

Comment 50

2 years ago
Created attachment 8777661 [details]
Bug 911987 part 4 - Add onwebkitanimation** and onwebkittransitionend event handlers.

Add the onwebkit prefixed event handler attributes of CSS-Animation
and CSS-Transition.

We need to support both content attributes and IDL attributes like
onanimation** attributes.

We should support attribute of lower-case and mixed-case. But currently
Gecko defined as mixed-case only. [1][2]

[1] https://dxr.mozilla.org/mozilla-central/rev/6608e5864780589b25d5421c3d3673ab30c4c318/dom/base/nsContentUtils.cpp#747
[2] https://dxr.mozilla.org/mozilla-central/rev/6608e5864780589b25d5421c3d3673ab30c4c318/dom/events/EventNameList.h#977

So we can't find the related Atom when we set content attributes as lower-case. [3]

[3] https://dxr.mozilla.org/mozilla-central/rev/6608e5864780589b25d5421c3d3673ab30c4c318/dom/base/nsGlobalWindow.h#818

In this patch, I add two Atoms in order to support mixed and lower case.

Review commit: https://reviewboard.mozilla.org/r/69130/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69130/
(Assignee)

Comment 51

2 years ago
Comment on attachment 8775391 [details]
Bug 911987 part 1 - Add onanimation** and ontransitionend event handlers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67562/diff/4-5/
(Assignee)

Comment 52

2 years ago
Comment on attachment 8775392 [details]
Bug 911987 part 2 - Add the onanimation** and ontransition end tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67564/diff/5-6/
(Assignee)

Comment 53

2 years ago
https://reviewboard.mozilla.org/r/67564/#review66230

> Why would the events be different?
> 
> I think we can make this test a lot simpler by just testing event handler registration. Isn't that all we need to test in this bug?
> 
> e.g. what about something like:
> 
>   // Create div elements with event handlers.
>   //
>   // We need two div elements: one with the content attribute specified and one
>   // with the IDL attribute specified since we can't set these independently.
>   function createTargetWithHandlers() {
>     var contentAttributeElement = document.createElement('div');
>     var idlAttributeElement     = document.createElement('div');
> 
>     document.body.appendChild(contentAttributeElement);
>     document.body.appendChild(idlAttributeElement);
> 
>     // Add handlers
>     [ 'onanimationstart',
>       'onanimationiteration',
>       'onanimationend',
>       'ontransitionend' ].forEach(event => {
> 
>       contentAttributeElement.setAttribute(event, 'handleEvent(event)');
>       contentAttributeElement.handlerType = 'content attribute';
> 
>       idlAttributeElement[event] = handleEvent;
>       idlAttributeElement.handlerType = 'IDL attribute';
>     });
> 
>     return [contentAttributeElement, idlAttributeElement];
>   }
> 
>   function handleEvent(event) {
>     // We expect each event to be checked then cleared after it is received.
>     if (event.target.receivedEvent) {
>       ok(false, `Received ${event.type} event but we have yet to clear previous
>                 {event.target.receivedEvent} event`);
>       return;
>     }
>     event.target.receivedEvent = event.type;
>   }
> 
>   function checkReceivedEvent(elems, eventType) {
>     elems.forEach(elem => {
>       is(elem.receivedEvent, eventType,
>         `Expected to receive '${eventType}', got '${elem.receivedEvent}',
>           for event handler registered using ${elem.handlerType}`);
>       elem.receivedEvent = undefined;
>     });
>   }
> 
>   // Take over the refresh driver right from the start.
>   advance_clock(0);
> 
>   // 1. Test for CSS Animation event handlers
> 
>   var targets = createTargetWithHandlers();
>   targets.forEach(target => {
>     target.style.animation = 'anim 100ms 2';
>   });
> 
>   advance_clock(0);
>   checkReceivedEvent(targets, 'animationstart');
> 
>   advance_clock(100);
>   checkReceivedEvent(targets, 'animationiteration');
> 
>   advance_clock(200);
>   checkReceivedEvent(targets, 'animationiteration');
> 
>   targets.forEach(target => target.remove());
> 
>   ...
> 
> I haven't run this code but I think you get the idea. It seems like that would be sufficient? What do you think?

Thanks Brian,

> Why would the events be different?

This test of comparing event is my misunderstanding about Event handling.
I thought that these event which was received from attribute and listener is different.

So I addressed it.
Would you please confirm this patch again?
https://reviewboard.mozilla.org/r/69130/#review66268

Hmm, I don't know how to implement webkit prefixed events. Smaug?
Attachment #8777661 - Flags: review?(masayuki)
Comment on attachment 8775392 [details]
Bug 911987 part 2 - Add the onanimation** and ontransition end tests.

https://reviewboard.mozilla.org/r/67564/#review66298

::: layout/style/test/test_animations_event_handler_attribute.html:29
(Diff revision 6)
> +// Create the div element with event handlers.
> +// Event handler content attribute and idl attribute is exclusive.
> +// We can't set two handler to one element. So separate two elements.

// Create elements with event handlers.
//
// We need two elements: one with the content attribute specified and one
// with the IDL attribute specified since we can't set these independently.

::: layout/style/test/test_animations_event_handler_attribute.html:74
(Diff revision 6)
> +}
> +
> +// Take over the refresh driver right from the start.
> +advance_clock(0);
> +
> +// 1. TEST FOR EVENT HANDLER(animation).

"Test CSS animation event handlers"?

::: layout/style/test/test_animations_event_handler_attribute.html:76
(Diff revision 6)
> +// Take over the refresh driver right from the start.
> +advance_clock(0);
> +
> +// 1. TEST FOR EVENT HANDLER(animation).
> +
> +var divs = createTargetWithHandlers(gDisplay);

s/divs/targets/ ?
s/createTargetWithHandlers/createAndRegisterTargets/ ?

::: layout/style/test/test_animations_event_handler_attribute.html:83
(Diff revision 6)
> +  div.setAttribute('style', 'animation: anim 100ms 2');
> +  getComputedStyle(div).animationName;  // flush
> +});
> +
> +advance_clock(0);
> +checkReceivedEvent("onanimationstart", divs);

The event name is "animationstart" -- wouldn't it be better to pass that and drop the substring(2) from checkReceivedEvent?

::: layout/style/test/test_animations_event_handler_attribute.html:93
(Diff revision 6)
> +advance_clock(200);
> +checkReceivedEvent("onanimationend", divs);
> +
> +divs.forEach(div => { div.remove(); });
> +
> +// 2. TEST FOR EVENT HANDLER(transition).

"Test CSS transition event handlers"?
Attachment #8775392 - Flags: review?(bbirtles) → review+
Attachment #8777660 - Flags: review?(bbirtles) → review+
Comment on attachment 8777660 [details]
Bug 911987 part 3 - Add onwebkit prefixed event handler tests.

https://reviewboard.mozilla.org/r/69128/#review66304

r=me with the following changes

::: layout/style/test/test_animations_event_handler_attribute.html:27
(Diff revision 1)
> +const STANDARD_ATTRIBUTES = [
> +  'onanimationstart',
> +  'onanimationiteration',
> +  'onanimationend',
> +  'ontransitionend'
> +];
> +
> +const PREFIXED_ATTRIBUTES = [
> +  'onwebkitanimationstart',
> +  'onwebkitanimationiteration',
> +  'onwebkitanimationend',
> +  'onwebkittransitionend'
> +];

I think it would be easier if you just passed these in as needed:

e.g.

  let targets = createAndRegisterTargets(gDisplay,
                                         [ 'onanimationstart',
                                           'onanimationiteration',
                                           'onanimationend' ]);

Then later:

  targets = createAndRegisterTargets(gDisplay, [ 'oniterationend' ]);

  targets = createAndRegisterTargets(gDisplay,
                                     [ 'onwebkitanimationstart',
                                       'onwebkitanimationiteration',
                                       'onwebkitanimationend' ]);

  targets = createAndRegisterTargets(gDisplay, [ 'onwebkititerationend' ]);

(Personally, I would drop the parentElement parameter and just hard code gDisplay into createAndRegisterTargets but that's up to you.)

I think that would make this test a lot easier to read.

::: layout/style/test/test_animations_event_handler_attribute.html:53
(Diff revision 1)
>    var idlAttributeElement     = document.createElement("div");
>    parentElement.appendChild(contentAttributeElement);
>    parentElement.appendChild(idlAttributeElement);
>  
>    // Add handlers
> -  [ 'onanimationstart',
> +  var attributes = eventAttributes ? eventAttributes : STANDARD_ATTRIBUTES;

Based on the comment above, I think we can drop this line.

::: layout/style/test/test_animations_event_handler_attribute.html:77
(Diff revision 1)
> -       `Expected to receive '${eventType.substring(2)}', got ` +
> +             `Expected to receive '${eventType.substring(2)}', got ` +
> -       `'${element.receivedEventType}', for event handler registered using ` +
> -       ` ${element.handlerType}`);
> +             `'${element.receivedEventType}', for event handler registered ` +
> +             `using ${element.handlerType}`);

(Note that you don't actually need to break up and concatenate strings when using template literals over line breaks. But maybe that adds too much whitespace if you preserve indentation?)

::: layout/style/test/test_animations_event_handler_attribute.html:139
(Diff revision 1)
> +divs[0].remove();
> +divs[1].remove();

For consistency with the other places in this file:

  divs.forEach(div => { div.remove() });
(Reporter)

Comment 57

2 years ago
Comment on attachment 8777661 [details]
Bug 911987 part 4 - Add onwebkitanimation** and onwebkittransitionend event handlers.

https://reviewboard.mozilla.org/r/69130/#review66326

So please explain.
Would be perhaps good to have some test uploaded to the bug which could be run in various browsers testing the existence of different
onfoo in elements, and whether content attributes work and then also what the type of the event actually is.

::: dom/webidl/EventHandler.webidl:140
(Diff revision 1)
> +           // CSS-Animation and CSS-Transition legacy handlers.
> +           // This handler isn't standard.
> +           attribute EventHandler onwebkitanimationend;
> +           attribute EventHandler onwebkitanimationiteration;
> +           attribute EventHandler onwebkitanimationstart;
> +           attribute EventHandler onwebkitTransitionEnd;

I don't understand why *TransitionEnd is a special here. Per http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Element.idl#L204 those handlers are similar, all lowercase. 
And that is what various examples in the web seem to use too, all lowercase for handler.
Attachment #8777661 - Flags: review?(bugs) → review-
(Reporter)

Comment 58

2 years ago
I was told that
data:text/html,<script>document.write("onwebkitTransitionEnd" in document.documentElement); </script>
is false in webkit and 
data:text/html,<script>document.write("onwebkittransitionend" in document.documentElement); </script>
is true

So there is no attribute EventHandler onwebkitTransitionEnd;
but onwebkittransitionend
(In reply to Olli Pettay [:smaug] from comment #58)
> I was told that
> data:text/html,<script>document.write("onwebkitTransitionEnd" in
> document.documentElement); </script>
> is false in webkit and 
> data:text/html,<script>document.write("onwebkittransitionend" in
> document.documentElement); </script>
> is true
> 
> So there is no attribute EventHandler onwebkitTransitionEnd;
> but onwebkittransitionend

Those events aren't on document.documentElement, but window, but Olli is right -- it's all lowercase.

https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/frame/Window.idl?q=onwebkittransitionend&sq=package:chromium&l=193&dr=C
(err hang on, Safari has these on Element and Window, but Chrome just has them on Window it seems...?)
Summary: Add onanimation* event handlers, and ontransitionend (and webkit versions → Add onanimation* event handlers, and ontransitionend (and webkit versions)
(In reply to Olli Pettay [:smaug] from comment #57)
> Comment on attachment 8777661 [details]
> Bug 911987 part 4 - Add onwebkitanimation** and onwebkittransitionend event
> handlers.
> 
> https://reviewboard.mozilla.org/r/69130/#review66326
> 
> So please explain.
> Would be perhaps good to have some test uploaded to the bug which could be
> run in various browsers testing the existence of different
> onfoo in elements, and whether content attributes work and then also what
> the type of the event actually is.

There is a summary of on(webkit)transitionend attributes support in comment 26.

I noticed Mike Taylor also has a test for this here: https://miketaylr.com/bzla/onwebkittransitionend.html

However, I think Mantaroh noticed that in Safari, querying the event type produced different results depending on whether you accessed it in the event handler callback or if you stored the event and accessed it later, i.e. it would initially be the webkit version, and then it would reset to the unprefixed version. Perhaps due to the resetting behavior defined here: https://dom.spec.whatwg.org/#concept-event-listener-invoke

Mantaroh, could you make up a test (jsfiddle etc. would be fine) that can be run cross-browser that reports:

* For both prefixed and unprefixed versions, and
* For animationstart, animationiteration, animationend, transitionend, and transitionstart (only specced in CSS Animations Level 2, but I believe IE10+ have this):
  * Whether an event is received when the listener is registered using:
    - content attribute (on element)
    - IDL attribute on element
    - IDL attribute on Window
    - addEventListener
  * The event type in the handler
  * The event type if the event is stored and accessed later (i.e. in a separate task/microtask)

I think you might already have something like this?
Flags: needinfo?(mantaroh)
(Assignee)

Comment 62

2 years ago
Created attachment 8778068 [details]
animation event attribute investigation - Summary.pdf

Thanks Olli, Brian,

(In reply to Brian Birtles (:birtles) from comment #61)
> Mantaroh, could you make up a test (jsfiddle etc. would be fine) that can be
> run cross-browser that reports:
> 
> * For both prefixed and unprefixed versions, and
> * For animationstart, animationiteration, animationend, transitionend, and
> transitionstart (only specced in CSS Animations Level 2, but I believe IE10+
> have this):
> * Whether an event is received when the listener is registered using:
> - content attribute (on element)
> - IDL attribute on element
> - IDL attribute on Window
> - addEventListener
> * The event type in the handler
> * The event type if the event is stored and accessed later (i.e. in a
> separate task/microtask)
> 
> I think you might already have something like this?
I updated example. (http://output.jsbin.com/soqomaq)

The attachment is the result of this.
(Original sheet is http://bit.ly/2aHNmjB )

All browsers are behavior as whatwg's specification. [1]
[1] https://dom.spec.whatwg.org/#concept-event-listener-invoke

> - IDL attribute on Window
The UserAgent which is support IDL attribute of animation is Safari/Firefox only.
So I checked these browser. (using http://output.jsbin.com/lavoxak/)
These browser can get the event on Window.


(In reply to Olli Pettay [:smaug] from comment #57)
> Comment on attachment 8777661 [details]
> I don't understand why *TransitionEnd is a special here. Per
> http://trac.webkit.org/browser/trunk/Source/WebCore/dom/Element.idl#L204
> those handlers are similar, all lowercase. 
> And that is what various examples in the web seem to use too, all lowercase
> for handler.
Oh sorry, the patch of part 4 is my mistake.
We should define onwebkittransitionend as lower-case.
Flags: needinfo?(mantaroh)
(Assignee)

Comment 63

2 years ago
Comment on attachment 8775391 [details]
Bug 911987 part 1 - Add onanimation** and ontransitionend event handlers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67562/diff/5-6/
Attachment #8777661 - Flags: review- → review?(bugs)
(Assignee)

Comment 64

2 years ago
Comment on attachment 8775392 [details]
Bug 911987 part 2 - Add the onanimation** and ontransition end tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/67564/diff/6-7/
(Assignee)

Comment 65

2 years ago
Comment on attachment 8777660 [details]
Bug 911987 part 3 - Add onwebkit prefixed event handler tests.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69128/diff/1-2/
(Assignee)

Comment 66

2 years ago
Comment on attachment 8777661 [details]
Bug 911987 part 4 - Add onwebkitanimation** and onwebkittransitionend event handlers.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69130/diff/1-2/
(Reporter)

Updated

2 years ago
Attachment #8777661 - Flags: review?(bugs) → review+
(Reporter)

Comment 68

2 years ago
Comment on attachment 8777661 [details]
Bug 911987 part 4 - Add onwebkitanimation** and onwebkittransitionend event handlers.

https://reviewboard.mozilla.org/r/69130/#review66928

Comment 69

2 years ago
Pushed by mantaroh@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/42b9cd44c546
part 1 - Add onanimation** and ontransitionend event handlers. r=masayuki,smaug
https://hg.mozilla.org/integration/autoland/rev/c941f7d76a27
part 2 - Add the onanimation** and ontransition end tests. r=birtles
https://hg.mozilla.org/integration/autoland/rev/893ae2ebfe9f
part 3 - Add onwebkit prefixed event handler tests. r=birtles
https://hg.mozilla.org/integration/autoland/rev/9dc08c2240db
part 4 - Add onwebkitanimation** and onwebkittransitionend event handlers. r=smaug

Comment 70

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/42b9cd44c546
https://hg.mozilla.org/mozilla-central/rev/c941f7d76a27
https://hg.mozilla.org/mozilla-central/rev/893ae2ebfe9f
https://hg.mozilla.org/mozilla-central/rev/9dc08c2240db
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
You need to log in before you can comment on or make changes to this bug.