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+

elkurin
Reported 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.
Attachments
WIP (5.43 KB, patch)
2018-08-21 20:47 PDT, Ryosuke Niwa 		no flags
DetailsFormatted DiffDiff
Fixes the bug (12.06 KB, patch)
2018-08-21 21:37 PDT, Ryosuke Niwa 		koivisto: review+
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Radar WebKit Bug Importer
Comment 1 2018-08-01 22:41:14 PDT
<rdar://problem/42842997>
Ryosuke Niwa
Comment 2 2018-08-21 20:47:05 PDT
Created attachment 347762 [details] WIP
Ryosuke Niwa
Comment 3 2018-08-21 21:37:28 PDT
Created attachment 347765 [details] Fixes the bug
Antti Koivisto
Comment 4 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.
Ryosuke Niwa
Comment 5 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.
Ryosuke Niwa
Comment 6 2018-08-22 12:59:26 PDT
Committed r235191: <https://trac.webkit.org/changeset/235191>
Note You need to log in before you can comment on or make changes to this bug.