Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(347)

Issue 166163005: [SVG2] Add tabindex handling in svg. (Closed)

Created:
6 years, 10 months ago by Erik Dahlström (inactive)
Modified:
6 years, 7 months ago
CC:
blink-reviews, arv+blink, watchdog-blink-watchlist_google.com, ed+blinkwatch_opera.com, f(malita), gyuyoung.kim_webkit.org, Inactive, Stephen Chennney, pdr.
Base URL:
https://chromium.googlesource.com/chromium/blink.git@master
Visibility:
Public.

Description

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

Patch Set 1 #

Patch Set 2 : reupload #

Patch Set 3 : rebased #

Patch Set 4 : rebased + add test rebaseline #

Total comments: 9

Patch Set 5 : review fs+chris #

Total comments: 4

Patch Set 6 : move code to Element #

Patch Set 7 : add pixeltest for outline #

Patch Set 8 : rebase #

Patch Set 9 : make some elements unfocusable, and add test #

Patch Set 10 : rebase #

Patch Set 11 : rebase #

Patch Set 12 : add focus ring reftest #

Patch Set 13 : let form controls override tabindex() #

Total comments: 4

Patch Set 14 : fix review issues #

Patch Set 15 : make SVGElement::supportsFocus return false #

Total comments: 1

Patch Set 16 : fix nit #

Patch Set 17 : rebase #

Unified diffs Side-by-side diffs Delta from patch set Stats (+491 lines, -109 lines) Patch
M LayoutTests/TestExpectations View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +4 lines, -0 lines 0 comments Download
A LayoutTests/fast/svg/tabindex-focus.html View 1 2 3 4 5 6 7 8 1 chunk +205 lines, -0 lines 0 comments Download
A LayoutTests/svg/custom/focus-ring-2.svg View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +6 lines, -0 lines 0 comments Download
A LayoutTests/svg/custom/focus-ring-2-expected.html View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +5 lines, -0 lines 0 comments Download
A LayoutTests/svg/custom/tabindex-order.html View 1 2 3 4 1 chunk +94 lines, -0 lines 0 comments Download
A LayoutTests/svg/custom/tabindex-order-expected.txt View 1 chunk +35 lines, -0 lines 0 comments Download
A + LayoutTests/svg/repaint/focus-element.html View 1 2 3 4 5 6 7 8 9 10 1 chunk +5 lines, -6 lines 0 comments Download
M Source/core/dom/Element.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 4 chunks +7 lines, -3 lines 0 comments Download
M Source/core/dom/Element.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 4 chunks +51 lines, -0 lines 0 comments Download
M Source/core/html/HTMLAnchorElement.h View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M Source/core/html/HTMLAnchorElement.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/html/HTMLAreaElement.h View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/html/HTMLAreaElement.cpp View 1 2 3 4 5 1 chunk +0 lines, -7 lines 0 comments Download
M Source/core/html/HTMLElement.h View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +0 lines, -4 lines 0 comments Download
M Source/core/html/HTMLElement.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 3 chunks +4 lines, -46 lines 0 comments Download
M Source/core/svg/SVGAElement.h View 1 2 3 4 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/svg/SVGAElement.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 3 chunks +27 lines, -7 lines 0 comments Download
M Source/core/svg/SVGCircleElement.h View 1 2 3 4 1 chunk +0 lines, -2 lines 0 comments Download
M Source/core/svg/SVGClipPathElement.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/svg/SVGDefsElement.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +2 lines, -1 line 0 comments Download
M Source/core/svg/SVGElement.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 2 chunks +5 lines, -1 line 0 comments Download
M Source/core/svg/SVGElement.cpp View 1 2 3 4 5 6 7 8 9 10 11 12 5 chunks +32 lines, -6 lines 0 comments Download
M Source/core/svg/SVGElement.idl View 1 2 3 4 5 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/svg/SVGEllipseElement.h View 1 2 3 4 1 chunk +0 lines, -2 lines 0 comments Download
M Source/core/svg/SVGGElement.h View 1 2 3 4 1 chunk +0 lines, -2 lines 0 comments Download
M Source/core/svg/SVGGraphicsElement.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +2 lines, -0 lines 0 comments Download
M Source/core/svg/SVGImageElement.h View 1 2 3 4 1 chunk +0 lines, -1 line 0 comments Download
M Source/core/svg/SVGLineElement.h View 1 2 3 4 5 1 chunk +0 lines, -2 lines 0 comments Download
M Source/core/svg/SVGPathElement.h View 1 2 3 4 5 1 chunk +0 lines, -2 lines 0 comments Download
M Source/core/svg/SVGPolyElement.h View 1 2 3 4 5 1 chunk +0 lines, -2 lines 0 comments Download
M Source/core/svg/SVGRectElement.h View 1 2 3 4 1 chunk +0 lines, -2 lines 0 comments Download
M Source/core/svg/SVGSVGElement.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +0 lines, -2 lines 0 comments Download
M Source/core/svg/SVGSwitchElement.h View 1 2 3 4 1 chunk +0 lines, -2 lines 0 comments Download
M Source/core/svg/SVGSymbolElement.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 1 chunk +0 lines, -2 lines 0 comments Download
M Source/core/svg/SVGTextElement.h View 1 2 3 4 1 chunk +0 lines, -2 lines 0 comments Download
M Source/core/svg/SVGUseElement.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +0 lines, -2 lines 0 comments Download

