Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1840333002:
Support slots' fallback contents in focus navigation (Closed)
|
Created:
4 years, 8 months ago by yuzuchan Modified:
4 years, 8 months ago CC:
blink-reviews, blink-reviews-dom_chromium.org, chromium-reviews, dglazkov+blink, eae+blinkwatch, rwlbuis, sof, webcomponents-bugzilla_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSupport slots' fallback contents in focus navigation
This CL implements slots' fallback contents in focus navigation.
That is, if a focus hits a slot without any assigned nodes, it traverses the slot's children as its fallback contents.
See the spec here: https://w3c.github.io/webcomponents/spec/shadow/#sequential-focus-navigation
BUG=595285
Committed: https://crrev.com/7d503c6b674b0a2e9e5059edd89b52b5412f20ec
Cr-Commit-Position: refs/heads/master@{#384540}
Patch Set 1 #
Total comments: 11
Patch Set 2 : Add testcases and debug #
Total comments: 6
Patch Set 3 : Refactor #
Total comments: 4
Patch Set 4 : Refactor #
Messages
Total messages: 18 (5 generated)
yuzus@chromium.org changed reviewers: + hayato@chromium.org, kochi@chromium.org
PTAL
On 2016/03/30 03:01:57, yuzuchan wrote: > PTAL Thanks, I'll take a look. The spec is upstreamed and now the official link is: https://w3c.github.io/webcomponents/spec/shadow/#sequential-focus-navigation Please update the link in the CL description.
Description was changed from ========== Support slots' fallback contents in focus navigation This CL implements slots' fallback contents in focus navigation. That is, if a focus hits a slot without any assigned nodes, it traverses the slot's children as its fallback contents. See the google docs for the detailed description here: https://docs.google.com/document/d/1mNV2Z231SrAZuMJvBVNfVNHfULkMwUZOZ75vZM1K9... BUG=595285 ========== to ========== Support slots' fallback contents in focus navigation This CL implements slots' fallback contents in focus navigation. That is, if a focus hits a slot without any assigned nodes, it traverses the slot's children as its fallback contents. See the spec here: https://w3c.github.io/webcomponents/spec/shadow/#sequential-focus-navigation BUG=595285 ==========
On 2016/03/30 at 03:41:32, kochi wrote: > On 2016/03/30 03:01:57, yuzuchan wrote: > > PTAL > > Thanks, I'll take a look. > > The spec is upstreamed and now the official link is: > https://w3c.github.io/webcomponents/spec/shadow/#sequential-focus-navigation > > Please update the link in the CL description. Thanks, I updated the description.
Overall the code look good. Let's iterate on it. https://codereview.chromium.org/1840333002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/shadow-dom/slot-fallback-focus.html (right): https://codereview.chromium.org/1840333002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/shadow-dom/slot-fallback-focus.html:31: </div> Can you add more test cases to be more comprehensive? e.g. * <slot>...fallback contents...<slot>...fallback2...</slot></slot> * <slot>...<shadow host>focusable elements to be slotted</shadow host>...</slot> etc. etc. https://codereview.chromium.org/1840333002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/shadow-dom/slot-fallback-focus.html:46: ]; Could you also check a case with the same tree structure, adding an focusable element (e.g. '<input slot="s1">' under shadow host then the fallback contents are ignored. https://codereview.chromium.org/1840333002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/shadow-dom/slot-fallback-focus.html:49: assert_true(shouldNavigateFocus(elements[i], elements[i + 1], 'forward'), elements[i]+" to "+ elements[i+1]); nit: use single quote for string literal and spaces around "+". https://codereview.chromium.org/1840333002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/shadow-dom/slot-fallback-focus.html:52: assert_true(shouldNavigateFocus(elements[i], elements[i + 1], 'backward'), elements[i]+" to "+ elements[i+1]); nit: ditto. https://codereview.chromium.org/1840333002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/1840333002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/page/FocusController.cpp:94: ScopedFocusNavigation(HTMLSlotElement&, const Element*, bool); Can the third boolean argument be removed? If the value is always initialized slotElement.getAssignedNodes().isEmpty(), how about doing it in this constructor? BTW, in Blink's style preference, a boolean parameter is discouraged and an enum constant is preferred for readability unless the function is single-parameter function (e.g. setFooFlag(bool) is okay, setFoo(Foo*, bool) is not good).
PTAL https://codereview.chromium.org/1840333002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/shadow-dom/slot-fallback-focus.html (right): https://codereview.chromium.org/1840333002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/shadow-dom/slot-fallback-focus.html:31: </div> On 2016/03/30 at 06:28:33, kochi wrote: > Can you add more test cases to be more comprehensive? > e.g. > * <slot>...fallback contents...<slot>...fallback2...</slot></slot> > * <slot>...<shadow host>focusable elements to be slotted</shadow host>...</slot> > > etc. etc. Thanks for the feedback! By adding the testcases suggested, I got to find a bug and fix it :) https://codereview.chromium.org/1840333002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/shadow-dom/slot-fallback-focus.html:46: ]; On 2016/03/30 at 06:28:34, kochi wrote: > Could you also check a case with the same tree structure, > adding an focusable element (e.g. '<input slot="s1">' under shadow host > then the fallback contents are ignored. Not with the same tree structure, but I added a focusable element inside slot-2, which has assigned elements, and made sure it is ignored. https://codereview.chromium.org/1840333002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/shadow-dom/slot-fallback-focus.html:49: assert_true(shouldNavigateFocus(elements[i], elements[i + 1], 'forward'), elements[i]+" to "+ elements[i+1]); On 2016/03/30 at 06:28:33, kochi wrote: > nit: use single quote for string literal and spaces around "+". Done. https://codereview.chromium.org/1840333002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/shadow-dom/slot-fallback-focus.html:52: assert_true(shouldNavigateFocus(elements[i], elements[i + 1], 'backward'), elements[i]+" to "+ elements[i+1]); On 2016/03/30 at 06:28:33, kochi wrote: > nit: ditto. Done. https://codereview.chromium.org/1840333002/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/1840333002/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/page/FocusController.cpp:94: ScopedFocusNavigation(HTMLSlotElement&, const Element*, bool); On 2016/03/30 at 06:28:34, kochi wrote: > Can the third boolean argument be removed? > If the value is always initialized slotElement.getAssignedNodes().isEmpty(), > how about doing it in this constructor? > > BTW, in Blink's style preference, a boolean parameter is discouraged and an enum > constant is preferred for readability unless the function is single-parameter > function (e.g. setFooFlag(bool) is okay, setFoo(Foo*, bool) is not good). Gotcha! Thank you :)
https://codereview.chromium.org/1840333002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/1840333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:130: if (m_slotFallbackTraversal) { I am afraid that the responsibilities of SlotScopedTraversal and ScopedFocusNavigation are getting unclear. - Now SlotScopedTraversal has findFallbackScopeOwnerSlot(). - But ScopedFocusNavagion::moveToNext is being updated here, instead of updating ScopedFocusTraversal::next(). If SlotScopedTraversal is only used for traversing *assigned nodes* (and its descendants), it would be better to avoid to define findFallbackScopeOwnerSlot() in SlotScopedTraversal. Instead, findFallbackScopeOwnerSlot() should be defined in ScopedFocusNavigation to make the responsibility clear. https://codereview.chromium.org/1840333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:132: while (m_current && (SlotScopedTraversal::isSlotScoped(*m_current) || SlotScopedTraversal::findFallbackScopeOwnerSlot(*m_current) != m_rootSlot)) > SlotScopedTraversal::isSlotScoped(*m_current) || SlotScopedTraversal::findFallbackScopeOwnerSlot(*m_current) != m_rootSlot It looks we have several similar patterns here. Could you extract this pattern as an utility function? e.g. bool ScopedFocusNavigation::isFallbackContent(HTMLSlotElement& slot, Element& current) { // Implement this. } Then, the loop would become more readable: while (m_current && !ScopedFocusNavigation::isFallbackContent(*m_rootSlot, *m_current)) { m_current = ElementTraversal::next(*m_current, m_rootSlot); } https://codereview.chromium.org/1840333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:234: if (SlotScopedTraversal::isSlotFallbackScoped(current)) It looks a little wasteful to do the same check twice here. This can be: if (HTMLSlotElement* slot = SlotScopedTraversal::findFallbackScopeOwnerSlot(current)) return ScopedFocusNavigation(*slot, ¤t); Can you do the similar thing to line 232-234?
PTAL This is how I feel about this patch: http://i.imgur.com/PaijGaC.gif http://i.imgur.com/N7oh7Um.gif http://i.imgur.com/s0l0hHR.gif https://codereview.chromium.org/1840333002/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/1840333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:130: if (m_slotFallbackTraversal) { On 2016/03/31 at 09:45:02, hayato wrote: > I am afraid that the responsibilities of SlotScopedTraversal and ScopedFocusNavigation are getting unclear. > > > - Now SlotScopedTraversal has findFallbackScopeOwnerSlot(). > - But ScopedFocusNavagion::moveToNext is being updated here, instead of updating ScopedFocusTraversal::next(). > > If SlotScopedTraversal is only used for traversing *assigned nodes* (and its descendants), > it would be better to avoid to define findFallbackScopeOwnerSlot() in SlotScopedTraversal. > > Instead, findFallbackScopeOwnerSlot() should be defined in ScopedFocusNavigation to make the responsibility clear. Thanks for the advice, actually I was not sure where to put the functions as well. I moved them to ScopedFocusNavigation. https://codereview.chromium.org/1840333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:132: while (m_current && (SlotScopedTraversal::isSlotScoped(*m_current) || SlotScopedTraversal::findFallbackScopeOwnerSlot(*m_current) != m_rootSlot)) On 2016/03/31 at 09:45:02, hayato wrote: > > SlotScopedTraversal::isSlotScoped(*m_current) || SlotScopedTraversal::findFallbackScopeOwnerSlot(*m_current) != m_rootSlot > > It looks we have several similar patterns here. > Could you extract this pattern as an utility function? > > e.g. > bool ScopedFocusNavigation::isFallbackContent(HTMLSlotElement& slot, Element& current) { > // Implement this. > } > > Then, the loop would become more readable: > > while (m_current && !ScopedFocusNavigation::isFallbackContent(*m_rootSlot, *m_current)) { > m_current = ElementTraversal::next(*m_current, m_rootSlot); > } I created a utility function as you suggested. However, isSlotScoped || isFallbackContentForAnotherSlot does not necessarily mean that it is not "isFallbackContent", so I added several lines to make sure that the input element is a fallback content. https://codereview.chromium.org/1840333002/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:234: if (SlotScopedTraversal::isSlotFallbackScoped(current)) On 2016/03/31 at 09:45:02, hayato wrote: > It looks a little wasteful to do the same check twice here. > > This can be: > > if (HTMLSlotElement* slot = SlotScopedTraversal::findFallbackScopeOwnerSlot(current)) > return ScopedFocusNavigation(*slot, ¤t); > > Can you do the similar thing to line 232-234? Done.
lgtm This is how I feel about this patch: http://i.imgur.com/Kj7ESBN.gif https://codereview.chromium.org/1840333002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/1840333002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:300: Element* parent = const_cast<Element*>(current.parentElement()); No need to const_cast here, I think.
lgtm https://codereview.chromium.org/1840333002/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/shadow-dom/slot-fallback-focus.html (right): https://codereview.chromium.org/1840333002/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/shadow-dom/slot-fallback-focus.html:31: </div> On 2016/03/31 07:03:46, yuzuchan wrote: > On 2016/03/30 at 06:28:33, kochi wrote: > > Can you add more test cases to be more comprehensive? > > e.g. > > * <slot>...fallback contents...<slot>...fallback2...</slot></slot> > > * <slot>...<shadow host>focusable elements to be slotted</shadow > host>...</slot> > > > > etc. etc. > > Thanks for the feedback! By adding the testcases suggested, I got to find a bug > and fix it :) Cool! https://codereview.chromium.org/1840333002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/shadow-dom/slot-fallback-focus.html (right): https://codereview.chromium.org/1840333002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/shadow-dom/slot-fallback-focus.html:66: for (var i = 0; i + 1 < elements.length; ++i) FYI it's okay to leave this as it is, but strictly spaking 'var' here doesn't scope the variable 'i' in this for loop, which is JavaScript's way how 'var' scope works. If you use 'let' the scope works as you expect.
The CQ bit was checked by yuzus@chromium.org
This is how I feel about this patch: http://i.imgur.com/RMFAEcM.gif https://codereview.chromium.org/1840333002/diff/40001/third_party/WebKit/Layo... File third_party/WebKit/LayoutTests/shadow-dom/slot-fallback-focus.html (right): https://codereview.chromium.org/1840333002/diff/40001/third_party/WebKit/Layo... third_party/WebKit/LayoutTests/shadow-dom/slot-fallback-focus.html:66: for (var i = 0; i + 1 < elements.length; ++i) On 2016/04/01 at 06:10:59, kochi wrote: > FYI it's okay to leave this as it is, but strictly spaking 'var' here doesn't scope > the variable 'i' in this for loop, which is JavaScript's way how 'var' scope works. > If you use 'let' the scope works as you expect. Acknowlegded :) Thanks! https://codereview.chromium.org/1840333002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/page/FocusController.cpp (right): https://codereview.chromium.org/1840333002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/page/FocusController.cpp:300: Element* parent = const_cast<Element*>(current.parentElement()); On 2016/04/01 at 05:45:34, hayato wrote: > No need to const_cast here, I think. Done.
The patchset sent to the CQ was uploaded after l-g-t-m from hayato@chromium.org, kochi@chromium.org Link to the patchset: https://codereview.chromium.org/1840333002/#ps60001 (title: "Refactor")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1840333002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1840333002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Support slots' fallback contents in focus navigation This CL implements slots' fallback contents in focus navigation. That is, if a focus hits a slot without any assigned nodes, it traverses the slot's children as its fallback contents. See the spec here: https://w3c.github.io/webcomponents/spec/shadow/#sequential-focus-navigation BUG=595285 ========== to ========== Support slots' fallback contents in focus navigation This CL implements slots' fallback contents in focus navigation. That is, if a focus hits a slot without any assigned nodes, it traverses the slot's children as its fallback contents. See the spec here: https://w3c.github.io/webcomponents/spec/shadow/#sequential-focus-navigation BUG=595285 Committed: https://crrev.com/7d503c6b674b0a2e9e5059edd89b52b5412f20ec Cr-Commit-Position: refs/heads/master@{#384540} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/7d503c6b674b0a2e9e5059edd89b52b5412f20ec Cr-Commit-Position: refs/heads/master@{#384540} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
