Closed Bug 1302341 Opened 5 years ago Closed 5 years ago

SVG link element focus behavior

Categories

(Core :: SVG, defect)

51 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla54
Tracking Status
firefox54 --- fixed

People

(Reporter: mail, Assigned: daoshengmu)

References

Details

Attachments

(2 files)

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
Depends on: 778654
Component: Untriaged → SVG
Product: Firefox → Core
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.
Assignee: nobody → dmu
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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.
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
Attachment #8795591 - Flags: review?(cam) → review+
(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.
Flags: needinfo?(cam)
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.
See Also: → 1303681
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?
Attachment #8795590 - Flags: review?(cam) → review+
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
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Duplicate of this bug: 1303681
See Also: → 1415787
You need to log in before you can comment on or make changes to this bug.