Messages

Total messages: 26 (0 generated)
Erik Dahlström (inactive)
6 years, 10 months ago (2014-02-14 16:36:58 UTC) #1
rwlbuis
On 2014/02/14 16:36:58, Erik Dahlström wrote: There are chunk mismatches, can you upload again?
6 years, 10 months ago (2014-02-14 16:57:34 UTC) #2
Erik Dahlström (inactive)
On 2014/02/14 16:57:34, rwlbuis wrote: > On 2014/02/14 16:36:58, Erik Dahlström wrote: > > There ...
6 years, 9 months ago (2014-03-14 09:06:33 UTC) #3
fs
https://codereview.chromium.org/166163005/diff/270001/LayoutTests/svg/custom/tabindex-order.html File LayoutTests/svg/custom/tabindex-order.html (right): https://codereview.chromium.org/166163005/diff/270001/LayoutTests/svg/custom/tabindex-order.html#newcode3 LayoutTests/svg/custom/tabindex-order.html:3: <script> Could maybe use js-test.js for slightly less boilerplate? ...
6 years, 9 months ago (2014-03-14 16:05:11 UTC) #4
Inactive
https://codereview.chromium.org/166163005/diff/270001/Source/core/svg/SVGAElement.h File Source/core/svg/SVGAElement.h (right): https://codereview.chromium.org/166163005/diff/270001/Source/core/svg/SVGAElement.h#newcode54 Source/core/svg/SVGAElement.h:54: virtual bool canStartSelection() const OVERRIDE FINAL; FINAL is redundant ...
6 years, 9 months ago (2014-03-17 00:19:26 UTC) #5
Erik Dahlström (inactive)
https://codereview.chromium.org/166163005/diff/270001/LayoutTests/svg/custom/tabindex-order.html File LayoutTests/svg/custom/tabindex-order.html (right): https://codereview.chromium.org/166163005/diff/270001/LayoutTests/svg/custom/tabindex-order.html#newcode3 LayoutTests/svg/custom/tabindex-order.html:3: <script> On 2014/03/14 16:05:12, fs wrote: > Could maybe ...
6 years, 9 months ago (2014-03-17 14:45:29 UTC) #6
fs
On 2014/03/17 14:45:29, Erik Dahlström wrote: > https://codereview.chromium.org/166163005/diff/270001/LayoutTests/svg/custom/tabindex-order.html > File LayoutTests/svg/custom/tabindex-order.html (right): > > https://codereview.chromium.org/166163005/diff/270001/LayoutTests/svg/custom/tabindex-order.html#newcode3 ...
6 years, 9 months ago (2014-03-17 14:53:08 UTC) #7
pdr.
In general this looks pretty good. Is it possible to add a pixel test that ...
6 years, 9 months ago (2014-03-24 16:29:53 UTC) #8
Stephen Chennney
Just one question. And I second pdr's comment about moving some things common with HTML ...
6 years, 9 months ago (2014-03-24 17:43:49 UTC) #9
Erik Dahlström (inactive)
On 2014/03/24 16:29:53, pdr wrote: > In general this looks pretty good. > > Is ...
6 years, 9 months ago (2014-03-25 08:23:48 UTC) #10
Erik Dahlström (inactive)
https://codereview.chromium.org/166163005/diff/290001/Source/core/svg/SVGElement.cpp File Source/core/svg/SVGElement.cpp (right): https://codereview.chromium.org/166163005/diff/290001/Source/core/svg/SVGElement.cpp#newcode647 Source/core/svg/SVGElement.cpp:647: bool SVGElement::supportsFocus() const On 2014/03/24 16:29:54, pdr wrote: > ...
6 years, 9 months ago (2014-03-25 08:24:02 UTC) #11
krit
What about <defs>, <pattern>, <linearGradient>, <radialGradient>, <cursor>, <mask>, <filter>, <fe*>, <font>, <glyph>, <symbol> and <marker>? ...
6 years, 8 months ago (2014-04-02 07:58:02 UTC) #12
Erik Dahlström (inactive)
On 2014/04/02 07:58:02, krit wrote: > What about <defs>, <pattern>, <linearGradient>, <radialGradient>, <cursor>, > <mask>, ...
6 years, 8 months ago (2014-04-02 14:35:30 UTC) #13
fs
I almost wonder if it wouldn't end up being nicer with SVGElement::supportsFocus returning false, and ...
6 years, 7 months ago (2014-04-29 08:30:30 UTC) #14
Erik Dahlström (inactive)
> I almost wonder if it wouldn't end up being nicer with SVGElement::supportsFocus > returning ...
6 years, 7 months ago (2014-04-29 12:12:27 UTC) #15
fs
LGTM w/ nit. https://codereview.chromium.org/166163005/diff/490001/Source/core/svg/SVGAElement.cpp File Source/core/svg/SVGAElement.cpp (right): https://codereview.chromium.org/166163005/diff/490001/Source/core/svg/SVGAElement.cpp#newcode202 Source/core/svg/SVGAElement.cpp:202: return SVGElement::supportsFocus(); Nit: Element::supportsFocus?
6 years, 7 months ago (2014-04-29 12:30:16 UTC) #16
Erik Dahlström (inactive)
The CQ bit was checked by ed@opera.com
6 years, 7 months ago (2014-04-29 12:40:41 UTC) #17
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ed@opera.com/166163005/510001
6 years, 7 months ago (2014-04-29 12:40:44 UTC) #18
Erik Dahlström (inactive)
The CQ bit was unchecked by ed@opera.com
6 years, 7 months ago (2014-04-29 15:18:40 UTC) #19
Erik Dahlström (inactive)
The CQ bit was checked by ed@opera.com
6 years, 7 months ago (2014-04-29 15:18:43 UTC) #20
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ed@opera.com/166163005/510001
6 years, 7 months ago (2014-04-29 15:19:01 UTC) #21
commit-bot: I haz the power
The CQ bit was unchecked by commit-bot@chromium.org
6 years, 7 months ago (2014-04-29 20:14:44 UTC) #22
commit-bot: I haz the power
Failed to apply patch for LayoutTests/TestExpectations: While running patch -p1 --forward --force --no-backup-if-mismatch; patching file ...
6 years, 7 months ago (2014-04-29 20:14:45 UTC) #23
Erik Dahlström (inactive)
The CQ bit was checked by ed@opera.com
6 years, 7 months ago (2014-04-30 07:04:45 UTC) #24
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ed@opera.com/166163005/520001
6 years, 7 months ago (2014-04-30 07:05:11 UTC) #25
commit-bot: I haz the power
6 years, 7 months ago (2014-04-30 09:17:48 UTC) #26
Message was sent while issue was closed.
Change committed as 172964

Powered by Google App Engine
This is Rietveld 408576698