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: Create a SVG link element. http://jsbin.com/kuziwipehe/edit?html,css,output Actual results: * the focus outline of the link is a dot in the upper left corner * the tabIndex property provides value -1 Expected results: * the focus outline of the link is around the link's children's outline * the tabIndex property provides value 0

I notice it can be fixed by removing our old override function in SVGAElement (https://dxr.mozilla.org/mozilla-central/rev/c55bcb7c777ea09431b4d16903ed079ae5632648/dom/svg/SVGAElement.cpp#193). We should just use nsSVGElement::IsFocusableInternal(). Another thing we need to follow up is why text's bounding box in SVG link element is wrong, but SVG geometries are good.

(In reply to Daosheng Mu[:daoshengmu] from comment #1) > I notice it can be fixed by removing our old override function in > SVGAElement > (https://dxr.mozilla.org/mozilla-central/rev/ > c55bcb7c777ea09431b4d16903ed079ae5632648/dom/svg/SVGAElement.cpp#193). We > should just use nsSVGElement::IsFocusableInternal(). > > Another thing we need to follow up is why text's bounding box in SVG link > element is wrong, but SVG geometries are good. In the previous version, we expect all frame SVG container types could have invalid bounding boxes, but int the case of SVGTextFrame, it is valid. So, we should add an exception for SVGTextFrame at aOutValid = !aFrame->IsFrameOfType(nsIFrame::eSVGContainer); I also add a reftest for SVGLinkElement to help us detect some issues like this.

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

Comment on attachment 8795590 [details] Bug 1302341 - Part 1: SVGAElement focus checks tabIndex first and to be 0 by default; https://reviewboard.mozilla.org/r/81590/#review80558 ::: dom/svg/SVGAElement.cpp (Diff revision 1) > > bool > -SVGAElement::IsFocusableInternal(int32_t *aTabIndex, bool aWithMouse) > -{ > - nsCOMPtr<nsIURI> uri; > - if (IsLink(getter_AddRefs(uri))) { I don't understand why removing this method fixes the bug. Can you explain? Shouldn't we still be calling IsLink() at some point, to influence whether the default tabindex (when the attribute is not specified) is 0 or -1 depending on whether eTabFocus_linksMask is set? (That flag is, by default, not set on macOS.) I think we need to have a TabIndexDefault() implementation that returns 0, like HTMLAnchorElement does, so that in JS mySVGAElement.tabIndex returns 0 for an <a> element with no tabindex attribute.

Comment on attachment 8795591 [details] Bug 1302341 - Part 2: SVGTextFrame should be valid when unioning borderBoxes; https://reviewboard.mozilla.org/r/81592/#review80568

(In reply to Cameron McCormack (:heycam) from comment #6) > Comment on attachment 8795590 [details] > Bug 1302341 - Part 1: Remove SVG link element IsFocusableInternal override; > > https://reviewboard.mozilla.org/r/81590/#review80558 > > ::: dom/svg/SVGAElement.cpp > (Diff revision 1) > > > > bool > > -SVGAElement::IsFocusableInternal(int32_t *aTabIndex, bool aWithMouse) > > -{ > > - nsCOMPtr<nsIURI> uri; > > - if (IsLink(getter_AddRefs(uri))) { > > I don't understand why removing this method fixes the bug. Can you explain? You are right, I should keep it. > Shouldn't we still be calling IsLink() at some point, to influence whether > the default tabindex (when the attribute is not specified) is 0 or -1 > depending on whether eTabFocus_linksMask is set? (That flag is, by default, > not set on macOS.) > I think the problem is why sTabFocusModel not works on macOS. sTabFocusModel is 1 here, and it is expected to be eTabFocus_any. And other platforms are available as Bug1303681 Comment 3. Do you think it is strange? Should I investigate it? > I think we need to have a TabIndexDefault() implementation that returns 0, > like HTMLAnchorElement does, so that in JS mySVGAElement.tabIndex returns 0 > for an <a> element with no tabindex attribute. It sounds SVGAElement is special to other SVGElements. If SVGAElement was expected to be focusable by TAB, we should let it return 0 by default.

Btw, if i change the order of if (IsLink(getter_AddRefs(uri))) { if (aTabIndex) { *aTabIndex = ((sTabFocusModel & eTabFocus_linksMask) == 0 ? -1 : 0); } return true; } and if (nsSVGElement::IsFocusableInternal(aTabIndex, aWithMouse)) { return true; } It will work.

As I don't understand what that code really does, here's the expected behavior: <a xlink:href="..."> should have .tabIndex = 0 (and be sorted in DOM order in the document's tab focus order) <a xlink:href="..." tabindex="1"> should have .tabIndex = 1 (and be sorted accordingly in the document's tab focus order) <a xlink:href="..." tabindex="-1"> should have .tabIndex = -1 (and be removed from the document's tab focus order, but remain focusable to mouse and script) <rect ...> should have .tabIndex = -1 (and be inert) <rect ... tabindex="0"> should have .tabIndex = 0 (and be sorted in DOM order in the document's tab focus order) <rect ... tabindex="1"> should have .tabIndex = 1 (and be sorted accordingly in the document's tab focus order) <rect ... tabindex="-1"> should have .tabIndex = -1 (and be removed from the document's tab focus order, but remain focusable to mouse and script)

Try looks good, https://treeherder.mozilla.org/#/jobs?repo=try&revision=236947ee8849 In this update, I let check nsSVGElement::IsFocusableInternal() before checking sTabFocusModel in SVGAElement::IsFocusableInternal. Because sTabFocusModel does not work on macOS. This refers to HTMLAnchorElement::IsHTMLFocusable() that is the same case in HTMLDocument. Moreover, I add a test at test_tabindex.html to confirm SVGAElement is 0 by default and can be focused.

After rebasing, try looks good https://treeherder.mozilla.org/#/jobs?repo=try&revision=ceb2207285ae88c46eef5b78ae608ce942835610.

Try looks good again, https://treeherder.mozilla.org/#/jobs?repo=try&revision=71a878e648c86ae4a4278b20692b99fdb498d4e3. These patches depend on Bug 1302340, please land Bug 1302340 first.

Comment on attachment 8795590 [details] Bug 1302341 - Part 1: SVGAElement focus checks tabIndex first and to be 0 by default; https://reviewboard.mozilla.org/r/81590/#review110496 r=me with the added sub-test. ::: dom/svg/SVGAElement.h:54 (Diff revision 4) > bool aCompileEventHandlers) override; > virtual void UnbindFromTree(bool aDeep = true, > bool aNullParent = true) override; > NS_IMETHOD_(bool) IsAttributeMapped(const nsIAtom* aAttribute) const override; > - virtual bool IsFocusableInternal(int32_t* aTabIndex, bool aWithMouse) override; > + virtual int32_t TabIndexDefault() override; > + virtual bool IsSVGFocusable(bool *aIsFocusable, int32_t *aTabIndex) override; Nit: "*"s next to types. ::: dom/svg/SVGAElement.cpp:209 (Diff revision 4) > + } > + return false; > +} > + > +bool > +SVGAElement::IsSVGFocusable(bool *aIsFocusable, int32_t *aTabIndex) Nit: "*"s next to types. ::: dom/svg/test/test_tabindex.html:21 (Diff revision 4) > + <a xlink:href="#" id="l" tabindex="3"> > + <circle cx="10" cy="230" r="10"/> > + </a> Can you add another <a> element here without an xlink:href="" attribute, so that we can test the "non-link <a>" case?

Pushed by dmu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/8e1741d22ce0 Part 1: SVGAElement focus checks tabIndex first and to be 0 by default; r=heycam https://hg.mozilla.org/integration/autoland/rev/8f2925fcd9ff Part 2: SVGTextFrame should be valid when unioning borderBoxes; r=heycam

https://hg.mozilla.org/mozilla-central/rev/8e1741d22ce0 https://hg.mozilla.org/mozilla-central/rev/8f2925fcd9ff