Bug 178001

Summary: Focus navigation order in slot fallback contents is wrong
Product: WebKit Reporter: elkurin
Component: DOMAssignee: Ryosuke Niwa <rniwa>
Status: RESOLVED FIXED    
Severity: Normal CC: cdumez, elkurin, kochi, koivisto, rniwa, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: Safari 11   
Hardware: Mac   
OS: OS X 10.11   
Bug Depends on:    
Bug Blocks: 148695    
Attachments:
Description Flags
WIP
none
Fixes the bug koivisto: review+

Description elkurin 2017-10-06 01:37:03 PDT 
By running "focus-navigation-across-slots.html" test in WebKit Layout Tests, it seems that the navigation order is not following the spec.
https://w3c.github.io/webcomponents/spec/shadow/#sequential-focus-navigation

According to the spec 5.3 Step 2, in focus navigation, the subtree whose root is the slot element which no element is assigned to is considered as one of the focus navigation scope. Elements inside slot fallback contents scope should be navigated sequential.
In this test, the order should be 14->17->15->13->16.
Comment 1 Radar WebKit Bug Importer 2018-08-01 22:41:14 PDT 
<rdar://problem/42842997>
Comment 2 Ryosuke Niwa 2018-08-21 20:47:05 PDT 
Created attachment 347762 [details]
WIP
Comment 3 Ryosuke Niwa 2018-08-21 21:37:28 PDT 
Created attachment 347765 [details]
Fixes the bug
Comment 4 Antti Koivisto 2018-08-22 03:49:28 PDT 
Comment on attachment 347765 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=347765&action=review

> Source/WebCore/page/FocusController.cpp:283
> +        auto slot = makeRef(downcast<HTMLSlotElement>(element));
> +        return FocusNavigationScope(slot, slot->assignedNodes() ? SlotKind::Assigned : SlotKind::Fallback);

makeRef here look unnecessary. FocusNavigationScope doesn't ref the slot so someone better be keeping it alive.
Comment 5 Ryosuke Niwa 2018-08-22 12:56:33 PDT 
(In reply to Antti Koivisto from comment #4)
> Comment on attachment 347765 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=347765&action=review
> 
> > Source/WebCore/page/FocusController.cpp:283
> > +        auto slot = makeRef(downcast<HTMLSlotElement>(element));
> > +        return FocusNavigationScope(slot, slot->assignedNodes() ? SlotKind::Assigned : SlotKind::Fallback);
> 
> makeRef here look unnecessary. FocusNavigationScope doesn't ref the slot so
> someone better be keeping it alive.

I'll revert this change. I was trying to adopt Ref/RefPtr everywhere but decided to do it in a separate patch.
Comment 6 Ryosuke Niwa 2018-08-22 12:59:26 PDT 
Committed r235191: <https://trac.webkit.org/changeset/235191>