| Summary: | Support for the SVG `onend` attribute | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Antoine Quint <graouts> | ||||
| Component: | SVG | Assignee: | Antoine Quint <graouts> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | commit-queue, graouts, jonlee, sabouhallawa, webkit-bug-importer, zimmermann | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | Unspecified | ||||||
| OS: | Unspecified | ||||||
| Attachments: |
|
||||||
|
Description
Antoine Quint
2015-10-21 08:52:43 PDT
Created attachment 263680 [details]
Patch
Comment on attachment 263680 [details] Patch Clearing flags on attachment: 263680 Committed r191392: <http://trac.webkit.org/changeset/191392> All reviewed patches have been landed. Closing bug. 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? (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. 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"? 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. Said, Darin, I've made the changes you've suggested in the patch for https://bugs.webkit.org/show_bug.cgi?id=150442. |