SVG elements and negative tabindex

RESOLVED FIXED in Firefox 54

Status

()

RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mail, Assigned: daoshengmu)

Tracking

51 Branch
mozilla54
Points:
---

Firefox Tracking Flags

(firefox54 fixed)

Details

Attachments

(1 attachment)

(Reporter)

Description

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

Updated

2 years ago
Depends on: 778654
Component: Untriaged → SVG
Product: Firefox → Core
(Assignee)

Comment 1

2 years ago
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.
Comment hidden (mozreview-request)
(Assignee)

Comment 3

2 years ago
Try looks good, https://treeherder.mozilla.org/#/jobs?repo=try&revision=b58de85a9fa4.
Assignee: nobody → dmu
Status: UNCONFIRMED → NEW
Ever confirmed: true
Comment hidden (mozreview-request)
(Assignee)

Comment 5

2 years ago
(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.
Comment hidden (mozreview-request)
(Reporter)

Comment 7

2 years ago
(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.
(Assignee)

Comment 8

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

Comment 9

2 years ago
(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 hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 13

2 years ago
mozreview-review
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 14

2 years ago
mozreview-review
Comment on attachment 8795681 [details]
Bug 1302340 - Allow SVGElement to be focusable when tabindex is -1;

https://reviewboard.mozilla.org/r/81648/#review110494
Attachment #8795681 - Flags: review?(cam) → review+
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 17

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

Comment 18

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/a20c22b2d924
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox54: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
You need to log in before you can comment on or make changes to this bug.