Bug 130212 – AX: tabindex support in SVG2

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[James Craig]

Created attachment 226626 [details]
test case

AX: tabindex support in SVG2
https://svgwg.org/svg2-draft/single-page.html#interact-Focus

"Erik Dahlstrom stated [on the SVG WG call] that tabindex support is now under code review for Blink and it should show up in Chrome Canary soon."

Mozilla has an open bug, too:
https://bugzilla.mozilla.org/show_bug.cgi?id=778654

[chris fleizach]

do you have a link to the blink bug

[Radar WebKit Bug Importer]

<rdar://problem/16496602>

[James Craig]

FWIW, tabindex works to some degree. If you include a tabindex and navigate with VoiceOver, focus is pulled along to the element with tabindex, and the focus ring shows up. It may just be that the tab *key* doesn't work in SVG, or traverse into embedded SVG images. WebKit has some similar issues with getting the tab key to traverse into shadow DOM content. See related bug 117857.

[Dirk Schulze]

(In reply to comment #3)
> FWIW, tabindex works to some degree. If you include a tabindex and navigate with VoiceOver, focus is pulled along to the element with tabindex, and the focus ring shows up. It may just be that the tab *key* doesn't work in SVG, or traverse into embedded SVG images. WebKit has some similar issues with getting the tab key to traverse into shadow DOM content. See related bug 117857.

The Blink CL is https://codereview.chromium.org/166163005/ I planned to back port the patch if possible.

A question to "or traverse into embedded SVG images". What would you expect? I assume you mean inline SVG in HTML. Because SVG images (<img>, CSS Images) should not trigger tab index.

[Dirk Schulze]

Created attachment 230697 [details]
Patch

[WebKit Commit Bot]

Attachment 230697 [details] did not pass style-queue:


ERROR: Source/WebCore/svg/SVGAElement.cpp:231:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Dirk Schulze]

