Bug 150393 – Support for the SVG `onend` attribute

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Antoine Quint]

While we've added support for the `endEvent` event in SVG (see https://bugs.webkit.org/show_bug.cgi?id=121587), we have not added support for the `onend` attribute which allows a JS event listener to be added declaratively for the `endEvent` event.

[Antoine Quint]

Created attachment 263680 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/23201745>

[WebKit Commit Bot]

Comment on attachment 263680 [details]
Patch

Clearing flags on attachment: 263680

Committed r191392: <http://trac.webkit.org/changeset/191392>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Said Abou-Hallawa]

Comment on attachment 263680 [details]
Patch

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

> LayoutTests/svg/animations/end-event-attribute-expected.svg:2
> +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1 Tiny//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11-tiny.dtd">

I think you do not have to include the tags <?xml> and <!DOCTYPE>.

> LayoutTests/svg/animations/end-event-attribute-expected.svg:3
> +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">

You do not need to include xmlns:xlink="http://www.w3.org/1999/xlink" since you are not using this namespace in the SVG.

> LayoutTests/svg/animations/end-event-attribute.svg:12
> +        document.getElementById("rect").setAttribute("fill", "green");

Do not you need to force layout or setTimeout() before notifyDone() to make sure the test runner captures the page in a steady state?

[Antoine Quint]

(In reply to comment #5)
> > LayoutTests/svg/animations/end-event-attribute-expected.svg:2
> > +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1 Tiny//EN" "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11-tiny.dtd">
> 
> I think you do not have to include the tags <?xml> and <!DOCTYPE>.

That's true, doesn't hurt. I might update those tests in an upcoming related patch (thinking of implementing the `beginEvent` event and `onbegin` attribute).
 
> > LayoutTests/svg/animations/end-event-attribute-expected.svg:3
> > +<svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
> 
> You do not need to include xmlns:xlink="http://www.w3.org/1999/xlink" since
> you are not using this namespace in the SVG.

True too. Will look again when adding related features.

> > LayoutTests/svg/animations/end-event-attribute.svg:12
> > +        document.getElementById("rect").setAttribute("fill", "green");
> 
> Do not you need to force layout or setTimeout() before notifyDone() to make
> sure the test runner captures the page in a steady state?

At least on my system, tests have been running reliably without the need for a forced layout.

[Darin Adler]

Comment on attachment 263680 [details]
Patch

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

> Source/WebCore/svg/animation/SVGSMILElement.cpp:485
> +    } else if (name == SVGNames::onendAttr) {
> +        setAttributeEventListener(eventNames().endEventEvent, name, value);
>      } else

This looks good, but coding style for a one liner like this is supposed to be no braces.

What is going on with the name endEventEvent? What? Is the event named "end" or is it named "endEvent"?

[Antoine Quint]

Comment on attachment 263680 [details]
Patch

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

>> Source/WebCore/svg/animation/SVGSMILElement.cpp:485
>>      } else
> 
> This looks good, but coding style for a one liner like this is supposed to be no braces.
> 
> What is going on with the name endEventEvent? What? Is the event named "end" or is it named "endEvent"?

Can't believe I missed the style compliance. Odd also that the style bot didn't catch my error. Filed https://bugs.webkit.org/show_bug.cgi?id=150440 to follow up on that.

I will be making more changes to that area very soon and I will take the time to fix this error as well if that's alright. If you'd rather, I can make a quicker fix dedicated to fixing this.

As for the event name, yes, believe it or not, it's called `endEvent` (see http://www.w3.org/TR/SVG/animate.html#InterfaceTimeEvent), but the attribute to register an `endEvent` event listener declaratively is `onend` (see http://www.w3.org/TR/SVG/script.html#AnimationEvents). Not that there is also an `end` event supported by WebKit for speech synthesis.

[Antoine Quint]

Said, Darin, I've made the changes you've suggested in the patch for https://bugs.webkit.org/show_bug.cgi?id=150442.