Last Comment Bug 1302340 - SVG elements and negative tabindex
: SVG elements and negative tabindex
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: SVG (show other bugs)
: 51 Branch
: Unspecified Unspecified
-- normal (vote)
: mozilla54
Assigned To: Daosheng Mu[:daoshengmu]
:
: Sean Voisen (:svoisen)
Mentors:
Depends on: 778654
Blocks:
  Show dependency treegraph
 
Reported: 2016-09-12 23:53 PDT by Rodney Rehm
Modified: 2017-02-06 17:40 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Bug 1302340 - Allow SVGElement to be focusable when tabindex is -1; (58 bytes, text/x-review-board-request)
2016-09-28 05:05 PDT, Daosheng Mu[:daoshengmu]
heycam: review+
Details

Description User image Rodney Rehm 2016-09-12 23:53:00 PDT
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
Comment 1 User image Daosheng Mu[:daoshengmu] 2016-09-27 20:32:55 PDT
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 2 User image Daosheng Mu[:daoshengmu] 2016-09-28 05:05:28 PDT Comment hidden (mozreview-request)
Comment 3 User image Daosheng Mu[:daoshengmu] 2016-09-28 06:18:59 PDT
Try looks good, https://treeherder.mozilla.org/#/jobs?repo=try&revision=b58de85a9fa4.
Comment 4 User image Daosheng Mu[:daoshengmu] 2016-09-29 20:46:16 PDT Comment hidden (mozreview-request)
Comment 5 User image Daosheng Mu[:daoshengmu] 2016-09-29 20:51:08 PDT
(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 6 User image Daosheng Mu[:daoshengmu] 2016-09-29 20:53:04 PDT Comment hidden (mozreview-request)
Comment 7 User image Rodney Rehm 2016-10-05 04:40:33 PDT
(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.
Comment 8 User image Daosheng Mu[:daoshengmu] 2016-10-05 07:30:00 PDT
(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.
Comment 9 User image Rodney Rehm 2016-10-10 09:10:46 PDT
(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.
Comment 10 User image Robert Longson [:longsonr] 2016-10-10 14:50:14 PDT
(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 11 User image Daosheng Mu[:daoshengmu] 2017-01-22 23:44:02 PST Comment hidden (mozreview-request)
Comment 12 User image Daosheng Mu[:daoshengmu] 2017-02-02 06:19:07 PST Comment hidden (mozreview-request)
Comment 13 User image Cameron McCormack (:heycam) (away Jan 18) 2017-02-02 22:08:24 PST
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 User image Cameron McCormack (:heycam) (away Jan 18) 2017-02-02 22:08:34 PST
Comment on attachment 8795681 [details]
Bug 1302340 - Allow SVGElement to be focusable when tabindex is -1;

https://reviewboard.mozilla.org/r/81648/#review110494
Comment 15 User image Daosheng Mu[:daoshengmu] 2017-02-06 00:28:50 PST Comment hidden (mozreview-request)
Comment 16 User image Daosheng Mu[:daoshengmu] 2017-02-06 00:42:27 PST Comment hidden (mozreview-request)
Comment 17 User image Pulsebot 2017-02-06 00:47:44 PST
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 User image Wes Kocher (:KWierso) 2017-02-06 17:40:55 PST
https://hg.mozilla.org/mozilla-central/rev/a20c22b2d924

Note You need to log in before you can comment on or make changes to this bug.