Implement 'tabIndex' attribute for SVG elements

RESOLVED FIXED in Firefox 51

Status

()

Core
SVG
--
enhancement
RESOLVED FIXED
5 years ago
a month ago

People

(Reporter: Jeremie, Assigned: daoshengmu)

Tracking

(Depends on: 4 bugs, Blocks: 3 bugs, {dev-doc-complete})

unspecified
mozilla51
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox51 fixed)

Details

(Whiteboard: [parity-webkit][parity-blink], URL)

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(6 attachments, 6 obsolete attachments)

8.04 KB, patch
Robert Longson
: feedback+
Details | Diff | Splinter Review
58 bytes, text/x-review-board-request
heycam
: review+
peterv
: review+
Details | Review
58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
58 bytes, text/x-review-board-request
heycam
: review+
Details | Review
393 bytes, text/html
Details
(Reporter)

Description

5 years ago
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.
(Reporter)

Updated

5 years ago
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)

Comment 10

3 years ago
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

3 years ago
Erik Dalhstrom has tabindex under code review now before it drops in Canary. So, it will be in Chrome Canary soon.

Updated

3 years ago
Assignee: nobody → cabanier

Comment 12

3 years ago
Thanks Rik.

Comment 13

3 years ago
(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...

Comment 16

3 years ago
Created attachment 8400172 [details] [diff] [review]
1. Add IDL for tabindex
Attachment #8400172 - Flags: review?(cam)
Friendly 2 week review nudge for Cameron.
Flags: needinfo?(cam)

Comment 18

3 years ago
Created attachment 8407288 [details] [diff] [review]
tabindexidl.patch
Attachment #8400172 - Attachment is obsolete: true
Attachment #8400172 - Flags: review?(cam)

Updated

3 years ago
Attachment #8407288 - Flags: review?(cam)

Updated

3 years ago
Duplicate of this bug: 996407
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 21

3 years ago
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

3 years ago
Created attachment 8408763 [details] [diff] [review]
1. Add DispayOutline object to tree for SVG elements that can be focused
Attachment #8407288 - Attachment is obsolete: true
Attachment #8408763 - Flags: review?(cam)

Updated

3 years ago
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

Comment 23

3 years ago
Created attachment 8408764 [details] [diff] [review]
2. Add IDL + support for tabindex + focus to SVG elements. Also added tests.
Attachment #8408764 - Flags: review?(cam)

Comment 24

3 years ago
Created attachment 8409277 [details] [diff] [review]
2. Add IDL + support for tabindex + focus to SVG elements. Also added tests.
Attachment #8408764 - Attachment is obsolete: true
Attachment #8408764 - Flags: review?(cam)
Attachment #8409277 - Flags: review?(cam)

Comment 25

3 years ago
try run: https://tbpl.mozilla.org/?tree=Try&rev=f0bbb14ecd65

Updated

3 years ago
Flags: needinfo?(cam)

Comment 26

3 years ago
Please move the code to a common base class so we don't have two copies.

Comment 27

3 years ago
Is this code in the nightly builds now?

Comment 28

3 years ago
(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

3 years ago
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)

Comment 31

3 years ago
(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)

Comment 32

3 years ago
It looks like this one's pretty close to resolution. Is there anything we could do to help it move forward?

Comment 33

3 years ago
(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

3 years ago
It's not something I could finish unfortunately, I'm just tracking the bug against another project that's waiting for it.

Comment 35

3 years ago
Are we any closer to resolution on this one?

Comment 36

3 years ago
This is part of the SVG2 specification. It needs to be implemented. It is mostly there in Webkit and Chrome.

Comment 37

3 years ago
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

3 years ago
Rik, do you have status update on this bug? Do you still need review/needinfo from cam?
Flags: needinfo?(cam) → needinfo?(cabanier)

Comment 39

2 years ago
Any new information on this one?
Keywords: dev-doc-needed

Comment 40

2 years ago
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

Updated

a year ago
Duplicate of this bug: 1257399
Blocks: 1262352
(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]
(Assignee)

Comment 44

a year ago
I start to work on this bug. Add more complicated test case for this implementation, http://daoshengmu.github.io/layout-sample/svg/bug778654/
(Assignee)

Comment 45

a year ago
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 47

a year ago
Comment 26 still needs addressing
(Assignee)

Comment 48

a year ago
Created attachment 8751276 [details] [diff] [review]
Move tabIndex functions to Element class

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)

Updated

a year ago
Assignee: cabanier → dmu

Comment 49

a year ago
Comment on attachment 8751276 [details] [diff] [review]
Move tabIndex functions to Element class

exactly what I expected. :-)
Attachment #8751276 - Flags: feedback?(longsonr) → feedback+
(Assignee)

Comment 50

a year ago
Created attachment 8751731 [details]
Bug 778654 - Implement and move tabIndex functions to Element class to avoid duplicate work;

Review commit: https://reviewboard.mozilla.org/r/52191/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52191/
Attachment #8751731 - Flags: review?(cam)
Attachment #8751732 - Flags: review?(cam)
Attachment #8751733 - Flags: review?(cam)
(Assignee)

Comment 51

a year ago
Created attachment 8751732 [details]
Bug 778654 - Display tabIndex outline;

Review commit: https://reviewboard.mozilla.org/r/52193/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52193/
(Assignee)

Comment 52

a year ago
Created attachment 8751733 [details]
MozReview Request: Bug 778654 - Make SVG <a> elements focusable by default; r?heycam

Review commit: https://reviewboard.mozilla.org/r/52195/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/52195/
(Assignee)

Comment 53

a year ago
http://daoshengmu.github.io/layout-sample/svg/bug778654/focusAndBlur.html For Focus and Blur sample.
(Assignee)

Comment 54

a year ago
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/
(Assignee)

Comment 55

a year ago
Comment on attachment 8751732 [details]
Bug 778654 - Display tabIndex outline;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52193/diff/1-2/
(Assignee)

Comment 56

a year ago
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/

Comment 57

a year ago
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).
(Assignee)

Comment 58

a year ago
Created attachment 8753309 [details]
Bug 778654 - Add test files;

Review commit: https://reviewboard.mozilla.org/r/53208/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/53208/
Attachment #8753309 - Flags: review?(cam)
(Assignee)

Comment 59

a year ago
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/
(Assignee)

Comment 60

a year ago
Comment on attachment 8751732 [details]
Bug 778654 - Display tabIndex outline;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52193/diff/2-3/
(Assignee)

Comment 61

a year ago
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/
(Assignee)

Comment 62

a year ago
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.
(Assignee)

Comment 63

a year ago
(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)
(Assignee)

Updated

a year ago
Depends on: 1275191
(Assignee)

Comment 66

a year ago
Created attachment 8755732 [details]
Bug 778654 - Mark tests in web-platform-test pass;

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)
(Assignee)

Comment 67

a year ago
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/
(Assignee)

Comment 68

a year ago
Comment on attachment 8751732 [details]
Bug 778654 - Display tabIndex outline;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52193/diff/3-4/
(Assignee)

Comment 69

a year ago
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/
(Assignee)

Comment 70

a year ago
Comment on attachment 8753309 [details]
Bug 778654 - Add test files;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53208/diff/1-2/
(Assignee)

Comment 71

a year ago
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.
(Assignee)

Comment 72

a year ago
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.
(Assignee)

Comment 73

a year ago
(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-
(Assignee)

Comment 75

a year ago
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-
(Assignee)

Comment 76

a year ago
Comment on attachment 8751732 [details]
Bug 778654 - Display tabIndex outline;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52193/diff/4-5/
(Assignee)

Comment 77

a year ago
Comment on attachment 8753309 [details]
Bug 778654 - Add test files;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53208/diff/2-3/
(Assignee)

Comment 78

a year ago
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/
(Assignee)

Updated

a year ago
Attachment #8751733 - Attachment is obsolete: true
Attachment #8751733 - Flags: review?(cam)
(Assignee)

Comment 79

a year ago
(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.
(Assignee)

Updated

a year ago
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-
(Assignee)

Comment 81

a year ago
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-
(Assignee)

Comment 82

a year ago
Comment on attachment 8751732 [details]
Bug 778654 - Display tabIndex outline;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52193/diff/5-6/
(Assignee)

Comment 83

a year ago
Comment on attachment 8753309 [details]
Bug 778654 - Add test files;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53208/diff/3-4/
(Assignee)

Comment 84

a year ago
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/
(Assignee)

Updated

a year ago
Attachment #8751731 - Flags: review?(dmu)
(Assignee)

Comment 85

a year ago
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)
(Assignee)

Comment 86

a year ago
Comment on attachment 8751732 [details]
Bug 778654 - Display tabIndex outline;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52193/diff/6-7/
(Assignee)

Comment 87

a year ago
Comment on attachment 8753309 [details]
Bug 778654 - Add test files;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53208/diff/4-5/
(Assignee)

Comment 88

a year ago
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/
(Assignee)

Updated

a year ago
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)
(Assignee)

Comment 90

a year ago
Created attachment 8763749 [details]
sample for SVG container outline box

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)
(Assignee)

Updated

a year ago
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?
(Assignee)

Updated

a year ago
Blocks: 1278172
(Assignee)

Comment 96

a year ago
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.
(Assignee)

Comment 97

11 months ago
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)
(Assignee)

Comment 98

11 months ago
Comment on attachment 8751732 [details]
Bug 778654 - Display tabIndex outline;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52193/diff/7-8/
(Assignee)

Comment 99

11 months ago
Comment on attachment 8753309 [details]
Bug 778654 - Add test files;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53208/diff/5-6/
(Assignee)

Comment 100

11 months ago
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/
(Assignee)

Comment 101

11 months ago
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+
(Assignee)

Comment 105

11 months ago
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/
(Assignee)

Comment 106

11 months ago
Comment on attachment 8751732 [details]
Bug 778654 - Display tabIndex outline;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/52193/diff/8-9/
(Assignee)

Comment 107

11 months ago
Comment on attachment 8753309 [details]
Bug 778654 - Add test files;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53208/diff/6-7/
(Assignee)

Comment 108

11 months ago
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/
(Assignee)

Comment 109

11 months ago
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; ';
(Assignee)

Comment 110

11 months ago
Comment on attachment 8753309 [details]
Bug 778654 - Add test files;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53208/diff/7-8/
(Assignee)

Comment 111

11 months ago
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/
(Assignee)

Comment 112

11 months ago
Comment on attachment 8753309 [details]
Bug 778654 - Add test files;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/53208/diff/8-9/
(Assignee)

Comment 113

11 months ago
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/
(Assignee)

Comment 114

11 months ago
Try result looks good, https://treeherder.mozilla.org/#/jobs?repo=try&revision=1dc1927b0ae7
(Assignee)

Updated

11 months ago
Keywords: checkin-needed
(Assignee)

Updated

11 months ago
No longer depends on: 1275191

Comment 115

11 months ago
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

Comment 116

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7c1e2b74a444
https://hg.mozilla.org/mozilla-central/rev/a42d9b555906
https://hg.mozilla.org/mozilla-central/rev/f270cd47fba8
https://hg.mozilla.org/mozilla-central/rev/6f45f00e67c6
Status: NEW → RESOLVED
Last Resolved: 11 months ago
status-firefox51: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla51
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)
(Assignee)

Comment 118

10 months ago
(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
Keywords: dev-doc-needed → dev-doc-complete

Updated

9 months ago
Depends on: 1116966

Comment 120

9 months ago
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
(Assignee)

Comment 121

9 months ago
(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

Comment 122

9 months ago
(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.
(Assignee)

Comment 123

9 months ago
(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.

Comment 124

9 months ago
(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

Updated

9 months ago
Blocks: 1302340

Updated

9 months ago
Blocks: 1302341

Comment 125

9 months ago
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

Comment 127

9 months ago
(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).

Updated

6 months ago
Depends on: 1326978

Updated

6 months ago
Depends on: 1327887

Updated

6 months ago
Depends on: 1327888
Blocks: 1328534

Updated

2 months ago
Blocks: 369507

Updated

a month ago
Depends on: 1366494
You need to log in before you can comment on or make changes to this bug.