Consider slots' tabindex in focus navigation

Consider slots' tabindex in focus navigation

This CL changes focus navigation with slots which have tabindex; the tab indices no longer be ignored but be considered.
See the spec here: http://w3c.github.io/webcomponents/spec/shadow/#sequential-focus-navigation

BUG=599388
Committed: https://crrev.com/e9f6cc8df5886e724680b9141fbbdebdaabc3d2b
Cr-Commit-Position: refs/heads/master@{#384516}

[yuzuchan, 2016-03-31 09:24:12]

Description was changed from

==========
Consider slots' tabindex in focus navigation

This CL changes focus navigation with slots which have tabindex; the tab indices
no longer be ignored but be considered.

BUG=599388
==========

to

==========
Consider slots' tabindex in focus navigation

This CL changes focus navigation with slots which have tabindex; the tab indices
no longer be ignored but be considered.
See the spec here:
http://w3c.github.io/webcomponents/spec/shadow/#sequential-focus-navigation

BUG=599388
==========

[yuzuchan, 2016-03-31 09:24:13]

yuzus@chromium.org changed reviewers:
+ hayato@chromium.org, kochi@chromium.org

[yuzuchan, 2016-03-31 09:28:09]

PTAL

[yuzuchan, 2016-04-01 05:14:36]

PTAL :) 
This is how I feel about this patch: http://i.imgur.com/T7Mvlqx.gif

[hayato, 2016-04-01 05:56:57]

lgtm

https://codereview.chromium.org/1848753002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (right):

https://codereview.chromium.org/1848753002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:286: 
Two empty lines -> One empty line.

https://codereview.chromium.org/1848753002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/1848753002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.cpp:340: return
isNonKeyboardFocusableShadowHost(element) ||
isShadowInsertionPointFocusScopeOwner(element) ? 0 : element.tabIndex();
It would be better to use parenthesis.

return (isNonKeyboardFocusableShadowHost(element) ||
isShadowInsertionPointFocusScopeOwner(element)) ? 0 : element.tabIndex();

[yuzuchan, 2016-04-01 06:06:55]

The CQ bit was checked by yuzus@chromium.org

[yuzuchan, 2016-04-01 06:06:56]

This is how I feel about this patch: http://i.imgur.com/GLRqlTr.gif

https://codereview.chromium.org/1848753002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (right):

https://codereview.chromium.org/1848753002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:286: 
On 2016/04/01 at 05:56:57, hayato wrote:
> Two empty lines -> One empty line.

Done.

https://codereview.chromium.org/1848753002/diff/20001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/page/FocusController.cpp (right):

https://codereview.chromium.org/1848753002/diff/20001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/page/FocusController.cpp:340: return
isNonKeyboardFocusableShadowHost(element) ||
isShadowInsertionPointFocusScopeOwner(element) ? 0 : element.tabIndex();
On 2016/04/01 at 05:56:57, hayato wrote:
> It would be better to use parenthesis.
> 
> return (isNonKeyboardFocusableShadowHost(element) ||
isShadowInsertionPointFocusScopeOwner(element)) ? 0 : element.tabIndex();

Done.

[yuzuchan, 2016-04-01 06:06:56]

The patchset sent to the CQ was uploaded after l-g-t-m from hayato@chromium.org
Link to the patchset: https://codereview.chromium.org/1848753002/#ps40001
(title: "Refactor")

[commit-bot: I haz the power, 2016-04-01 06:07:21]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1848753002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1848753002/40001

[kochi, 2016-04-01 06:22:44]

Sorry for being late, lgtm

https://codereview.chromium.org/1848753002/diff/40001/third_party/WebKit/Sour...
File third_party/WebKit/Source/core/html/HTMLSlotElement.cpp (right):

https://codereview.chromium.org/1848753002/diff/40001/third_party/WebKit/Sour...
third_party/WebKit/Source/core/html/HTMLSlotElement.cpp:283: {
Maybe insert the same comment as HTMLAnchorElement::tabIndex() is useful
to explain why.
// Skip the supportsFocus check in HTMLElement.

[commit-bot: I haz the power, 2016-04-01 08:03:59]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-01 08:06:00]

Description was changed from

==========
Consider slots' tabindex in focus navigation

This CL changes focus navigation with slots which have tabindex; the tab indices
no longer be ignored but be considered.
See the spec here:
http://w3c.github.io/webcomponents/spec/shadow/#sequential-focus-navigation

BUG=599388
==========

to

==========
Consider slots' tabindex in focus navigation

This CL changes focus navigation with slots which have tabindex; the tab indices
no longer be ignored but be considered.
See the spec here:
http://w3c.github.io/webcomponents/spec/shadow/#sequential-focus-navigation

BUG=599388

Committed: https://crrev.com/e9f6cc8df5886e724680b9141fbbdebdaabc3d2b
Cr-Commit-Position: refs/heads/master@{#384516}
==========

[commit-bot: I haz the power, 2016-04-01 08:06:01]

Patchset 3 (id:??) landed as
https://crrev.com/e9f6cc8df5886e724680b9141fbbdebdaabc3d2b
Cr-Commit-Position: refs/heads/master@{#384516}