Bug 200997 – SVG element should not become focusable when focus and key event listeners are added

• Status: RESOLVED
• Resolution: FIXED
• Component: SVG

[Ryosuke Niwa]

SVGElement::isMouseFocusable() has bizarre code which checks the presence of keydown, keyup, keypress, focusin, focusout, focus, and blur event listeners.

We shouldn't make an element focusable just because it has a bunch of event listeners.

[Ryosuke Niwa]

Created attachment 376945 [details]
Patch

[Said Abou-Hallawa]

Comment on attachment 376945 [details]
Patch

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

> Source/WebCore/svg/SVGAElement.cpp:162
>      if (hasEditableStyle())
>          return SVGGraphicsElement::supportsFocus();

How can this element be editable?

> LayoutTests/svg/custom/tabindex-order.html:66
> +    <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">

I think you do not have to add these namespaces in html. The xlink namespaces is removed in SVG2 beside it is not used in this test.

> LayoutTests/svg/custom/tabindex-order.html:68
> +        <rect class="tab" id="rect without tabindex is not focusable" width="1" height="1"/>

I am not sure why the id's contains white spaces. I see they are not referenced but they look weird to me. You may use comment element <!-- --> instead.

[Ryosuke Niwa]

(In reply to Said Abou-Hallawa from comment #2)
> Comment on attachment 376945 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=376945&action=review
> 
> > Source/WebCore/svg/SVGAElement.cpp:162
> >      if (hasEditableStyle())
> >          return SVGGraphicsElement::supportsFocus();
> 
> How can this element be editable?

You can put contenteditble attribute anywhere, or put the entire document into design mode by document.designMode = 'on'.

> > LayoutTests/svg/custom/tabindex-order.html:66
> > +    <svg xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink">
> 
> I think you do not have to add these namespaces in html. The xlink
> namespaces is removed in SVG2 beside it is not used in this test.

Yeah, I added for some testing. Removed.

> > LayoutTests/svg/custom/tabindex-order.html:68
> > +        <rect class="tab" id="rect without tabindex is not focusable" width="1" height="1"/>
> 
> I am not sure why the id's contains white spaces. I see they are not
> referenced but they look weird to me. You may use comment element <!-- -->
> instead.

Well, these IDs are like that because if they ever get focused, it would show up in the test result. The rest of the test uses that convention so I'd keep that for now.

[Ryosuke Niwa]

Thanks for the review!

[Ryosuke Niwa]

Committed r248983: <https://trac.webkit.org/changeset/248983>

[Radar WebKit Bug Importer]

<rdar://problem/54579978>

[Ryosuke Niwa]

*** Bug 140024 has been marked as a duplicate of this bug. ***