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
do you have a link to the blink bug
<rdar://problem/16496602>
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.
(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.
Created attachment 230697 [details] Patch
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.
(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.
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
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
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
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
(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.
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
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
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
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
Created attachment 230750 [details] Patch
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.
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
(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.
Created attachment 230841 [details] Patch
Comment on attachment 230841 [details] Patch thanks. r=me
Comment on attachment 230841 [details] Patch Clearing flags on attachment: 230841 Committed r168313: <http://trac.webkit.org/changeset/168313>
All reviewed patches have been landed. Closing bug.
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.
Please file a new bug asking for visual focus support to be added to svg elements
(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.
I filed a bug to ask that visual focus be addded: https://bugs.webkit.org/show_bug.cgi?id=132958