Closed Bug 778654 Opened 12 years ago Closed 8 years ago

Implement 'tabIndex' attribute for SVG elements

Categories

(Core :: SVG, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox51 --- fixed

People

(Reporter: Jeremie, Assigned: daoshengmu)

References

(Depends on 3 open bugs, Blocks 3 open bugs, Regressed 1 open bug, )

Details

(Keywords: dev-doc-complete, Whiteboard: [parity-webkit][parity-blink])

Attachments

(6 files, 6 obsolete files)

8.04 KB, patch
longsonr
: feedback+
Details | Diff | Splinter Review
58 bytes, text/x-review-board-request
heycam
: review+
peterv
: review+
Details
58 bytes, text/x-review-board-request
heycam
: review+
Details
58 bytes, text/x-review-board-request
heycam
: review+
Details
58 bytes, text/x-review-board-request
heycam
: review+
Details
393 bytes, text/html
Details
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.
Depends on: 409404
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?
Flags: needinfo?(jwatt)
Flags: needinfo?(enndeakin)
You would just need to implement nsIContent::IsFocusable on the desired elements and set the aTtabIndex argument as necessary within that implementation.
Flags: needinfo?(enndeakin)
OK!

Maybe I could just tweak the SVGAElement::IsFocusable code that morphs tabindex to -1...
Status: I wrote a half-baked patch and then this totally fell off my radar. Note I would still like to hear from jwatt.
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.
Flags: needinfo?(jwatt) → needinfo?(cam)
Rich went with duplicating tabIndex, focus and blur on SVGElement (option c), and I think that's fine.
Flags: needinfo?(cam)
Okay, fine by me too. Thanks, Cameron.
Jonathan do you know who could pick up this work?
Flags: needinfo?(jwatt)
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.
Flags: needinfo?(jwatt)
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
Erik Dalhstrom has tabindex under code review now before it drops in Canary. So, it will be in Chrome Canary soon.
Assignee: nobody → cabanier
Thanks Rik.
(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?
(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
I had tried adding this to SVGAElement::IsFocusable:
+  if (HasAttr(kNameSpaceID_None, nsGkAtoms::tabindex)) {
+    return true;
+  }

And somewhere else... sorry I've forgotten...
Attached patch 1. Add IDL for tabindex (obsolete) — Splinter Review
Attachment #8400172 - Flags: review?(cam)
Friendly 2 week review nudge for Cameron.
Flags: needinfo?(cam)
Attached patch tabindexidl.patch (obsolete) — Splinter Review
Attachment #8400172 - Attachment is obsolete: true
Attachment #8400172 - Flags: review?(cam)
Attachment #8407288 - Flags: review?(cam)
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.
Attachment #8407288 - Flags: review?(cam)
Flags: needinfo?(cam)
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?
Attachment #8407288 - Attachment is obsolete: true
Attachment #8408763 - Flags: review?(cam)
Attachment #8408763 - Attachment description: 1. Add DispayOutline obejct to tree for SVG elements that can be focused → 1. Add DispayOutline object to tree for SVG elements that can be focused
Attachment #8408764 - Attachment is obsolete: true
Attachment #8408764 - Flags: review?(cam)
Attachment #8409277 - Flags: review?(cam)
Flags: needinfo?(cam)
Please move the code to a common base class so we don't have two copies.
Is this code in the nightly builds now?
(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.
My review comment not to create two copies of the code still needs addressing. If that's done Cam or I will rereview.
Rick is comment 29 something you agree with and can address?
Flags: needinfo?(cabanier)
(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.
Flags: needinfo?(cabanier)
It looks like this one's pretty close to resolution. Is there anything we could do to help it move forward?
(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?
It's not something I could finish unfortunately, I'm just tracking the bug against another project that's waiting for it.
Are we any closer to resolution on this one?
This is part of the SVG2 specification. It needs to be implemented. It is mostly there in Webkit and Chrome.
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.
Rik, do you have status update on this bug? Do you still need review/needinfo from cam?
Flags: needinfo?(cam) → needinfo?(cabanier)
Any new information on this one?
Has any further progress been made on tabindex?

We are beginning to write test cases for SVG2 and SVG2 accessibility. Although we have not done exhaustive testing yet, SVG tabindex is now working in Webkit and Chrome.
(In reply to alexander :surkov from comment #38)
> Rik, do you have status update on this bug? Do you still need
> review/needinfo from cam?

More than a year passed by since the needinfo was requested. Cam, can the pages be merged?

Sebastian
Severity: normal → enhancement
Flags: needinfo?(cam)
Summary: implement tabindex attribute for SVG content → Implement 'tabIndex' attribute for SVG elements
(In reply to Rich Schwerdtfeger from comment #40)
> SVG tabindex is now working in Webkit and Chrome.

I've added this info to the whiteboard.

(In reply to Sebastian Zartner [:sebo] from comment #41)
> (In reply to alexander :surkov from comment #38)
> > Rik, do you have status update on this bug? Do you still need
> > review/needinfo from cam?
> 
> More than a year passed by since the needinfo was requested. Cam, can the
> pages be merged?

Ping to both of you. Can Rik's patches be merged? (Sorry for writing 'pages' earlier!)

Sebastian
Whiteboard: [parity-webkit][parity-blink]
I start to work on this bug. Add more complicated test case for this implementation, http://daoshengmu.github.io/layout-sample/svg/bug778654/
Rik, do you have any update for this bug?

If you don't have time to move forward it, I would like to take over the following works.
Hi Daosheng, I neglected review of Rik's patches and never got back to them.  Apologies to Rik.  If you can pick them up, update them, I can review them.  Thank you.
Flags: needinfo?(cam)
Comment 26 still needs addressing
Robert, please give me some feedback about this patch. Due to both of nsSVGElemnt and nsGenericHTMLElement have tabIndex support, I move their duplicate functions to Element class.
Attachment #8751276 - Flags: feedback?(longsonr)
Assignee: cabanier → dmu
Comment on attachment 8751276 [details] [diff] [review]
Move tabIndex functions to Element class

exactly what I expected. :-)
Attachment #8751276 - Flags: feedback?(longsonr) → feedback+
Comment on attachment 8751731 [details]
Bug 778654 - Implement and move tabIndex functions to Element class to avoid duplicate work;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52191/diff/1-2/
Comment on attachment 8751732 [details]
Bug 778654 - Display tabIndex outline;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52193/diff/1-2/
Comment on attachment 8751733 [details]
MozReview Request: Bug 778654 - Make SVG <a> elements focusable by default; r?heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52195/diff/1-2/
foreignObject seems to be missing.

for a tags you need to check that the href (or in our case xlink:href) is explicitly set (it should have an IsExplicitlySet method).
Comment on attachment 8751731 [details]
Bug 778654 - Implement and move tabIndex functions to Element class to avoid duplicate work;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52191/diff/2-3/
Comment on attachment 8751732 [details]
Bug 778654 - Display tabIndex outline;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52193/diff/2-3/
Comment on attachment 8751733 [details]
MozReview Request: Bug 778654 - Make SVG <a> elements focusable by default; r?heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52195/diff/2-3/
In this update, I mark tests for wpt and remove tabindex-transform SVG test because there is an AA problem after SVG transform on reftest.
(In reply to Robert Longson from comment #57)
> foreignObject seems to be missing.
> 
> for a tags you need to check that the href (or in our case xlink:href) is
> explicitly set (it should have an IsExplicitlySet method).

I will figure out how to support this at my next update.
(In reply to Daosheng Mu[:daoshengmu] from comment #50)
> Created attachment 8751731 [details]
> MozReview Request: Bug 778654 - Implement and move tabIndex functions to
> Element class to avoid duplicate work. r?heycam

This looks OK to me, but I think we should get a DOM peer to review it.  Maybe :peterv or :smaug?  It is a bit confusing that Blur is virtual but Focus is not, though.
Comment on attachment 8751732 [details]
Bug 778654 - Display tabIndex outline;

https://reviewboard.mozilla.org/r/52193/#review51092

We need to make outlines paint for <text>, <tspan>, <textPath> and (as Robert mentioned) <foreignObject> too.  Chrome supports outlines on all of these elements.  <text> and <foreignObject> should work just fine with a DisplayOutline() call like you've added to nsSVGPathGeometryFrame/nsSVGDisplayContainerFrame.

However, to support outlines on individual text content children (<tspan> and <textPath>) it will be a bit harder, since we generate only a single display item for the entire text element, and not individual display items for <tspan> children.  Each of these children might have glyphs painted anywhere, due to the x/y/dx/dy/rotate attributes.  The nsDisplayOutline display item determines its rectangle by looking at the bounds of the frame, but the nsInlineFrame that corresponds to the <tspan> doesn't know about the SVG glyph positioning attributes.  Because outlines paint on top of content, we can't just paint them inside SVGTextFrame::Paint.  So we might need to create a new display item type for outlines of SVG text content children, then add them in SVGTextFrame::BuildDisplayList.  This new display item type (maybe called nsDisplaySVGTextChildOutline) would need to ask the SVGTextFrame for the rectangle that encloses all of the glyphs for its frame.

I would be OK with supporting outlines on <tspan> / <textPath> in a followup bug.

::: dom/svg/SVGAElement.cpp:197
(Diff revision 3)
> +  if (nsSVGElement::IsFocusableInternal(aTabIndex, aWithMouse)) {
> +    return true;
> +  }

I think this change should be in the previous patch, or a separate patch "Make SVG <a> elements focusable by default".

::: layout/svg/svg.css:93
(Diff revision 3)
> +text:-moz-focusring,
> +use:-moz-focusring,
> +image:-moz-focusring
> +{
> +   /* Don't specify the outline-color, we should always use initial value. */
> +   outline: 1px dotted;

Is there any disadvantage to writing:

  *:-moz-focusring {
    outline: 1px dotted;
  }

?  If there is some reason we can't do this, then we need to include svg, tspan, textPath and foreignObject in the rule here.
Attachment #8751732 - Flags: review?(cam)
Depends on: 1275191
Review commit: https://reviewboard.mozilla.org/r/54762/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/54762/
Attachment #8751731 - Attachment description: MozReview Request: Bug 778654 - Implement and move tabIndex functions to Element class to avoid duplicate work. r?heycam → MozReview Request: Bug 778654 - Implement and move tabIndex functions to Element class to avoid duplicate work; r?heycam, peterv
Attachment #8751732 - Attachment description: MozReview Request: Bug 778654 - Display tabIndex outline. r?heycam → MozReview Request: Bug 778654 - Display tabIndex outline; r?heycam
Attachment #8751733 - Attachment description: MozReview Request: Bug 778654 - Add test files. r?heycam → MozReview Request: Bug 778654 - Make SVG <a> elements focusable by default; r?heycam
Attachment #8753309 - Attachment description: MozReview Request: Bug 778654 - Mark tests in web-platform-test pass. r?heycam → MozReview Request: Bug 778654 - Add test files. r?heycam
Attachment #8755732 - Flags: review?(cam)
Attachment #8751731 - Flags: review?(peterv)
Attachment #8751732 - Flags: review?(cam)
Comment on attachment 8751731 [details]
Bug 778654 - Implement and move tabIndex functions to Element class to avoid duplicate work;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52191/diff/3-4/
Comment on attachment 8751732 [details]
Bug 778654 - Display tabIndex outline;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52193/diff/3-4/
Comment on attachment 8751733 [details]
MozReview Request: Bug 778654 - Make SVG <a> elements focusable by default; r?heycam

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52195/diff/3-4/
Comment on attachment 8753309 [details]
Bug 778654 - Add test files;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53208/diff/1-2/
https://reviewboard.mozilla.org/r/52193/#review51092

I file Bug 1275191 to follow up this.

> Is there any disadvantage to writing:
> 
>   *:-moz-focusring {
>     outline: 1px dotted;
>   }
> 
> ?  If there is some reason we can't do this, then we need to include svg, tspan, textPath and foreignObject in the rule here.

*:-moz-focusring is ok. In original, I just make some types of SVG can be supported. In our discussion, it looks like that we can support all types of SVG. It makes sense to let all SVG styles be enabled.
https://reviewboard.mozilla.org/r/52191/#review51424

::: dom/base/Element.h:231
(Diff revision 4)
> +
> +  /**
> +   * Show blur and clear focus.
> +   */
> +  virtual void Blur(mozilla::ErrorResult& aError);
> +

Peterv, because I want to share the same code of tabIndex() that are used by SVGElement and nsGenericHTMLElement, I move them to Element.

Please help me review focus() & Blur() interface as comment 64 mentioned. Thanks a lot.
(In reply to Daosheng Mu[:daoshengmu] from comment #72)
> https://reviewboard.mozilla.org/r/52191/#review51424
> 
> ::: dom/base/Element.h:231
> (Diff revision 4)
> > +
> > +  /**
> > +   * Show blur and clear focus.
> > +   */
> > +  virtual void Blur(mozilla::ErrorResult& aError);
> > +
> 
> Peterv, because I want to share the same code of tabIndex() that are used by
> SVGElement and nsGenericHTMLElement, I move them to Element.
> 
> Please help me review focus() & Blur() interface as comment 64 mentioned.
> Thanks a lot.
Flags: needinfo?(peterv)
Flags: needinfo?(peterv)
Comment on attachment 8751731 [details]
Bug 778654 - Implement and move tabIndex functions to Element class to avoid duplicate work;

This should rely on ParseAttribute to force the attribute value to int, instead of doing it in the TabIndex getter.
I don't see a need for SetTabIndex in nsGenericHTMLElement.
I don't see a need for Blur in nsXULElement.
Why is Focus a nonvirtual method in Element taking the element as an argument? I know nsFocusManager::SetFocus takes a nsIDOMElement, but you can QI "this" to nsIDOMElement. That would remove the need for those other Focus methods (also the one in nsXULElement). For bonus points you should file a bug on changing the SetFocus method on nsFocusManager to take an Element and removing all the QIs in the callers.
Why doesn't SVGAElement::IsFocusableInternal need changes?
Attachment #8751731 - Flags: review?(peterv) → review-
Comment on attachment 8751731 [details]
Bug 778654 - Implement and move tabIndex functions to Element class to avoid duplicate work;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52191/diff/4-5/
Attachment #8751731 - Attachment description: MozReview Request: Bug 778654 - Implement and move tabIndex functions to Element class to avoid duplicate work; r?heycam, peterv → Bug 778654 - Implement and move tabIndex functions to Element class to avoid duplicate work;
Attachment #8751732 - Attachment description: MozReview Request: Bug 778654 - Display tabIndex outline; r?heycam → Bug 778654 - Display tabIndex outline;
Attachment #8753309 - Attachment description: MozReview Request: Bug 778654 - Add test files. r?heycam → Bug 778654 - Add test files;
Attachment #8755732 - Attachment description: MozReview Request: Bug 778654 - Mark tests in web-platform-test pass. r?heycam → Bug 778654 - Mark tests in web-platform-test pass;
Attachment #8751731 - Flags: review?(peterv)
Attachment #8751731 - Flags: review?(dmu)
Attachment #8751731 - Flags: review-
Comment on attachment 8751732 [details]
Bug 778654 - Display tabIndex outline;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52193/diff/4-5/
Comment on attachment 8753309 [details]
Bug 778654 - Add test files;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53208/diff/2-3/
Comment on attachment 8755732 [details]
Bug 778654 - Mark tests in web-platform-test pass;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54762/diff/1-2/
Attachment #8751733 - Attachment is obsolete: true
Attachment #8751733 - Flags: review?(cam)
(In reply to Peter Van der Beken [:peterv] from comment #74)
> Comment on attachment 8751731 [details]
> Bug 778654 - Implement and move tabIndex functions to Element class to avoid
> duplicate work;
> 
> This should rely on ParseAttribute to force the attribute value to int,

Please help take a look at my modification of Element:ParseAttribute. As far as I know, ParseAttribute is used for converting an attribute string value to attribute type. But in the case of getTabIndex, what should be the string value? Currently, I make this as an empty string to get the attribute value.

> instead of doing it in the TabIndex getter.
> I don't see a need for SetTabIndex in nsGenericHTMLElement.
> I don't see a need for Blur in nsXULElement.

Indeed, it needn't.

I know is doesn't overloading work for derived classes. My solution is adding using Element::SetTabIndex; at Class declaration.

> Why is Focus a nonvirtual method in Element taking the element as an
> argument? I know nsFocusManager::SetFocus takes a nsIDOMElement, but you can
> QI "this" to nsIDOMElement. 
I have used QI in this update. So I can remove this argument.

> That would remove the need for those other Focus
> methods (also the one in nsXULElement). For bonus points you should file a
> bug on changing the SetFocus method on nsFocusManager to take an Element and
> removing all the QIs in the callers.

I have filed Bug 1278172.
> Why doesn't SVGAElement::IsFocusableInternal need changes?

Originally, I put this commit at another patch. It should be merged. Done.
Attachment #8751731 - Flags: review?(dmu)
Comment on attachment 8751731 [details]
Bug 778654 - Implement and move tabIndex functions to Element class to avoid duplicate work;

https://reviewboard.mozilla.org/r/52191/#review54746

::: dom/base/Element.cpp:298
(Diff revision 5)
> +int32_t
> +Element::TabIndex()
> +{
> +  nsAttrValue result;
> +  const nsAutoString tabStr;
> +  if (ParseAttribute(kNameSpaceID_None, nsGkAtoms::tabindex, tabStr, result)) {

This is wrong, you need to convert the value on setting. SetAttr calls ParseAttribute, so you probably want to change nsSVGElement::ParseAttribute. The getter needs to do something similar to what nsGenericHTMLElement::GetIntAttr does. You just need to mimic what we currently do for HTML elements in nsGenericHTMLElement.
Attachment #8751731 - Flags: review?(peterv) → review-
Comment on attachment 8751731 [details]
Bug 778654 - Implement and move tabIndex functions to Element class to avoid duplicate work;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52191/diff/5-6/
Attachment #8751731 - Flags: review?(peterv)
Attachment #8751731 - Flags: review?(dmu)
Attachment #8751731 - Flags: review-
Comment on attachment 8751732 [details]
Bug 778654 - Display tabIndex outline;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52193/diff/5-6/
Comment on attachment 8753309 [details]
Bug 778654 - Add test files;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53208/diff/3-4/
Comment on attachment 8755732 [details]
Bug 778654 - Mark tests in web-platform-test pass;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54762/diff/2-3/
Attachment #8751731 - Flags: review?(dmu)
Comment on attachment 8751731 [details]
Bug 778654 - Implement and move tabIndex functions to Element class to avoid duplicate work;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52191/diff/6-7/
Attachment #8751731 - Flags: review?(dmu)
Comment on attachment 8751732 [details]
Bug 778654 - Display tabIndex outline;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52193/diff/6-7/
Comment on attachment 8753309 [details]
Bug 778654 - Add test files;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53208/diff/4-5/
Comment on attachment 8755732 [details]
Bug 778654 - Mark tests in web-platform-test pass;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54762/diff/3-4/
Attachment #8751731 - Flags: review?(dmu)
Attachment #8408763 - Attachment is obsolete: true
Attachment #8408763 - Flags: review?(cam)
Attachment #8409277 - Attachment is obsolete: true
Flags: needinfo?(cabanier)
Attachment #8409277 - Flags: review?(cam)
It looks like outlines aren't positioned properly when used on container elements.  For example with:

<!DOCTYPE html>
<style>
  g { outline: 1px solid red; }
</style>
<svg>
  <g>
    <circle cx="50" cy="50" r="20"></circle>
  </g>
</svg>

I would expect the outline to go around the bounding box of the <g> (i.e., tightly around the circle).  Can you find out why that's not working?
Flags: needinfo?(dmu)
heycam, I notice SVG outline is not supported on currently Nightly build. If you apply my attachment 8751732 [details] that will show the outline of SVGContainer and SVFPathGeometry. Besides, I notice our SVG container coordinate is not the same with Chrome's. Our origin is at the left-top corner of the viewport, but Chrome's origin is at the left-top corner of its elements.
Flags: needinfo?(dmu)
Depends on: 1281215
https://reviewboard.mozilla.org/r/52191/#review57366

r=me on the dom/svg/ changes.

::: dom/svg/nsSVGElement.cpp:614
(Diff revision 7)
>            aResult.SetTo(transformList->GetBaseValue(), &aValue);
>            didSetResult = true;
>          }
>          foundMatch = true;
> +      } else if (aAttribute == nsGkAtoms::tabindex) {
> +        aResult.ParseIntValue(aValue);

I think we should be returning false from this function when ParseIntValue fails.  We could do that by doing:

  didSetResult = aResult.ParseIntValue(aValue);
Comment on attachment 8751731 [details]
Bug 778654 - Implement and move tabIndex functions to Element class to avoid duplicate work;

https://reviewboard.mozilla.org/r/52191/#review57368
Attachment #8751731 - Flags: review?(cam) → review+
Comment on attachment 8753309 [details]
Bug 778654 - Add test files;

https://reviewboard.mozilla.org/r/53208/#review57374

This is OK, but I'd like to see some more extensive tests.

::: dom/svg/test/test_tabindex.html:10
(Diff revision 5)
> +  <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
> +</head>
> +<body>
> +<svg xmlns="http://www.w3.org/2000/svg" overflow="visible">
> +  <foreignObject id="f" x="0" y="0" width="200" height="60" requiredExtensions="http://www.w3.org/1999/xhtml" tabindex="0">

Remove requiredExtensions="", which isn't required.

::: dom/svg/test/test_tabindex.html:38
(Diff revision 5)
> +    is(t.tabIndex, 2, "text initial tabIndex");
> +    t.tabIndex = 3;
> +    is(t.tabIndex, 3, "text is set to 3");

In addition to testing that assigning to tabIndex works, can you please add some tests that they actually affect the tab order, for example by using synthesizeKey("VK_TAB", {}) and checking document.activeElement.

::: layout/reftests/svg/tabindex.html:11
(Diff revision 5)
> +}
> +</style>
> +<body onload="document.getElementById('f').focus()">
> +<svg xmlns="http://www.w3.org/2000/svg" overflow="visible">
> +  <g id="f" tabindex="0">
> +    <rect width="100" height="100" fill="yellow"/>

Since this is more of a test of outline rendering rather than tabindex, please call the test file outline.html and just use:

  rect {
    outline: 1px dotted;
  }
  
in the style.  Please also add all renderable SVG elements in the test to make sure outlines render correctly on those.
Attachment #8753309 - Flags: review?(cam)
Comment on attachment 8755732 [details]
Bug 778654 - Mark tests in web-platform-test pass;

https://reviewboard.mozilla.org/r/54762/#review57376
Attachment #8755732 - Flags: review?(cam) → review+
Attachment #8751732 - Flags: review?(cam)
Attachment #8751731 - Flags: review?(peterv) → review-
Comment on attachment 8751731 [details]
Bug 778654 - Implement and move tabIndex functions to Element class to avoid duplicate work;

https://reviewboard.mozilla.org/r/52191/#review56542

::: dom/base/Element.h:217
(Diff revision 7)
> +   * Set tabIndex value to this element.
> +   */
> +  void SetTabIndex(int32_t aTabIndex, mozilla::ErrorResult& aError);
> +
> +  /**
> +   * Make focus on this aElement.

element

::: dom/base/Element.cpp:298
(Diff revision 7)
> +int32_t
> +Element::TabIndex()
> +{
> +  const nsAttrValue* attrVal = mAttrsAndChildren.GetAttr(nsGkAtoms::tabindex);
> +  if (attrVal) {
> +    if (attrVal->Type() == nsAttrValue::eInteger) {

Combine the 2 if statements into 1.

::: dom/html/nsGenericHTMLElement.h:109
(Diff revision 7)
>    {
>      SetHTMLBoolAttr(nsGkAtoms::hidden, aHidden, aError);
>    }
>    virtual void Click();
> -  virtual int32_t TabIndexDefault()
> +  virtual void Focus(mozilla::ErrorResult& aError)
>    {

Can't we just use Element::Focus instead of overriding and then just calling Element::Focus?

::: dom/svg/nsSVGElement.h:320
(Diff revision 7)
>  
>    // WebIDL
>    mozilla::dom::SVGSVGElement* GetOwnerSVGElement();
>    nsSVGElement* GetViewportElement();
>    already_AddRefed<mozilla::dom::SVGAnimatedString> ClassName();
> +  void Focus(mozilla::ErrorResult& aError);

virtual and override. But again, can't we just use Element::Focus?
Blocks: 1278172
I have updated my patches with reviewer's comments, and the try result looks great. (https://hg.mozilla.org/try/rev/faf12f1ea226). Because Mozreview can't push commits while there is any of the reviewers is not available to take review. Please wait my next push when it is available for me.
Comment on attachment 8751731 [details]
Bug 778654 - Implement and move tabIndex functions to Element class to avoid duplicate work;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52191/diff/7-8/
Attachment #8751731 - Flags: review- → review?(peterv)
Attachment #8751732 - Flags: review?(cam)
Attachment #8753309 - Flags: review?(cam)
Comment on attachment 8751732 [details]
Bug 778654 - Display tabIndex outline;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52193/diff/7-8/
Comment on attachment 8753309 [details]
Bug 778654 - Add test files;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53208/diff/5-6/
Comment on attachment 8755732 [details]
Bug 778654 - Mark tests in web-platform-test pass;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54762/diff/4-5/
Because bug 1278172 is landed, we can keep going this bug.

In this update, I have enhanced my patches by following the above comments.
Try result is good, https://treeherder.mozilla.org/#/jobs?repo=try&revision=bddd67b5058b.
Comment on attachment 8751732 [details]
Bug 778654 - Display tabIndex outline;

https://reviewboard.mozilla.org/r/52193/#review65802

::: layout/svg/svg.css:80
(Diff revision 8)
>    mask: inherit;
>    opacity: inherit;
>  }
>  
> +*:-moz-focusring
> +{

Nit: "{" on previous line.
Attachment #8751732 - Flags: review?(cam) → review+
Comment on attachment 8753309 [details]
Bug 778654 - Add test files;

https://reviewboard.mozilla.org/r/53208/#review65804

r=me with these things addressed.

::: layout/reftests/svg/outline-ref.html:38
(Diff revision 6)
> +  </g>
> +</svg>
> +</body>
> +</html>
> +
> +<script type="text/javascript">

The HTML parser complains about content after the closing </html> tag.  Please put this just before </body>.

Nit: Also, the type="" attribute doesn't do anything these days, so please remove it.

::: layout/reftests/svg/outline-ref.html:51
(Diff revision 6)
> +
> +  // Outline has two more pixels than its bounding rect.
> +  var width = boundingRect.width + 2;
> +  var height = boundingRect.height + 2;
> +
> +  var topLine = document.createElement("div");

Instead of creating four <div>s for each side of the outline rectangle, can we just create a single <div> that forms the entire outline?

::: layout/reftests/svg/outline-ref.html:94
(Diff revision 6)
> +  var rect = document.getElementById('rect');
> +  var bbox = rect.getBoundingClientRect();
> +  createOutline(bbox);
> +
> +  var foreignObject = document.getElementById('foreignObject');
> +  bbox = foreignObject.getBoundingClientRect();
> +  createOutline(bbox);

If you want this to be a bit more concise, you could write something like:

  var elements = ["rect", "foreignObject", ...];
  elements.forEach(id => {
    var element = document.getElementById(id);
    createOutline(element.getBoundingClientRect());
  });

and then handle the <g> at the end separately.

::: layout/reftests/svg/outline-ref.html:145
(Diff revision 6)
> +  var ggbbox = new Object();
> +  ggbbox.left = ggRectbbox.left;
> +  ggbbox.top = ggRectbbox.top;
> +  ggbbox.right = ggCirclebbox.right;
> +  ggbbox.bottom = ggCirclebbox.bottom;

Small nit: I don't think |new Object()| is very idiomatic JavaScript.  It would be more common to see:

  var ggbbox = {
    left: ggRectbbox.left,
    top: ggRectbbox.top,
    ...
  };
  ggbbox.width = ...;
  ...

::: layout/reftests/svg/outline.html:1
(Diff revision 6)
> +<!DOCTYPE <!DOCTYPE html>

This line looks wrong.

::: layout/reftests/svg/outline.html:26
(Diff revision 6)
> +    <rect width="100" height="100" fill="yellow"/>
> +    <text x="0" y="140" font-family="Verdana" font-size="20">
> +      Hello world
> +    </text>
> +    <foreignObject x="0" y="160" width="200" height="60">
> +      <body xmlns="http://www.w3.org/1999/xhtml">

When I view source on this document, I see it complains about the use of <body> in the middle of the document.  I think this should be a <div>, to make the HTML parser happy.

::: layout/reftests/svg/outline.html:33
(Diff revision 6)
> +      </body>
> +    </foreignObject>
> +    <circle cx="40" cy="275" r="40" fill="green"/>
> +    <ellipse cx="70" cy="380" rx="70" ry="50" fill="yellow"/>
> +    <image x="0" y="450" height="100" width="100" />
> +    <line x1="0" y1="580" x2="100" y2="580" style="stroke:rgb(255,0,0);stroke-width:2"/>

Nit: please be consistent with how properties are set on these elements.  On most elements you are using presentation attributes like fill="lime", but here and on a couple of other elements, you are using style="".  (I know that outline will have to be set in style="", since there is no corresponding presentation attribute for it.)

::: layout/reftests/svg/outline.html:38
(Diff revision 6)
> +    <line x1="0" y1="580" x2="100" y2="580" style="stroke:rgb(255,0,0);stroke-width:2"/>
> +    <path d="M50 600 L10 650 L90 650 Z" />
> +    <polygon points="300,50 350,0 400,50" fill="lime"/>
> +    <polyline points="300,80 350,70 400,80" style="fill:none;stroke:black;stroke-width:3"/>
> +    <g transform="translate(300, 70)" style="outline: 1px solid">
> +      <circle cx="50" cy="50" r="20" fill="blue" style="outline: 0px"></circle>

Nit: please use the self-closing form <circle .../> (and on the <circle> and <rect> below) like you do with other shape elements in the document.

::: layout/reftests/svg/outline.html:43
(Diff revision 6)
> +      <circle cx="50" cy="50" r="20" fill="blue" style="outline: 0px"></circle>
> +    </g>
> +    <g transform="translate(300, 150)" style="outline: 1px solid">
> +      <circle cx="50" cy="50" r="20" fill="green" style="outline: 0px"></circle>
> +      <g>
> +      <rect x="15" y="15" width="30" height="10" style="fill:rgb(0,0,255)"></rect>

Nit: this should be indented one more level.

(All of these comments apply to outline-ref.html, too.)

::: layout/reftests/svg/outline.html:47
(Diff revision 6)
> +      <g>
> +      <rect x="15" y="15" width="30" height="10" style="fill:rgb(0,0,255)"></rect>
> +      </g>
> +    </g>
> +  </g>
> +</svg>

Please add a subtest for an inner <svg> element, too.
Attachment #8753309 - Flags: review?(cam) → review+
Comment on attachment 8751731 [details]
Bug 778654 - Implement and move tabIndex functions to Element class to avoid duplicate work;

https://reviewboard.mozilla.org/r/52191/#review66360

::: dom/xul/nsXULElement.h:565
(Diff revision 8)
>      already_AddRefed<nsIRDFCompositeDataSource> GetDatabase();
>      already_AddRefed<nsIXULTemplateBuilder> GetBuilder();
>      already_AddRefed<nsIRDFResource> GetResource(mozilla::ErrorResult& rv);
>      nsIControllers* GetControllers(mozilla::ErrorResult& rv);
>      already_AddRefed<mozilla::dom::BoxObject> GetBoxObject(mozilla::ErrorResult& rv);
> -    void Focus(mozilla::ErrorResult& rv);
> +    virtual void Focus(mozilla::ErrorResult& rv) override;

It looks to me like nsXULElement::Focus is identical to the Element::Focus that you're adding. If that's the case then you should just remove nsXULElement::Focus too.
Attachment #8751731 - Flags: review?(peterv) → review+
Comment on attachment 8751731 [details]
Bug 778654 - Implement and move tabIndex functions to Element class to avoid duplicate work;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52191/diff/8-9/
Comment on attachment 8751732 [details]
Bug 778654 - Display tabIndex outline;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52193/diff/8-9/
Comment on attachment 8753309 [details]
Bug 778654 - Add test files;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53208/diff/6-7/
Comment on attachment 8755732 [details]
Bug 778654 - Mark tests in web-platform-test pass;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54762/diff/5-6/
https://reviewboard.mozilla.org/r/53208/#review65804

> Instead of creating four <div>s for each side of the outline rectangle, can we just create a single <div> that forms the entire outline?

Yes. Let's do this way.
var lines = document.createElement("div");
var styles = 'border: 1px solid; '
              + 'width: ' + width + 'px; '
              + 'height: ' + height + 'px; '
              + 'position: absolute; '
              + 'top: ' + top + 'px; '
              + 'left: ' + left + 'px; ';
Comment on attachment 8753309 [details]
Bug 778654 - Add test files;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53208/diff/7-8/
Comment on attachment 8755732 [details]
Bug 778654 - Mark tests in web-platform-test pass;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54762/diff/6-7/
Comment on attachment 8753309 [details]
Bug 778654 - Add test files;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53208/diff/8-9/
Comment on attachment 8755732 [details]
Bug 778654 - Mark tests in web-platform-test pass;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/54762/diff/7-8/
Keywords: checkin-needed
No longer depends on: 1275191
Pushed by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7c1e2b74a444
Implement and move tabIndex functions to Element class to avoid duplicate work.r=peterv,heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/a42d9b555906
Display tabIndex outline.r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/f270cd47fba8
Add test files.r=heycam
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f45f00e67c6
Mark tests in web-platform-test pass.r=heycam
Keywords: checkin-needed
Added a description for the tabindex attribute at https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/tabindex and added it to the release notes for Firefox 51 at https://developer.mozilla.org/en-US/Firefox/Releases/51.

Daosheng, can you please check the two pages and let me know whether everything is ok?

Sebastian
Flags: needinfo?(dmu)
(In reply to Sebastian Zartner [:sebo] from comment #117)
> Added a description for the tabindex attribute at
> https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/tabindex and
> added it to the release notes for Firefox 51 at
> https://developer.mozilla.org/en-US/Firefox/Releases/51.
> 
> Daosheng, can you please check the two pages and let me know whether
> everything is ok?
> 
> Sebastian

Hi Sebastian,

They look good. Thanks for the description.
Flags: needinfo?(dmu)
(In reply to Daosheng Mu[:daoshengmu] from comment #118)
> (In reply to Sebastian Zartner [:sebo] from comment #117)
> > Added a description for the tabindex attribute at
> > https://developer.mozilla.org/en-US/docs/Web/SVG/Attribute/tabindex and
> > added it to the release notes for Firefox 51 at
> > https://developer.mozilla.org/en-US/Firefox/Releases/51.
> > 
> > Daosheng, can you please check the two pages and let me know whether
> > everything is ok?
> > 
> > Sebastian
> 
> Hi Sebastian,
> 
> They look good. Thanks for the description.

Good. Thank you for the review and especially for the implementation! :-)

Sebastian
Depends on: 1116966
Is it intentional that tabindex="-1" does nothing? see http://jsbin.com/fipirihuje/1/edit?html,output for a quick test-case.

To match behavior of HTML elements (and Blink/Webkit);

* <text tabindex="-1"> should be script and pointer focusable (like HTML elements)
* <a xlink:href="…" tabindex="-1"> should remove the element from the document's tabbing order
* <a xlink:href="…"> is getting a tiny focus outline that does not match the outline of its content
(In reply to Rodney Rehm from comment #120)
> Is it intentional that tabindex="-1" does nothing? see
> http://jsbin.com/fipirihuje/1/edit?html,output for a quick test-case.
> 
> To match behavior of HTML elements (and Blink/Webkit);
> 
> * <text tabindex="-1"> should be script and pointer focusable (like HTML
> elements)
> * <a xlink:href="…" tabindex="-1"> should remove the element from the
> document's tabbing order
> * <a xlink:href="…"> is getting a tiny focus outline that does not match the
> outline of its content

Hi Rodney,

Thanks for your feedback. I think I miss the implementation of tabindex on SVGAElement and negative tabindex should be focusable.

In the case of <a xlink:href="…" tabindex="-1">, it is already removed from tabbing order. We just need to make sure the focus method is work on it. 

https://developer.mozilla.org/en-US/docs/Web/HTML/Global_attributes/tabindex
(In reply to Daosheng Mu[:daoshengmu] from comment #121)
> Thanks for your feedback. I think I miss the implementation of tabindex on
> SVGAElement and negative tabindex should be focusable.

should I open new bugs for that, or do you want to track this here?

> In the case of <a xlink:href="…" tabindex="-1">, it is already removed from
> tabbing order. We just need to make sure the focus method is work on it. 

I can press the tab key to make it receive focus. The ouline just isn't where it's supposed to be. http://jsbin.com/cufobatuko/edit?html,output

I have one more nitpick, for <a xlink:href=""> I see element.tabIndex == -1, but it should be 0.
(In reply to Rodney Rehm from comment #122)
> (In reply to Daosheng Mu[:daoshengmu] from comment #121)
> > Thanks for your feedback. I think I miss the implementation of tabindex on
> > SVGAElement and negative tabindex should be focusable.
> 
> should I open new bugs for that, or do you want to track this here?

Please open a new bug on this, I will help resolve when I have time.

> 
> > In the case of <a xlink:href="…" tabindex="-1">, it is already removed from
> > tabbing order. We just need to make sure the focus method is work on it. 
> 
> I can press the tab key to make it receive focus. The ouline just isn't
> where it's supposed to be. http://jsbin.com/cufobatuko/edit?html,output
> 
> I have one more nitpick, for <a xlink:href=""> I see element.tabIndex == -1,
> but it should be 0.

Interesting! SVGAElement seems to have some works to do.
(In reply to Daosheng Mu[:daoshengmu] from comment #123)
> Please open a new bug on this, I will help resolve when I have time.

https://bugzilla.mozilla.org/show_bug.cgi?id=1302340
https://bugzilla.mozilla.org/show_bug.cgi?id=1302341
Blocks: 1302340
Blocks: 1302341
Hope this is fixed soon.
In my case I wanted to implement an accessible button with SVG, and can just not implement the keyboard behaviour because of that bug.

My code:
	<svg tabindex="0" role="button" class="header__navdrawerOpenBtn icon" aria-label="open navdrawer">
		<use xlink:href="#accessibility"></use>
	</svg>


In other browsers, I can focus that "svg button", not in firefox yet.
(In reply to lingtalfi from comment #125)
> Hope this is fixed soon.

This is already fixed in Firefox 51. Please see comment 117.

Sebastian
(In reply to Sebastian Zartner [:sebo] from comment #126)
> (In reply to lingtalfi from comment #125)
> > Hope this is fixed soon.
> 
> This is already fixed in Firefox 51. Please see comment 117.
> 
> Sebastian

Awesome, thanks (tested and it works).
Depends on: 1326978
Depends on: 1327887
Depends on: 1327888
Blocks: svg2
Depends on: 1366494
Regressions: 1589164
You need to log in before you can comment on or make changes to this bug.