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

Issue 1707443003: Consider slots as a focus scope (Closed)

Created:
4 years, 10 months ago by yuzuchan
Modified:
4 years, 9 months ago
Reviewers:
hayato, kojii, kochi
CC:
Aaron Boodman, abarth-chromium, ben+mojo_chromium.org, blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, darin (slow to review), dglazkov+blink, eae+blinkwatch, kalyank, qsr+mojo_chromium.org, rjkroege, rwlbuis, sadrul, sof, viettrungluu+watch_chromium.org, yzshen+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@master
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Consider slots as a focus scope V1 Slots can now have their own focus scope just like shadow roots, so that the assigned elements would be traversed together, which is visually more consistent than the current implementation. This reflects the results of the discussion in TPAC. See the discussion here: https://github.com/w3c/webcomponents/issues/375 BUG=591909 Committed: https://crrev.com/856302237805d511bcc93ffc965b674406d7c80a Cr-Commit-Position: refs/heads/master@{#379523}

Patch Set 1 #

Patch Set 2 : Checkout unrelated changes #

Patch Set 3 : Refactoring #

Patch Set 4 : Refactoring related to OILPAN #

Patch Set 5 : Fix constructor usage #

Patch Set 6 : Refactoring related to OILPAN #

Patch Set 7 : Refactoring related to OILPAN #

Total comments: 1

Patch Set 8 : Implementing new traversal inside FocusController #

Total comments: 67

Patch Set 9 : Refactoring in response to review comments #

Total comments: 15

Patch Set 10 : Refactoring in response to review comments #

Patch Set 11 : Refactoring in response to review comments #

Total comments: 53

Patch Set 12 : Refactoring related to OILPAN #

Patch Set 13 : Refactoring related to OILPAN #

Patch Set 14 : Refactoring in response to review comments #

Total comments: 2

Patch Set 15 : Debug for negative tabindex #

Patch Set 16 : Refactoring in response to review comments #

Patch Set 17 : Modify layout test #

Patch Set 18 : Update moveToLast and LayoutTest result #

Patch Set 19 : Checkout unnecessasry changes #

Patch Set 20 : Rebase #

Patch Set 21 : Adding document distribution #

Patch Set 22 : Adding distribution recalc to document #

Patch Set 23 : Minor changes #

Patch Set 24 : Refactoring for loops #

Patch Set 25 : Refactoring for loops #

Total comments: 2

Patch Set 26 : Adding initializations of member variables #

Messages

