Bug 150442 – Support for SVG `beginEvent` event and `onbegin` attribute

• Component: SVG
• Status: RESOLVED FIXED

[Antoine Quint]

We've added support for the SVG `endEvent` event (see https://bugs.webkit.org/show_bug.cgi?id=150393) and the `onend` attribute (see https://bugs.webkit.org/show_bug.cgi?id=121587). We should also implement the matching event and attribute for when an SVG timing element begins.

[Radar WebKit Bug Importer]

<rdar://problem/23216574>

[Antoine Quint]

Created attachment 263815 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 263815 [details]
Patch

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

> Source/WebCore/ChangeLog:17
> +               svg/animations/begin-event-syncbase.svg

I think the expected files should not be included in this section of the ChangeLog. It might be a bug in prepare-ChangeLog tool.

> LayoutTests/svg/animations/begin-event-attribute.svg:3
> +    <animate id="anim" attributeName="visibility" to="visible" begin="1s" end="2s" onbegin="handleBeginEvent()" />

What is the point in starting at "1s" and calling document.documentElement.setCurrentTime(1) when the document is loaded? Why do not we start at "0s"?

> LayoutTests/svg/animations/begin-event-script.svg:3
> +    <animate id="anim" attributeName="visibility" to="visible" begin="1s" end="2s" />

Ditto.

> LayoutTests/svg/animations/begin-event-syncbase.svg:3
> +    <animate id="anim" attributeName="visibility" to="visible" begin="1s" end="2s" />

Ditto.

[Antoine Quint]

(In reply to comment #3)
> Comment on attachment 263815 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=263815&action=review
> 
> > Source/WebCore/ChangeLog:17
> > +               svg/animations/begin-event-syncbase.svg
> 
> I think the expected files should not be included in this section of the
> ChangeLog. It might be a bug in prepare-ChangeLog tool.

Possible, I ran `webkit-patch upload` and just added my comments. Would you like me to manually remove the `-expected` files?

> > LayoutTests/svg/animations/begin-event-attribute.svg:3
> > +    <animate id="anim" attributeName="visibility" to="visible" begin="1s" end="2s" onbegin="handleBeginEvent()" />
> 
> What is the point in starting at "1s" and calling
> document.documentElement.setCurrentTime(1) when the document is loaded? Why
> do not we start at "0s"?

I did this so it was more explicit that we were seeking to the begin time rather than running script at the same time the `beginEvent` event may have been triggered and risking the possibility of missing the event. This may be ill-founded and if you like I could remove the explicit begin and call to `setCurrentTime()` to just wait for the document timeline to start post-load.

[Dean Jackson]

Comment on attachment 263815 [details]
Patch

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

>>> Source/WebCore/ChangeLog:17
>>> +               svg/animations/begin-event-syncbase.svg
>> 
>> I think the expected files should not be included in this section of the ChangeLog. It might be a bug in prepare-ChangeLog tool.
> 
> Possible, I ran `webkit-patch upload` and just added my comments. Would you like me to manually remove the `-expected` files?

Yes please.

[Antoine Quint]

Created attachment 263912 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 263912 [details]
Patch for landing

Clearing flags on attachment: 263912

Committed r191494: <http://trac.webkit.org/changeset/191494>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.