Last Comment Bug 778654 - implement tabindex attribute for SVG content
: implement tabindex attribute for SVG content
Status: NEW
: dev-doc-needed
Product: Core
Classification: Components
Component: SVG (show other bugs)
: unspecified
: All All
: -- normal with 4 votes (vote)
: ---
Assigned To: Rik Cabanier
:
Mentors:
: 996407 (view as bug list)
Depends on: 409404
Blocks:
  Show dependency treegraph
 
Reported: 2012-07-30 02:04 PDT by Jeremie Patonnier :Jeremie
Modified: 2015-10-04 02:44 PDT (History)
10 users (show)
surkov.alexander: needinfo? (cabanier)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
1. Add IDL for tabindex (4.75 KB, patch)
2014-04-01 13:38 PDT, Rik Cabanier
no flags Details | Diff | Review
tabindexidl.patch (12.99 KB, patch)
2014-04-15 21:22 PDT, Rik Cabanier
no flags Details | Diff | Review
1. Add DispayOutline object to tree for SVG elements that can be focused (1.89 KB, patch)
2014-04-17 21:31 PDT, Rik Cabanier
cabanier: review? (cam)
Details | Diff | Review
2. Add IDL + support for tabindex + focus to SVG elements. Also added tests. (10.80 KB, patch)
2014-04-17 21:33 PDT, Rik Cabanier
no flags Details | Diff | Review
2. Add IDL + support for tabindex + focus to SVG elements. Also added tests. (11.84 KB, patch)
2014-04-18 21:51 PDT, Rik Cabanier
cabanier: review? (cam)
Details | Diff | Review