Total messages: 32 (6 generated)
yuzuchan
PTAL
4 years, 10 months ago (2016-02-18 04:58:57 UTC) #3
hayato
Looks promising. Thank you! I'll review in detail soon, but at the first glance, we ...
4 years, 10 months ago (2016-02-18 08:02:38 UTC) #4
kochi
On 2016/02/18 08:02:38, hayato wrote: > Looks promising. Thank you! > > I'll review in ...
4 years, 10 months ago (2016-02-18 08:16:26 UTC) #5
hayato
I've done "Node to Element" refactoring for FocusController in the following series of CLs: - ...
4 years, 10 months ago (2016-02-19 08:42:41 UTC) #6
kochi
I'll look into this again once hayato's high-level comments are addressed.
4 years, 10 months ago (2016-02-22 08:29:15 UTC) #7
yuzuchan
PTAL
4 years, 9 months ago (2016-03-01 09:31:51 UTC) #8
kochi
I'll take further look tomorrow. https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Source/core/core.gypi File third_party/WebKit/Source/core/core.gypi (right): https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Source/core/core.gypi#newcode2294 third_party/WebKit/Source/core/core.gypi:2294: 'dom/AssignedElementTraversal.h', Maybe these files ...
4 years, 9 months ago (2016-03-01 12:55:41 UTC) #9
yuzuchan
Thank you for the review kochi@ san! https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Source/core/core.gypi File third_party/WebKit/Source/core/core.gypi (right): https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Source/core/core.gypi#newcode2294 third_party/WebKit/Source/core/core.gypi:2294: 'dom/AssignedElementTraversal.h', On ...
4 years, 9 months ago (2016-03-02 09:02:15 UTC) #10
kochi
https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp File third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp (right): https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp#newcode23 third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp:23: // assignedAncestor returns an ancestor element of current which ...
4 years, 9 months ago (2016-03-02 13:26:56 UTC) #11
kochi
https://codereview.chromium.org/1707443003/diff/160001/third_party/WebKit/Source/core/page/FocusController.cpp File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/1707443003/diff/160001/third_party/WebKit/Source/core/page/FocusController.cpp#newcode637 third_party/WebKit/Source/core/page/FocusController.cpp:637: toElement(node); On 2016/03/02 13:26:56, kochi wrote: > It looks ...
4 years, 9 months ago (2016-03-03 00:54:17 UTC) #12
yuzuchan
https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp File third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp (right): https://codereview.chromium.org/1707443003/diff/140001/third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp#newcode24 third_party/WebKit/Source/core/dom/AssignedElementTraversal.cpp:24: Element* element = const_cast<Element*>(&current); On 2016/03/02 13:26:56, kochi wrote: ...
4 years, 9 months ago (2016-03-03 04:12:43 UTC) #13
hayato
Yuzu, thank you for updating your CL. It looks great. The approach itself looks correct. ...
4 years, 9 months ago (2016-03-03 05:42:22 UTC) #14
yuzuchan
Thank you for review kochi san hayato san! https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/LayoutTests/fast/dom/shadow/focus-navigation-with-multiple-shadow-roots.html File third_party/WebKit/LayoutTests/fast/dom/shadow/focus-navigation-with-multiple-shadow-roots.html (right): https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/LayoutTests/fast/dom/shadow/focus-navigation-with-multiple-shadow-roots.html#newcode51 third_party/WebKit/LayoutTests/fast/dom/shadow/focus-navigation-with-multiple-shadow-roots.html:51: 'light-child-B', ...
4 years, 9 months ago (2016-03-04 03:48:38 UTC) #15
kochi
https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Source/core/core.gypi File third_party/WebKit/Source/core/core.gypi (right): https://codereview.chromium.org/1707443003/diff/200001/third_party/WebKit/Source/core/core.gypi#newcode2625 third_party/WebKit/Source/core/core.gypi:2625: 'dom/shadow/AssignedElementTraversal.h', On 2016/03/04 03:48:36, yuzuchan wrote: > On 2016/03/03 ...
4 years, 9 months ago (2016-03-04 04:16:30 UTC) #16
kochi
Filed crbug.com/591909 Could you update BUG= line in the description?
4 years, 9 months ago (2016-03-04 04:20:37 UTC) #17
yuzuchan
On 2016/03/04 at 04:20:37, kochi wrote: > Filed crbug.com/591909 > Could you update BUG= line ...
4 years, 9 months ago (2016-03-04 05:38:20 UTC) #19
yuzuchan
PTAL https://codereview.chromium.org/1707443003/diff/260001/third_party/WebKit/Source/core/dom/Node.cpp File third_party/WebKit/Source/core/dom/Node.cpp (right): https://codereview.chromium.org/1707443003/diff/260001/third_party/WebKit/Source/core/dom/Node.cpp#newcode2280 third_party/WebKit/Source/core/dom/Node.cpp:2280: return nullptr; On 2016/03/04 at 04:16:30, kochi wrote: ...
4 years, 9 months ago (2016-03-04 06:44:01 UTC) #20
kochi
almost look good, could you check oilpan bot failure?
4 years, 9 months ago (2016-03-04 08:42:40 UTC) #21
yuzuchan
On 2016/03/04 at 08:42:40, kochi wrote: > almost look good, could you check oilpan bot ...
4 years, 9 months ago (2016-03-04 11:42:18 UTC) #22
hayato
Yuzu, I think you could reproduce a crash in linux_blink_oilpan bot, as follows: 1. Go ...
4 years, 9 months ago (2016-03-07 03:38:32 UTC) #24
hayato
https://codereview.chromium.org/1707443003/diff/470001/third_party/WebKit/Source/core/page/FocusController.cpp File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/1707443003/diff/470001/third_party/WebKit/Source/core/page/FocusController.cpp#newcode101 third_party/WebKit/Source/core/page/FocusController.cpp:101: , m_current(const_cast<Element*>(current)) It looks all member variables will be ...
4 years, 9 months ago (2016-03-07 04:09:48 UTC) #25
yuzuchan
Thanks hayato-san for the detailed instructions, I think the oilpan bot failure is now resolved. ...
4 years, 9 months ago (2016-03-07 04:40:41 UTC) #26
hayato
Thank you. You can land this after confirming all bots become green (・∀・) LGTM
4 years, 9 months ago (2016-03-07 05:42:02 UTC) #27
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1707443003/490001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1707443003/490001
4 years, 9 months ago (2016-03-07 07:50:57 UTC) #29
commit-bot: I haz the power
Committed patchset #26 (id:490001)
4 years, 9 months ago (2016-03-07 07:55:48 UTC) #30
commit-bot: I haz the power
4 years, 9 months ago (2016-03-07 07:57:20 UTC) #32
Message was sent while issue was closed.
Patchset 26 (id:??) landed as
https://crrev.com/856302237805d511bcc93ffc965b674406d7c80a
Cr-Commit-Position: refs/heads/master@{#379523}

Powered by Google App Engine
This is Rietveld 408576698