Closed Bug 1302340 Opened 8 years ago Closed 7 years ago

SVG elements and negative tabindex

Categories

(Core :: SVG, defect)

Product:
Core
Component:
SVG
Version:
51 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla54
Tracking Flags:
Tracking Status
firefox54 --- fixed

People

(Reporter: mail, Assigned: daoshengmu)

References

Details

Attachments

(1 file)

Bug 1302340 - Allow SVGElement to be focusable when tabindex is -1;
58 bytes, text/x-review-board-request
heycam
: review+
Details 
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
Depends on: 778654
Component: Untriaged → SVG
Product: Firefox → Core
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.
Assignee: nobody → dmu
Status: UNCONFIRMED → NEW
Ever confirmed: true
(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
Attachment #8795681 - Flags: review?(cam) → review+
Pushed by dmu@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a20c22b2d924
Allow SVGElement to be focusable when tabindex is -1; r=heycam
https://hg.mozilla.org/mozilla-central/rev/a20c22b2d924
Status: NEW → RESOLVED
Closed: 7 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.

Attachment

General

Creator:
Created:
Updated:
Size: