[SVG2] Add tabindex handling in svg.

Add tabindex handling in svg, by reusing code from html.

Besides adding the tabindex attribute also reflect it to the tabIndex IDL attribute on the SVGElement interface.

Spec: https://svgwg.org/svg2-draft/interact.html#sequential-focus-navigation-and-the-tabindex-attribute
IDL attribute: https://svgwg.org/svg2-draft/types.html#__svg__SVGElement__tabIndex

BUG=352175
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=172964

[rwlbuis, 2014-02-14 16:57:34]

On 2014/02/14 16:36:58, Erik Dahlström wrote:

There are chunk mismatches, can you upload again?

[Erik Dahlström (inactive), 2014-03-14 09:06:33]

On 2014/02/14 16:57:34, rwlbuis wrote:
> On 2014/02/14 16:36:58, Erik Dahlström wrote:
> 
> There are chunk mismatches, can you upload again?

Done.

[fs, 2014-03-14 16:05:11]

https://codereview.chromium.org/166163005/diff/270001/LayoutTests/svg/custom/...
File LayoutTests/svg/custom/tabindex-order.html (right):

https://codereview.chromium.org/166163005/diff/270001/LayoutTests/svg/custom/...
LayoutTests/svg/custom/tabindex-order.html:3: <script>
Could maybe use js-test.js for slightly less boilerplate?

https://codereview.chromium.org/166163005/diff/270001/Source/core/svg/SVGAEle...
File Source/core/svg/SVGAElement.cpp (right):

https://codereview.chromium.org/166163005/diff/270001/Source/core/svg/SVGAEle...
Source/core/svg/SVGAElement.cpp:217: return
document().frameHost()->chrome().client().tabsToLinks();
There's a slight discrepancy here with how HTML seem to handle - HTMLElement
only return false if tabsToLinks() return false. I suppose that means that if
you set tabindex==-1 it'll still be considered focusable (and be part of the
tab-chain)?

https://codereview.chromium.org/166163005/diff/270001/Source/core/svg/SVGElem...
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/166163005/diff/270001/Source/core/svg/SVGElem...
Source/core/svg/SVGElement.cpp:649: return inDocument() && supportsFocus() &&
!isInert() && Element::tabIndex() >= 0;
IIUC, they way this is called the the first three will always be true? Also,
should 'visibility' be considered (call to tjhe base-class implementation)?

[Inactive, 2014-03-17 00:19:26]

https://codereview.chromium.org/166163005/diff/270001/Source/core/svg/SVGAEle...
File Source/core/svg/SVGAElement.h (right):

https://codereview.chromium.org/166163005/diff/270001/Source/core/svg/SVGAEle...
Source/core/svg/SVGAElement.h:54: virtual bool canStartSelection() const
OVERRIDE FINAL;
FINAL is redundant since the class is already marked as FINAL

https://codereview.chromium.org/166163005/diff/270001/Source/core/svg/SVGAEle...
Source/core/svg/SVGAElement.h:55: virtual short tabIndex() const OVERRIDE FINAL;
ditto

[Erik Dahlström (inactive), 2014-03-17 14:45:29]

https://codereview.chromium.org/166163005/diff/270001/LayoutTests/svg/custom/...
File LayoutTests/svg/custom/tabindex-order.html (right):

https://codereview.chromium.org/166163005/diff/270001/LayoutTests/svg/custom/...
LayoutTests/svg/custom/tabindex-order.html:3: <script>
On 2014/03/14 16:05:12, fs wrote:
> Could maybe use js-test.js for slightly less boilerplate?

This is based on LayoutTests/fast/html/tab-order.html. I wanted to keep them
similar, but I guess they could both be rewritten.

https://codereview.chromium.org/166163005/diff/270001/Source/core/svg/SVGAEle...
File Source/core/svg/SVGAElement.h (right):

https://codereview.chromium.org/166163005/diff/270001/Source/core/svg/SVGAEle...
Source/core/svg/SVGAElement.h:54: virtual bool canStartSelection() const
OVERRIDE FINAL;
On 2014/03/17 00:19:26, Chris Dumez wrote:
> FINAL is redundant since the class is already marked as FINAL

Done.

https://codereview.chromium.org/166163005/diff/270001/Source/core/svg/SVGAEle...
Source/core/svg/SVGAElement.h:55: virtual short tabIndex() const OVERRIDE FINAL;
On 2014/03/17 00:19:26, Chris Dumez wrote:
> ditto

Done.

https://codereview.chromium.org/166163005/diff/270001/Source/core/svg/SVGElem...
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/166163005/diff/270001/Source/core/svg/SVGElem...
Source/core/svg/SVGElement.cpp:649: return inDocument() && supportsFocus() &&
!isInert() && Element::tabIndex() >= 0;
On 2014/03/14 16:05:12, fs wrote:
> IIUC, they way this is called the the first three will always be true? Also,
> should 'visibility' be considered (call to tjhe base-class implementation)?

Hmm, true. I'll remove this.

visibility:hidden on a div means not focusable, even if tabindex > 0 is
specified, I think we want the same in svg too.

[fs, 2014-03-17 14:53:08]

On 2014/03/17 14:45:29, Erik Dahlström wrote:
>
https://codereview.chromium.org/166163005/diff/270001/LayoutTests/svg/custom/...
> File LayoutTests/svg/custom/tabindex-order.html (right):
> 
>
https://codereview.chromium.org/166163005/diff/270001/LayoutTests/svg/custom/...
> LayoutTests/svg/custom/tabindex-order.html:3: <script>
> On 2014/03/14 16:05:12, fs wrote:
> > Could maybe use js-test.js for slightly less boilerplate?
> 
> This is based on LayoutTests/fast/html/tab-order.html. I wanted to keep them
> similar, but I guess they could both be rewritten.

Ok. Fair enough to keep it as such then I suppose.

[pdr., 2014-03-24 16:29:53]

In general this looks pretty good.

Is it possible to add a pixel test that the focus ring (soft blue outline on
most platforms) draws correctly?

https://codereview.chromium.org/166163005/diff/290001/Source/core/svg/SVGElem...
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/166163005/diff/290001/Source/core/svg/SVGElem...
Source/core/svg/SVGElement.cpp:647: bool SVGElement::supportsFocus() const
There's a lot of duplicate and non-obvious logic from HTMLElement here. For
example, supportsFocus, supportsSpatialNavigationFocus, and the clamping logic
in parseAttribute. Can you extract this logic into a common place?

This code may one day end up on Element. It may be worth considering moving it
there, or making that future transition as painless as we can.

[Stephen Chennney, 2014-03-24 17:43:49]

Just one question. And I second pdr's comment about moving some things common
with HTML to Element.

https://codereview.chromium.org/166163005/diff/290001/Source/core/svg/SVGElem...
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/166163005/diff/290001/Source/core/svg/SVGElem...
Source/core/svg/SVGElement.cpp:675: ||
hasEventListeners(EventTypeNames::focusout);
Is this set spec defined? Do we need touch events or others that are typically
used for selecting something?

[Erik Dahlström (inactive), 2014-03-25 08:23:48]

On 2014/03/24 16:29:53, pdr wrote:
> In general this looks pretty good.
> 
> Is it possible to add a pixel test that the focus ring (soft blue outline on
> most platforms) draws correctly?

I can try to make it a reftest based on comparing the rendering to html, as I
presume there's already a pixel test to verify that platforms render the focus
ring correctly in html.

[Erik Dahlström (inactive), 2014-03-25 08:24:02]

https://codereview.chromium.org/166163005/diff/290001/Source/core/svg/SVGElem...
File Source/core/svg/SVGElement.cpp (right):

https://codereview.chromium.org/166163005/diff/290001/Source/core/svg/SVGElem...
Source/core/svg/SVGElement.cpp:647: bool SVGElement::supportsFocus() const
On 2014/03/24 16:29:54, pdr wrote:
> There's a lot of duplicate and non-obvious logic from HTMLElement here. For
> example, supportsFocus, supportsSpatialNavigationFocus, and the clamping logic
> in parseAttribute. Can you extract this logic into a common place?
> 
> This code may one day end up on Element. It may be worth considering moving it
> there, or making that future transition as painless as we can.

I'll try moving it up to Element, np.

https://codereview.chromium.org/166163005/diff/290001/Source/core/svg/SVGElem...
Source/core/svg/SVGElement.cpp:675: ||
hasEventListeners(EventTypeNames::focusout);
On 2014/03/24 17:43:50, Stephen Chenney wrote:
> Is this set spec defined? Do we need touch events or others that are typically
> used for selecting something?

It's not spec-defined no. It is mostly based on what Opera Presto did, and it's
meant to detect common scenarios where navigation is possible (e.g custom
dropdown menus and such). This functionality requires
--enable-spatial-navigation to be passed in on the commandline, it's disabled by
default.

[krit, 2014-04-02 07:58:02]

What about <defs>, <pattern>, <linearGradient>, <radialGradient>, <cursor>,
<mask>, <filter>, <fe*>, <font>, <glyph>, <symbol> and <marker>? Would they get
focus as well? I don't think they should. IIRC the spec says presentation
elements which is just a subset of all SVG elements. Not sure about elements
like <foreignObject> or <svg> though.

[Erik Dahlström (inactive), 2014-04-02 14:35:30]

On 2014/04/02 07:58:02, krit wrote:
> What about <defs>, <pattern>, <linearGradient>, <radialGradient>, <cursor>,
> <mask>, <filter>, <fe*>, <font>, <glyph>, <symbol> and <marker>? Would they
get
> focus as well? I don't think they should. IIRC the spec says presentation
> elements which is just a subset of all SVG elements. Not sure about elements
> like <foreignObject> or <svg> though.

Marker maybe (assuming we get the SVG2 markers that can be interactive). The
rest probably not.

I think <svg> should be fine if we allow <g> to be focusable when tabindex is
set appropriately. ForeignObject maybe, but probably not.

I'll make some jsharness-based testcases for checking this.

[fs, 2014-04-29 08:30:30]

