User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:51.0) Gecko/20100101 Firefox/51.0 Build ID: 20160912030421 Steps to reproduce: Add tabindex="-1" to svg elements like <text> and <a xlink:href="…"> see http://jsbin.com/fipirihuje/1/edit?html,output and http://jsbin.com/cufobatuko/edit?html,output Actual results: * <text tabindex="-1"> is not focusable by script and pointer * <a xlink:href="…" tabindex="-1"> is focusable by keyboard Expected results: * <text tabindex="-1"> should be focusable by script and pointer * <a xlink:href="…" tabindex="-1"> should not be focusable by keyboard

I find the root cause is IsFocusableInternal() has different implementation btw nsSVGElement and nsGenericHTMLElement. IIUC, for the usage of SVGElement, we just need to confirm if the tabindex is already assigned or larger or equal to 0, and it is focusable.

Try looks good, https://treeherder.mozilla.org/#/jobs?repo=try&revision=b58de85a9fa4.

(In reply to Daosheng Mu[:daoshengmu] from comment #4) > Comment on attachment 8795681 [details] > Bug 1302340 - Allow -1 tabIndex SVGElement to be focusable; > > Review request updated; see interdiff: > https://reviewboard.mozilla.org/r/81648/diff/1-2/ I find negative tabindex in SVGElements can be focusable in Firefox 52. Only when tabindex is -1, they will not be able to be focused. So I correct my commit message.

(In reply to Daosheng Mu[:daoshengmu] from comment #1) > […] IIUC, for the usage of > SVGElement, we just need to confirm if the tabindex is already assigned or > larger or equal to 0, and it is focusable. Not quite. Any element with tabindex="-1" (in fact any negative value) should be focusable by script and pointer, but it should not be part of the document's tabbing order. That means clicking on the element should shift focus to it. element.focus() should shift focus to it. But <kbd>Tab</kbd> should *not* shift focus to it.

(In reply to Rodney Rehm from comment #7) > (In reply to Daosheng Mu[:daoshengmu] from comment #1) > > […] IIUC, for the usage of > > SVGElement, we just need to confirm if the tabindex is already assigned or > > larger or equal to 0, and it is focusable. > > Not quite. Any element with tabindex="-1" (in fact any negative value) > should be focusable by script and pointer, but it should not be part of the > document's tabbing order. That means clicking on the element should shift > focus to it. element.focus() should shift focus to it. But <kbd>Tab</kbd> > should *not* shift focus to it. My patch should have resolved the problem that you mentioned above. > the tabindex is already assigned I check if the tabindex with a value no matter it is -1 or anything. Originally, the negative SVGElement tabindex can be focused, only tabIndex = -1 can't. This is because our default tabindex is -1, and SVGElement doesn't check it is changed.

(In reply to Daosheng Mu[:daoshengmu] from comment #8) > (In reply to Rodney Rehm from comment #7) > > Not quite. Any element with tabindex="-1" (in fact any negative value) > > should be focusable by script and pointer, but it should not be part of the > > document's tabbing order. That means clicking on the element should shift > > focus to it. element.focus() should shift focus to it. But <kbd>Tab</kbd> > > should *not* shift focus to it. > > My patch should have resolved the problem that you mentioned above. If your patch is active in 52.0a1 (2016-10-10) (64-bit), then no, it hasn't resolved the tabindex="-1" situation.

(In reply to Rodney Rehm from comment #9) > > If your patch is active in 52.0a1 (2016-10-10) (64-bit), then no, it hasn't > resolved the tabindex="-1" situation. the patch hasn't landed, if it had this bug would have changed state to fixed. You'd have to download the firefox source, apply the patch and build from source to test the patch currently (or find someone who would do that for you).

Comment on attachment 8795681 [details] Bug 1302340 - Allow SVGElement to be focusable when tabindex is -1; https://reviewboard.mozilla.org/r/81648/#review107760 r=me with the changes below. ::: dom/svg/nsSVGElement.cpp:1141 (Diff revision 4) > + return (index >= 0 > + || HasAttr(kNameSpaceID_None, nsGkAtoms::tabindex)); Nit: no need for the parents around the whole expression. ::: dom/svg/nsSVGElement.h:350 (Diff revision 5) > + > + return value > 0 ? eTrue : (value == 0 ? eFalse : eInherit); > + } > + > + bool IsEditableRoot() const; > + virtual bool IsSVGFocusable(bool *aIsFocusable, int32_t *aTabIndex); Nit: Move "*"s next to types. ::: dom/svg/nsSVGElement.cpp:1155 (Diff revision 5) > + > + return !parent || !parent->HasFlag(NODE_IS_EDITABLE); > +} > + > +bool > +nsSVGElement::IsSVGFocusable(bool *aIsFocusable, int32_t *aTabIndex) Nit: move "*"s next to types. ::: dom/svg/nsSVGElement.cpp:1172 (Diff revision 5) > -} > + } > > + int32_t tabIndex = TabIndex(); > + bool disallowOverridingFocusability = true; > + > + if (IsEditableRoot()) { SVG elements don't support the contenteditable="" attribute, which means they can't be an editable root. So we can remove this branch, and the IsEditableRoot / GetContentEditableValue functions here. We can also then remove the disallowOverridingFocusability variable and just return false at the end of the function. ::: dom/svg/nsSVGElement.cpp:1194 (Diff revision 5) > + *aTabIndex = tabIndex; > + } > + > + // If a tabindex is specified at all, or the default tabindex is 0, we're focusable > + *aIsFocusable = > + ((tabIndex >= 0) || HasAttr(kNameSpaceID_None, nsGkAtoms::tabindex)); Nit: I'd drop the parens around the whole expression, and around (tabIndex >= 0).

Comment on attachment 8795681 [details] Bug 1302340 - Allow SVGElement to be focusable when tabindex is -1; https://reviewboard.mozilla.org/r/81648/#review110494

Pushed by dmu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/a20c22b2d924 Allow SVGElement to be focusable when tabindex is -1; r=heycam