animation event handling broken: focusin

• Component: SVG
• Status: RESOLVED FIXED

[jay]

focusin is one of the events supported by SVG1.1
however the webkit implementation isn't working.

[jay]

Created attachment 13376 [details]
animateColor example

[jay]

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

[jay]

Created attachment 13377 [details]
animateColor example

[jay]

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

[Rob Buis]

Created attachment 95269 [details]
Patch

[Dirk Schulze]

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?

[Dirk Schulze]

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

[jay]

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

[jay]

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

[Rob Buis]

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.

[Rob Buis]

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.

[Rob Buis]

Created attachment 95291 [details]
Patch

[Rob Buis]

(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.

[Nikolas Zimmermann]

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"?

[Rob Buis]

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.

[Rob Buis]

(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.

[Rob Buis]

Created attachment 96764 [details]
Patch

[Rob Buis]

(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.

[Nikolas Zimmermann]

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.

[Rob Buis]

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

[Ryosuke Niwa]

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

[jay]

when is this landing in trunk?

not surprisingly latest nightly Version 5.0.5 (5533.21.1, r88541)
is not fixed afaict