Created attachment 13376 [details] animateColor example

not sure which parts of declarative animation are supported, hence the animateColor testcase

Created attachment 13377 [details] animateColor example

event handling is supported, the attachment demonstrates, a time delay, a mouseover and a tab event

Created attachment 95269 [details] Patch

Comment on attachment 95269 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95269&action=review > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:66 > + repaintRect.inflate(object->style()->outlineWidth()); Is that covered by an existing test? If not, you need to add a new test (pixel test?). > Source/WebCore/svg/SVGCircleElement.h:45 > + virtual bool supportsFocus() const { return true; } I wonder if this should go to SVGStyledTransformable. Since most childs of this class seem to have it enabled. > Source/WebCore/svg/SVGTextElement.h:47 > + virtual bool supportsFocus() const { return true; } Do tspans need this as well?

Another comment. Can you add a test with keyboard inputs? would be great.

Created attachment 95282 [details] previous testcase buggy ~ 4 years old :)

Dirk, 'tab' key should now animate onfocus circle (Opera wfm) ie whatever key is used to cycle through links, usually the tabkey Is it clear that this bug relates to the general case, and not animateColor & focusin in particular? http://www.w3.org/TR/SVG/interact.html#SVGEvents

Hi Jay, (In reply to comment #9) > Dirk, > > 'tab' key should now animate onfocus circle (Opera wfm) > > ie whatever key is used to cycle through links, usually the tabkey > > Is it clear that this bug relates to the general case, and not animateColor & focusin in particular? http://www.w3.org/TR/SVG/interact.html#SVGEvents Yes, my patch is also directed at bug 40545, which is more general. Cheers, Rob.

Hi Dirk, (In reply to comment #6) > (From update of attachment 95269 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95269&action=review > > > Source/WebCore/rendering/svg/SVGRenderSupport.cpp:66 > > + repaintRect.inflate(object->style()->outlineWidth()); > > Is that covered by an existing test? If not, you need to add a new test (pixel test?). Not really, still thinking about it. > > Source/WebCore/svg/SVGCircleElement.h:45 > > + virtual bool supportsFocus() const { return true; } > > I wonder if this should go to SVGStyledTransformable. Since most childs of this class seem to have it enabled. But for instance clippath is SVGStyledTransformable but should not have supportsFocus true. > > Source/WebCore/svg/SVGTextElement.h:47 > > + virtual bool supportsFocus() const { return true; } > > Do tspans need this as well? I don't think so since the code looks at parents for isFocusable(), and so ends up in <text>. Cheers, Rob.

Created attachment 95291 [details] Patch

(In reply to comment #12) > Created an attachment (id=95291) [details] > Patch I included the keyboard focus test as well. Not sure how to test the focus ring change? Cheers, Rob.

Comment on attachment 95291 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=95291&action=review Looks great, I wonder whether this is covered by a SVG 1.1 Second Edition test as well, that you'd want to import here? Some questions remain, can you explain them: > Source/WebCore/svg/SVGDefsElement.h:44 > + virtual bool supportsFocus() const { return true; } <defs> support focus? How? It's renderer is "hidden". > Source/WebCore/svg/SVGGElement.h:49 > + virtual bool supportsFocus() const { return true; } Ditto? How can you ever focus a "group"?

Hi Niko, Oops, I managed to completely miss this :( Not sure if I overlooked it in my inbox or never got the mail :) (In reply to comment #14) > (From update of attachment 95291 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=95291&action=review > Looks great, I wonder whether this is covered by a SVG 1.1 Second Edition test as well, that you'd want to import here? Could well be, Dirk has a spreadsheet for tracking how we do on SVG1.1SE tests. It may be in script-handle-02-b.svg for instance. > Some questions remain, can you explain them: > > Source/WebCore/svg/SVGDefsElement.h:44 > > + virtual bool supportsFocus() const { return true; } > <defs> support focus? How? It's renderer is "hidden". > > > Source/WebCore/svg/SVGGElement.h:49 > > + virtual bool supportsFocus() const { return true; } > Ditto? How can you ever focus a "group"? I took both according to the letter of the spec: http://www.w3.org/TR/SVG/intro.html#TermGraphicalEventAttribute Both <g> and <defs> have onfocus listed. Now the question is, does supportFocus() determine the visual focus, whether the focus event can be accepted/dispatched or both... With these focus methods it is a bit hard to see how they work together IMHO. Anyway, a <defs> shouldn't have visual focus for sure, it is not even displayed! I'll have a look whether Opera supports visual focus on <g>. Glad you like it! IMHO this is nice stuff to have and would fix some testcases, so hopefully we can land it this week! Cheers, Rob.

(In reply to comment #15) > > <defs> support focus? How? It's renderer is "hidden". > > > > > Source/WebCore/svg/SVGGElement.h:49 > > > + virtual bool supportsFocus() const { return true; } > > Ditto? How can you ever focus a "group"? > > I took both according to the letter of the spec: > > http://www.w3.org/TR/SVG/intro.html#TermGraphicalEventAttribute > > Both <g> and <defs> have onfocus listed. Now the question is, does supportFocus() determine the visual focus, whether the focus event can be accepted/dispatched or both... With these focus methods it is a bit hard to see how they work together IMHO. Anyway, a <defs> shouldn't have visual focus for sure, it is not even displayed! I'll have a look whether Opera supports visual focus on <g>. After looking into this further, the <g> seems reasonable to support as being focusable, like in Opera, it has a boundingbox after all and gets delivered focus events. I have to investigate <defs> further, but I doubt it makes much sense. It seems we also need tests(s) to track which elements get delivered focus events and which not... Cheers, Rob.

Created attachment 96764 [details] Patch

(In reply to comment #17) > Created an attachment (id=96764) [details] > Patch This patch has improved tests. Like I though <defs> makes no sense since no visual representation, so I left it out Cheers, Rob.

Comment on attachment 96764 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=96764&action=review Looks good to me, r=me!. > Source/WebCore/svg/SVGStyledElement.cpp:515 > + Element* eventTarget = const_cast<SVGStyledElement *>(this); > + return eventTarget->hasEventListeners(eventNames().focusinEvent) || eventTarget->hasEventListeners(eventNames().focusoutEvent); A pity _has_EventListeners is non-const, anyhow remove the whitespace between the star and Element in the const_cast please.

Committed r88555: <http://trac.webkit.org/changeset/88555>

GTK & Qt rebaselined landed in http://trac.webkit.org/changeset/88596.

when is this landing in trunk? not surprisingly latest nightly Version 5.0.5 (5533.21.1, r88541) is not fixed afaict