(In reply to comment #6)
> Attachment 230697 [details] did not pass style-queue:
> 
> 
> ERROR: Source/WebCore/svg/SVGAElement.cpp:231:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Total errors found: 1 in 15 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Missing space, fixed locally.

[Build Bot]

Comment on attachment 230697 [details]
Patch

Attachment 230697 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6233001847422976

New failing tests:
accessibility/svg-group-element-with-title.html
svg/custom/focus-event-handling-keyboard.xhtml
svg/custom/focus-event-handling.xhtml
svg/custom/linking-uri-01-b.svg

[Build Bot]

Created attachment 230712 [details]
Archive of layout-test-results from webkit-ews-02 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-02  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 230697 [details]
Patch

Attachment 230697 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6739979787042816

New failing tests:
accessibility/svg-group-element-with-title.html
svg/custom/focus-event-handling-keyboard.xhtml
svg/custom/focus-event-handling.xhtml
svg/custom/linking-uri-01-b.svg

[Build Bot]

Created attachment 230721 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[James Craig]

(In reply to comment #6)
> Attachment 230697 [details] did not pass style-queue:
> 
> ERROR: Source/WebCore/svg/SVGAElement.cpp:231:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Total errors found: 1 in 15 files

FYI, you can pass a --style flag to svn-create-patch and prepare-Changelog now. I forgot to run check-webkit-style frequently enough that I set these up to use the --style param by default in my bash profile.

[Build Bot]

Comment on attachment 230697 [details]
Patch

Attachment 230697 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5318208173113344

New failing tests:
accessibility/svg-group-element-with-title.html
svg/custom/focus-event-handling-keyboard.xhtml
svg/custom/focus-event-handling.xhtml
svg/custom/linking-uri-01-b.svg

[Build Bot]

Created attachment 230722 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 230697 [details]
Patch

Attachment 230697 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6081275953676288

New failing tests:
accessibility/svg-group-element-with-title.html
svg/custom/focus-event-handling-keyboard.xhtml
svg/custom/focus-event-handling.xhtml
svg/custom/linking-uri-01-b.svg

[Build Bot]

Created attachment 230729 [details]
Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-16  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[Dirk Schulze]

Created attachment 230750 [details]
Patch

[WebKit Commit Bot]

Attachment 230750 [details] did not pass style-queue:


ERROR: LayoutTests/ChangeLog:15:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: LayoutTests/ChangeLog:16:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
ERROR: LayoutTests/ChangeLog:17:  Need whitespace between colon and description  [changelog/filechangedescriptionwhitespace] [5]
Total errors found: 3 in 15 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[chris fleizach]

Comment on attachment 230750 [details]
Patch

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

just a few comments. thanks

> LayoutTests/accessibility/svg-group-element-with-title.html:9
> +  <g id="group1" role="group">

why is this change necessary?

> Source/WebCore/svg/SVGAElement.cpp:198
> +    // If not a link we should still be able to focus the element if it has tabIndex.

if it has "a" tab index

> Source/WebCore/svg/SVGElement.cpp:1158
> +    return eventTarget->hasEventListeners(eventNames().focusinEvent)

is there a reason you need to cast this to eventTarget? It seems like you could just call hasEventListeners() directly

> Source/WebCore/svg/SVGElement.cpp:1169
> +    return hasFocusEventListeners()

is there a reason you need to cast this to eventTarget? It seems like you could just call hasEventListeners() directly

> Source/WebCore/svg/SVGElement.h:145
> +    virtual short tabIndex() const override;

looks like this can probably be private

[Dirk Schulze]

(In reply to comment #19)
> (From update of attachment 230750 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=230750&action=review
> 
> just a few comments. thanks
> 
> > LayoutTests/accessibility/svg-group-element-with-title.html:9
> > +  <g id="group1" role="group">
> 
> why is this change necessary?

<g> is not a focusable element by default anymore and should't be according to SVG2. Therefore, it needs ARIA or something else to make it an AX element.

> 
> > Source/WebCore/svg/SVGAElement.cpp:198
> > +    // If not a link we should still be able to focus the element if it has tabIndex.
> 
> if it has "a" tab index

Fixed.

> 
> > Source/WebCore/svg/SVGElement.cpp:1158
> > +    return eventTarget->hasEventListeners(eventNames().focusinEvent)
> 
> is there a reason you need to cast this to eventTarget? It seems like you could just call hasEventListeners() directly

hasEventListeners is not a const function. A const cast is needed.

> 
> > Source/WebCore/svg/SVGElement.cpp:1169
> > +    return hasFocusEventListeners()
> 
> is there a reason you need to cast this to eventTarget? It seems like you could just call hasEventListeners() directly
> 
> > Source/WebCore/svg/SVGElement.h:145
> > +    virtual short tabIndex() const override;
> 
> looks like this can probably be private

tabIndex is part of SVGElement.idl and therefore it need to be public for JSSVGElement.

[Dirk Schulze]

Created attachment 230841 [details]
Patch

[chris fleizach]

Comment on attachment 230841 [details]
Patch

thanks. r=me

[WebKit Commit Bot]

Comment on attachment 230841 [details]
Patch

Clearing flags on attachment: 230841

Committed r168313: <http://trac.webkit.org/changeset/168313>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Rich Schwerdtfeger]

I recommend reopening the bug. When I have the setting turned on in webkit to show the visual focus for the elements that receive focus on tabbing the focus ring is not rendered. The visual focus only renders when VoiceOver is running.

[chris fleizach]

Please file a new bug asking for visual focus support to be added to svg elements

[Dirk Schulze]

(In reply to comment #25)
> I recommend reopening the bug. When I have the setting turned on in webkit to show the visual focus for the elements that receive focus on tabbing the focus ring is not rendered. The visual focus only renders when VoiceOver is running.

Yes, please open another bug report. You either tested with Safari 7 or you run into another bug where text did not get a focus ring. This was fixed recently too.

[Rich Schwerdtfeger]

I filed a bug to ask that visual focus be addded: https://bugs.webkit.org/show_bug.cgi?id=132958