Closed Bug 911987 Opened 11 years ago Closed 8 years ago

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

Categories

(Core :: DOM: Core & HTML, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: smaug, Assigned: mantaroh)

References

Details

(Keywords: dev-doc-complete)

Attachments

(5 files)

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)
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)
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)
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)
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)
So we probably should add them too then (both prefixed and unprefixed).
Flags: needinfo?(dholbert)
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)
Hi chiko.
Will you have time to work on this bug?
(I'd like to steal this bug.)
Status: NEW → ASSIGNED
Flags: needinfo?(chikoski)
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.
Assignee: chikoski → nobody
Status: ASSIGNED → NEW
https://treeherder.mozilla.org/#/jobs?repo=try&revision=841a62ba37cc6e7c4a3c1074f2751c18e5521a19
Assignee: nobody → mantaroh
Status: NEW → ASSIGNED
Flags: needinfo?(chikoski)
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
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+
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.
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+
Attachment #8775392 - Flags: review?(bbirtles)
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)
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)
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)
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/
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)
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)
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/
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)
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/
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/
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/
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?
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() });
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-
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...?)
Blocks: 1170774
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)
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)
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)
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/
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/
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/
Attachment #8777661 - Flags: review?(bugs) → review+
Comment on attachment 8777661 [details]
Bug 911987 part 4 - Add onwebkitanimation** and onwebkittransitionend event handlers.

https://reviewboard.mozilla.org/r/69130/#review66928
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
Component: DOM → DOM: Core & HTML

I'm a little confused by the changes in this bug. As far as I can tell, no other browser implements these "onowebkitanimation*" event handlers, including Chrome and Safari. Am I just missing something?

Flags: needinfo?(bugs)

I don't think so. Just reading the bug again, the situation here has been rather messy and the patches made Gecko at least somewhat consistent.
But worth to check again what other browsers support.

hiro, perhaps you know the current state?

Flags: needinfo?(bugs) → needinfo?(hikezoe.birchill)

I did test the link in comment 20 on both Safari 12.1.12 and Chromium 78.0.3888.0. To me both browsers still support onwebkitanimation* event handlers.

Hmm, bz is on PTO, I will ask him about the difference between the site in comment 20 and how he checked once after he come back to work.

Flags: needinfo?(hikezoe.birchill)

According to comment 26 there was also some discrepancy between support for IDL attributes and content attributes.

The testcase I was looking at is something like this:

<!doctype html>
<pre><script>
  var d = document;
  document.writeln("onwebkittransitionend" in d);
  document.writeln("onwebkitanimationstart" in d);
  document.writeln("onwebkitanimationiteration" in d);
  document.writeln("onwebkitanimationend" in d);
</script>

The results I see are:

  • Firefox: all true
  • Safari: all false
  • Chrome: all false

It looks like Safari has these properties on Element and Window (but not documents).

Firefox has them on Document and Window and HTMLElement, SVGElement, XULElement, but not other elements.

Chrome only has them on Window only.

Sorry for my blanket "Safari and Chrome don't support them" claim... The situation is more complicated than I thought, for sure. That said, the fact that we introduced a third behavior which matches neither Chrome nor Safari is a little unfortunate. And even if we did want to aim for "expose them consistently everywhere" as a reason to not match Safari, we're not exposing them everywhere Safari does...

(That said, I don't know whether Safari supports the corresponding content attributes on all elements.)

Flags: needinfo?(hikezoe.birchill)
Depends on: 1578121

Thanks for the clarification! Now I see what you meant.

I think we did innocently add them there since there are onanimationXX event handlers. To me it makes most sense to match onwebkitXX event handlers WebKit implementation because of the prefix. I filed bug 1578121 for that and also filed a chromium bug.

Flags: needinfo?(hikezoe.birchill)
You need to log in before you can comment on or make changes to this bug.