Bug 12894 - animation event handling broken: focusin
Summary: animation event handling broken: focusin
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 523.x (Safari 3)
Hardware: Mac OS X 10.4
: P2 Normal
Assignee: Rob Buis
URL:
Keywords: HasReduction
Depends on:
Blocks: 41761
  Show dependency treegraph
 
Reported: 2007-02-26 04:41 PST by jay
Modified: 2011-06-10 22:43 PDT (History)
5 users (show)

See Also:


Attachments
animateColor example (1008 bytes, image/svg+xml)
2007-02-26 04:42 PST, jay
no flags Details
animateColor example (1.02 KB, image/svg+xml)
2007-02-26 05:03 PST, jay
no flags Details
Patch (19.63 KB, patch)
2011-05-28 13:19 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
previous testcase buggy ~ 4 years old :) (943 bytes, image/svg+xml)
2011-05-29 01:09 PDT, jay
no flags Details
Patch (23.33 KB, patch)
2011-05-29 12:45 PDT, Rob Buis
no flags Details | Formatted Diff | Diff
Patch (28.07 KB, patch)
2011-06-10 11:20 PDT, Rob Buis
zimmermann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description jay 2007-02-26 04:41:07 PST
focusin is one of the events supported by SVG1.1
however the webkit implementation isn't working.
Comment 1 jay 2007-02-26 04:42:12 PST
Created attachment 13376 [details]
animateColor example
Comment 2 jay 2007-02-26 04:43:11 PST
not sure which parts of declarative animation are supported, hence the animateColor testcase
Comment 3 jay 2007-02-26 05:03:28 PST
Created attachment 13377 [details]
animateColor example
Comment 4 jay 2007-02-26 05:13:11 PST
event handling is supported, the attachment demonstrates, a time delay, a mouseover and a tab event
Comment 5 Rob Buis 2011-05-28 13:19:22 PDT
Created attachment 95269 [details]
Patch
Comment 6 Dirk Schulze 2011-05-28 13:33:40 PDT
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?
Comment 7 Dirk Schulze 2011-05-28 13:37:08 PDT
Another comment. Can you add a test with keyboard inputs? would be great.
Comment 8 jay 2011-05-29 01:09:17 PDT
Created attachment 95282 [details]
previous testcase buggy ~ 4 years old :)
Comment 9 jay 2011-05-29 01:17:02 PDT
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
Comment 10 Rob Buis 2011-05-29 10:19:04 PDT
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.
Comment 11 Rob Buis 2011-05-29 10:54:16 PDT
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.
Comment 12 Rob Buis 2011-05-29 12:45:01 PDT
Created attachment 95291 [details]
Patch
Comment 13 Rob Buis 2011-05-29 12:45:51 PDT
(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 14 Nikolas Zimmermann 2011-06-01 09:04:28 PDT
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"?
Comment 15 Rob Buis 2011-06-07 15:28:37 PDT
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.
Comment 16 Rob Buis 2011-06-08 08:09:54 PDT
(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.
Comment 17 Rob Buis 2011-06-10 11:20:55 PDT
Created attachment 96764 [details]
Patch
Comment 18 Rob Buis 2011-06-10 11:57:16 PDT
(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 19 Nikolas Zimmermann 2011-06-10 11:58:39 PDT
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.
Comment 20 Rob Buis 2011-06-10 12:43:54 PDT
Committed r88555: <http://trac.webkit.org/changeset/88555>
Comment 21 Ryosuke Niwa 2011-06-10 22:18:01 PDT
GTK & Qt rebaselined landed in http://trac.webkit.org/changeset/88596.
Comment 22 jay 2011-06-10 22:43:35 PDT
when is this landing in trunk?

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