I almost wonder if it wouldn't end up being nicer with SVGElement::supportsFocus
returning false, and then having the relevant subclasses (SVGGraphicsElement
mainly) bypass that. I guess we don't need to do that here and now though.

https://codereview.chromium.org/166163005/diff/450001/Source/core/dom/Element.h
File Source/core/dom/Element.h (right):

https://codereview.chromium.org/166163005/diff/450001/Source/core/dom/Element...
Source/core/dom/Element.h:539: bool supportsSpatialNavigationFocus() const;
Shouldn't this be virtual? (Used in Element::supportsFocus and overridden by
SVGElement.)

https://codereview.chromium.org/166163005/diff/450001/Source/core/html/HTMLEl...
File Source/core/html/HTMLElement.cpp (right):

https://codereview.chromium.org/166163005/diff/450001/Source/core/html/HTMLEl...
Source/core/html/HTMLElement.cpp:309: if (isIdAttributeName(name) || name ==
classAttr || name == styleAttr || name == tabindexAttr)
Could we do what SVGElement does here, and just replace the "old" body with a
call to the superclass - and leave the status quo for the other attributes?

[Erik Dahlström (inactive), 2014-04-29 12:12:27]

> I almost wonder if it wouldn't end up being nicer with
SVGElement::supportsFocus
> returning false, and then having the relevant subclasses (SVGGraphicsElement
> mainly) bypass that.

I've done that now.

https://codereview.chromium.org/166163005/diff/450001/Source/core/dom/Element.h
File Source/core/dom/Element.h (right):

https://codereview.chromium.org/166163005/diff/450001/Source/core/dom/Element...
Source/core/dom/Element.h:539: bool supportsSpatialNavigationFocus() const;
On 2014/04/29 08:30:31, fs wrote:
> Shouldn't this be virtual? (Used in Element::supportsFocus and overridden by
> SVGElement.)

Done.

https://codereview.chromium.org/166163005/diff/450001/Source/core/html/HTMLEl...
File Source/core/html/HTMLElement.cpp (right):

https://codereview.chromium.org/166163005/diff/450001/Source/core/html/HTMLEl...
Source/core/html/HTMLElement.cpp:309: if (isIdAttributeName(name) || name ==
classAttr || name == styleAttr || name == tabindexAttr)
On 2014/04/29 08:30:31, fs wrote:
> Could we do what SVGElement does here, and just replace the "old" body with a
> call to the superclass - and leave the status quo for the other attributes? 

Must have made a mistake while rebasing, it's fixed now.

[fs, 2014-04-29 12:30:16]

LGTM w/ nit.

https://codereview.chromium.org/166163005/diff/490001/Source/core/svg/SVGAEle...
File Source/core/svg/SVGAElement.cpp (right):

https://codereview.chromium.org/166163005/diff/490001/Source/core/svg/SVGAEle...
Source/core/svg/SVGAElement.cpp:202: return SVGElement::supportsFocus();
Nit: Element::supportsFocus?

[Erik Dahlström (inactive), 2014-04-29 12:40:41]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2014-04-29 12:40:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ed@opera.com/166163005/510001

[Erik Dahlström (inactive), 2014-04-29 15:18:40]

The CQ bit was unchecked by ed@opera.com

[Erik Dahlström (inactive), 2014-04-29 15:18:43]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2014-04-29 15:19:01]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ed@opera.com/166163005/510001

[commit-bot: I haz the power, 2014-04-29 20:14:44]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-29 20:14:45]

Failed to apply patch for LayoutTests/TestExpectations:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file LayoutTests/TestExpectations
  Hunk #1 FAILED at 1138.
  1 out of 1 hunk FAILED -- saving rejects to file
LayoutTests/TestExpectations.rej

Patch:       LayoutTests/TestExpectations
Index: LayoutTests/TestExpectations
diff --git a/LayoutTests/TestExpectations b/LayoutTests/TestExpectations
index
a14e15e37e9a408a188517cf174cbec7fa040b48..85e0b037dbfa01345e82e9fe565ae81d69e228d6
100644
--- a/LayoutTests/TestExpectations
+++ b/LayoutTests/TestExpectations
@@ -1138,3 +1138,7 @@ crbug.com/367846
svg/transforms/animated-path-inside-transformed-html.xhtml [ Pa
 crbug.com/367903 web-animations-api/timeline-time.html [ Pass Failure ]
 
 crbug.com/169282
virtual/threaded/compositing/visibility/visibility-image-layers.html [
NeedsRebaseline ]
+
+# Text selections on svg:a is no longer possible, same as for html:a, we no
longer get a caret in this test:
+crbug.com/352175 svg/custom/linking-uri-01-b.svg [ NeedsRebaseline ]
+crbug.com/352175 svg/repaint/focus-element.html [ NeedsRebaseline ]

[Erik Dahlström (inactive), 2014-04-30 07:04:45]

The CQ bit was checked by ed@opera.com

[commit-bot: I haz the power, 2014-04-30 07:05:11]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ed@opera.com/166163005/520001

[commit-bot: I haz the power, 2014-04-30 09:17:48]

Change committed as 172964