Description Jeremie Patonnier :Jeremie 2012-07-30 02:04:18 PDT
There is a discussion about adding the "tabindex" attribute to SVG content (see http://lists.w3.org/Archives/Public/www-svg/2012Jul/0108.html). This is a lot easier to understand (and use) for author used to HTML.

There is also the "focusable" attribute in SVG 1.2Tiny (cover by bug 409404) but it's not clear at the moment which attribute will be promote in SVG2, see http://www.w3.org/Graphics/SVG/WG/wiki/SVG2_Requirements_Input#Run-time_Synchronization_attributes

IMO, it would be good to implement both attributes : "focusable" for backward compatibility with legacy content, "tabindex" to make authors' life easier in the future when they'll mixe both technologies.
Comment 1 David Bolter [:davidb] 2013-08-14 06:36:36 PDT
Requests coming in from the wild - perhaps the time is now.

Jonathan, Neil, who would be the right person to work on this? How much effort do you think it would take?
Comment 2 Neil Deakin 2013-08-14 06:47:24 PDT
You would just need to implement nsIContent::IsFocusable on the desired elements and set the aTtabIndex argument as necessary within that implementation.
Comment 3 David Bolter [:davidb] 2013-08-14 07:10:05 PDT
OK!

Maybe I could just tweak the SVGAElement::IsFocusable code that morphs tabindex to -1...
Comment 4 David Bolter [:davidb] 2013-08-20 06:00:29 PDT
Status: I wrote a half-baked patch and then this totally fell off my radar. Note I would still like to hear from jwatt.
Comment 5 Jonathan Watt [:jwatt] 2013-08-20 06:13:38 PDT
I think we should only implement 'tabindex', at least for now. SVG Tiny has not generally been used on the public Web since of the desktop browsers only Opera implemented it I believe.

Regarding the a/b/c options in heycam's email (linked in the first paragraph of comment 0), I'd defer to heycam on that.
Comment 6 Cameron McCormack (:heycam) 2013-08-20 16:32:57 PDT
Rich went with duplicating tabIndex, focus and blur on SVGElement (option c), and I think that's fine.
Comment 7 Jonathan Watt [:jwatt] 2013-08-20 16:40:08 PDT
Okay, fine by me too. Thanks, Cameron.
Comment 8 David Bolter [:davidb] 2013-11-20 08:10:44 PST
Jonathan do you know who could pick up this work?
Comment 9 Jonathan Watt [:jwatt] 2013-11-20 08:20:36 PST
I don't think anyone is likely to be able to get to it before the end of the year. I potentially could in January, but check with my manager (Jet) regarding his priorities and how that would fit in.
Comment 10 Rich Schwerdtfeger 2014-03-06 12:56:19 PST
Jonathan, any progress on this? Tabindex in the SVG 2 specificaton: https://svgwg.org/svg2-draft/interact.html#sequential-focus-navigation-and-the-tabindex-attribute

I also added event handlers to be in line with HTML5 which started with focus:
https://svgwg.org/svg2-draft/interact.html#SVGEvents

The Document interface also includes support for activeElement:
https://svgwg.org/svg2-draft/struct.html#InterfaceSVGDocument

The SVG elements also support focus() and blur()

There is also support for onfocusin, onfocusout, onfocus, and onblur event handlers: https://svgwg.org/svg2-draft/script.html#GraphicsEvents
Comment 11 Rich Schwerdtfeger 2014-03-13 13:51:28 PDT
Erik Dalhstrom has tabindex under code review now before it drops in Canary. So, it will be in Chrome Canary soon.
Comment 12 Rich Schwerdtfeger 2014-03-31 06:28:30 PDT
Thanks Rik.
Comment 13 Rik Cabanier 2014-03-31 13:28:42 PDT
(In reply to David Bolter [:davidb] from comment #3)
> OK!
> 
> Maybe I could just tweak the SVGAElement::IsFocusable code that morphs
> tabindex to -1...

I'm experimenting with isFocusable, but that function is never called for an SVG file. 
Do you have an example that uses this functionality?
Comment 14 David Bolter [:davidb] 2014-03-31 13:54:25 PDT
(In reply to Rik Cabanier from comment #13)
> (In reply to David Bolter [:davidb] from comment #3)

> I'm experimenting with isFocusable, but that function is never called for an
> SVG file. 
> Do you have an example that uses this functionality?

Not sure if this works for you.
http://people.mozilla.org/~dbolter/svg-focus.html
Comment 15 David Bolter [:davidb] 2014-03-31 13:59:21 PDT
I had tried adding this to SVGAElement::IsFocusable:
+  if (HasAttr(kNameSpaceID_None, nsGkAtoms::tabindex)) {
+    return true;
+  }

And somewhere else... sorry I've forgotten...
Comment 16 Rik Cabanier 2014-04-01 13:38:54 PDT
Created attachment 8400172 [details] [diff] [review]
1. Add IDL for tabindex
Comment 17 David Bolter [:davidb] 2014-04-15 12:18:59 PDT
Friendly 2 week review nudge for Cameron.
Comment 18 Rik Cabanier 2014-04-15 21:22:30 PDT
Created attachment 8407288 [details] [diff] [review]
tabindexidl.patch
Comment 19 David Bolter [:davidb] 2014-04-16 07:27:04 PDT
*** Bug 996407 has been marked as a duplicate of this bug. ***
Comment 20 Cameron McCormack (:heycam) 2014-04-17 01:17:45 PDT
Comment on attachment 8407288 [details] [diff] [review]
tabindexidl.patch

Review of attachment 8407288 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry for the delay.  Some high level comments to start with.

I think the patch needs to be split up into two pieces: one implements 'outline' for SVG elements, the other does the focus related things.

::: content/svg/content/src/nsSVGElement.cpp
@@ +1145,5 @@
>    return mClassAttribute.ToDOMAnimatedString(this);
>  }
>  
> +Nullable<int32_t>
> +nsSVGElement::GetTabIndex()

Is there a way we can share code between these four methods you've added here and the ones that exist on nsGenericHTMLElement?  They basically should have the same behaviour.

::: content/svg/content/test/reftest/reftest.list
@@ +1,3 @@
> +# SVG reftests
> +
> +pref(svg.tabindex.enabled,true) == tabindex.html tabindex-ref.html
\ No newline at end of file

Reftests should go in layout/svg/reftests/.

::: dom/webidl/SVGElement.webidl
@@ +26,5 @@
>  
>    readonly attribute SVGSVGElement? ownerSVGElement;
>    readonly attribute SVGElement? viewportElement;
> +  
> +  [Pref="svg.tabindex.enabled"] attribute long? tabIndex;

Why is this nullable?  That doesn't match what's in the spec.

::: layout/svg/svg.css
@@ +85,5 @@
> +polyline:-moz-focusring,
> +rect:-moz-focusring,
> +text:-moz-focusring,
> +use:-moz-focusring,
> +image:-moz-focusring

I think this should be something like:

  :-moz-focusring:not(:root)

so that all SVG elements that would render can have an outline, except for the root element.
Comment 21 Rik Cabanier 2014-04-17 21:23:35 PDT
Comment on attachment 8407288 [details] [diff] [review]
tabindexidl.patch

Review of attachment 8407288 [details] [diff] [review]:
-----------------------------------------------------------------

::: content/svg/content/src/nsSVGElement.cpp
@@ +1145,5 @@
>    return mClassAttribute.ToDOMAnimatedString(this);
>  }
>  
> +Nullable<int32_t>
> +nsSVGElement::GetTabIndex()

The way to do that would be to put the functionality in their base class but it might not be the correct place for it and I'm wary to make such a big change.

::: content/svg/content/test/reftest/reftest.list
@@ +1,3 @@
> +# SVG reftests
> +
> +pref(svg.tabindex.enabled,true) == tabindex.html tabindex-ref.html
\ No newline at end of file

Fixed

::: dom/webidl/SVGElement.webidl
@@ +26,5 @@
>  
>    readonly attribute SVGSVGElement? ownerSVGElement;
>    readonly attribute SVGElement? viewportElement;
> +  
> +  [Pref="svg.tabindex.enabled"] attribute long? tabIndex;

I don't remember why I did this.
Fixed

::: layout/svg/svg.css
@@ +85,5 @@
> +polyline:-moz-focusring,
> +rect:-moz-focusring,
> +text:-moz-focusring,
> +use:-moz-focusring,
> +image:-moz-focusring

wouldn't that include items such as <symbol> and <gradient>?

I thought that we were going to call these elements out from the spec. Maybe we can pick up the email threat again?
Comment 22 Rik Cabanier 2014-04-17 21:31:49 PDT
Created attachment 8408763 [details] [diff] [review]
1. Add DispayOutline object to tree for SVG elements that can be focused
Comment 23 Rik Cabanier 2014-04-17 21:33:02 PDT
Created attachment 8408764 [details] [diff] [review]
2. Add IDL + support for tabindex + focus to SVG elements. Also added tests.
Comment 24 Rik Cabanier 2014-04-18 21:51:01 PDT
Created attachment 8409277 [details] [diff] [review]
2. Add IDL + support for tabindex + focus to SVG elements. Also added tests.
Comment 25 Rik Cabanier 2014-04-18 21:52:51 PDT
try run: https://tbpl.mozilla.org/?tree=Try&rev=f0bbb14ecd65
Comment 26 Robert Longson 2014-05-01 01:01:43 PDT
Please move the code to a common base class so we don't have two copies.
Comment 27 Rich Schwerdtfeger 2014-06-13 06:44:39 PDT
Is this code in the nightly builds now?
Comment 28 alexander :surkov 2014-06-13 07:17:15 PDT
(In reply to Rich Schwerdtfeger from comment #27)
> Is this code in the nightly builds now?

not yet, it's still requires review. Maybe somebody else should do it because the current reviewer doesn't look active.
Comment 29 Robert Longson 2014-06-13 07:33:18 PDT
My review comment not to create two copies of the code still needs addressing. If that's done Cam or I will rereview.
Comment 30 David Bolter [:davidb] 2014-07-08 12:57:15 PDT
Rick is comment 29 something you agree with and can address?
Comment 31 Rik Cabanier 2014-07-15 07:57:56 PDT
(In reply to David Bolter [:davidb] from comment #30)
> Rick is comment 29 something you agree with and can address?

I will try to get to it this week. At the time I thought it might be overkill to do it this way but I will investigate.
Comment 32 duane.obrien 2014-08-11 11:22:41 PDT
It looks like this one's pretty close to resolution. Is there anything we could do to help it move forward?
Comment 33 Rik Cabanier 2014-08-11 12:27:23 PDT
(In reply to duane.obrien from comment #32)
> It looks like this one's pretty close to resolution. Is there anything we
> could do to help it move forward?

I would like to finish it but keep getting pulled into other things :-(
Would you have time to get it done?
Comment 34 duane.obrien 2014-08-11 12:33:33 PDT
It's not something I could finish unfortunately, I'm just tracking the bug against another project that's waiting for it.
Comment 35 duane.obrien 2014-09-11 15:20:15 PDT
Are we any closer to resolution on this one?
Comment 36 Rich Schwerdtfeger 2014-12-11 15:39:33 PST
This is part of the SVG2 specification. It needs to be implemented. It is mostly there in Webkit and Chrome.
Comment 37 Rich Schwerdtfeger 2014-12-11 15:41:09 PST
I think you should only implement tabindex for now. There is an effort that has started called the SVG Accessibility Task Force and we will at some point consider keyboard navigation mechanisms.
Comment 38 alexander :surkov 2014-12-12 07:03:06 PST
Rik, do you have status update on this bug? Do you still need review/needinfo from cam?
Comment 39 duane.obrien 2015-01-27 14:15:00 PST
Any new information on this one?

Note You need to log in before you can comment on or make changes to this bug.


Privacy Policy