Bug 178001 - Focus navigation order in slot fallback contents is wrong
Summary: Focus navigation order in slot fallback contents is wrong
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: Safari 11
Hardware: Macintosh OS X 10.11
: P2 Normal
Assignee: Ryosuke Niwa
URL:
Keywords: InRadar
Depends on:
Blocks: 148695
  Show dependency treegraph
 
Reported: 2017-10-06 01:37 PDT by elkurin
Modified: 2018-08-22 12:59 PDT (History)
7 users (show)

See Also:


Attachments
WIP (5.43 KB, patch)
2018-08-21 20:47 PDT, Ryosuke Niwa
no flags Details | Formatted Diff | Diff
Fixes the bug (12.06 KB, patch)
2018-08-21 21:37 PDT, Ryosuke Niwa
koivisto: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>