Closed Bug 1413834 Opened 8 years ago Closed 8 years ago

Implement sequential focus navigation regarding shadow DOM

Categories

(Core :: DOM: Core & HTML, enhancement, P2)

Product:
Core
Component:
DOM: Core & HTML
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla60
Tracking Flags:
Tracking Status
firefox60 --- fixed

People

(Reporter: ben.tian, Assigned: ben.tian)

References

Details

Attachments

(6 files, 3 obsolete files)

Bug 1413834 - part 1: Limit frame traversal and getting of next tabindex inside scope owned by document root,
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1413834 - part 2: Implement helper class to iterate contents in scope,
59 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1413834 - part 3: Get next tabbable content in shadow DOM,
59 bytes, text/x-review-board-request
smaug
: review+
Details
Focus Navigation Illustration.pdf
183.25 KB, application/pdf
Details
Bug 1413834 - part 4: Limit frame traversal inside scope owned by document root when shadow host has no frame
59 bytes, text/x-review-board-request
emilio
: review+
smaug
: review+
Details
a testcase
386 bytes, text/html
Details
Blocks: 1413836
(In reply to Ben Tian [:btian] from comment #0) > Sequential focus navigation in Shadow DOM spec: > https://www.w3.org/TR/shadow-dom/#sequential-focus-navigation Quick note: - step 1 picks out documents, shadow roots, and slot elements as OWNERS. - step 2 categorizes elements into scopes of OWNERS. - step 3 sorts elements inside each scope based on focusable or not and tabindex. - step 4 merges elements in different scopes into a global ordered list of elements, as "document sequential focus navigation order."
Priority: -- → P2
(In reply to Ben Tian [:btian] from comment #1) > (In reply to Ben Tian [:btian] from comment #0) > > Sequential focus navigation in Shadow DOM spec: > > https://www.w3.org/TR/shadow-dom/#sequential-focus-navigation > > Quick note: > - step 1 picks out documents, shadow roots, and slot elements as OWNERS. > - step 2 categorizes elements into scopes of OWNERS. > - step 3 sorts elements inside each scope based on focusable or not and > tabindex. > - step 4 merges elements in different scopes into a global ordered list of > elements, as "document sequential focus navigation order." Some difference to handle between A) spec algorithm and B) current implementation: - A) traverses DOM tree for next content, while B) traverses frame tree instead. - A) takes all nodes in document as input and outputs a complete order for navigation, while B) takes start content as input and outputs next content to focus.
Some notes. (In reply to Ben Tian [:btian] from comment #0) > An example: > https://github.com/w3c/webcomponents/issues/375#issuecomment-178989178 Another example for testing https://github.com/w3c/webcomponents/issues/496#issuecomment-227940954 No current wpt for focus navigation per https://github.com/w3c/web-platform-tests/pull/3277 > As reference, sequential focus navigation in DOM spec: > https://html.spec.whatwg.org/multipage/interaction.html#sequential-focus- > navigation Upstreaming to DOM spec is still in progress. https://github.com/whatwg/html/issues/1583
(In reply to Ben Tian [:btian] from comment #2) > Some difference to handle between A) spec algorithm and B) current > implementation: > - A) traverses DOM tree for next content, while B) traverses frame tree > instead. Some problems to solve for the inconsistency: - on frame tree, do not traverse into shadow tree and slot‘s subtree to check tab index of elements inside. - on frame tree, tell whether frame’s content is a distributed node, of fallback content, or others (i.e., find correct owner of the content). - under correct conditions, enter/leave shadow tree and slot's subtree as another scope. I'll try to start with shadow tree only, no slot case.
(In reply to Ben Tian [:btian] from comment #4) > Some problems to solve for the inconsistency: > - on frame tree, do not traverse into shadow tree and slot‘s subtree to > check tab index of elements inside. > - on frame tree, tell whether frame’s content is a distributed node, of > fallback content, or others (i.e., find correct owner of the content). One way to tell is from flatten tree parent node [1] per [2]. [1] https://searchfox.org/mozilla-central/rev/c633ffa4c4611f202ca11270dcddb7b29edddff8/dom/base/nsIContentInlines.h#135 [2] https://mozilla.logbot.info/content/20171120#c13880508 > - under correct conditions, enter/leave shadow tree and slot's subtree as > another scope. > > I'll try to start with shadow tree only, no slot case.
Summarize my current approach as following: Two new methods in nsFocusManager: a) |GetNextTabbableContentInScope| finds next tabbable content of given start content within a focus navigation scope [1] by traversing DOM tree. - If a document/shadow host/slot is met, call method b) to enter its scope. b) |GetNextTabbableContentCrossScope| configures corresponding <root content, start content, tabindex> for the scope to enter. - If root content is shadow root/slot, call method a). - If root content is a document, call |GetNextTabbableContent|. Additionally in |nsFocusManager::GetNextTabbableContent|, - Call method a) at the beginning if |aStartContent| is a shadow host/slot, or |aRootContent| and |aStartContent| are in different scopes. - Call method a) in [2] if found tabbable content is a shadow host/slot. - Do not traverse subtree of shadow host during frame traversal. [1] https://www.w3.org/TR/shadow-dom/#sequential-focus-navigation [2] https://searchfox.org/mozilla-central/rev/9f3bd430c2b132c86c46126a0548661de876799a/dom/base/nsFocusManager.cpp#3302 === Olli, do you think this is the right approach? Let me know for any feedback.
Flags: needinfo?(bugs)
Attached patch WIP patch (obsolete) — — Splinter Review
WIP patch that moves focus forward/backward correctly on example below. The patch is based on comment 6 approach. (In reply to Ben Tian [:btian] from comment #3) > Another example for testing > https://github.com/w3c/webcomponents/issues/496#issuecomment-227940954 === I'll revise flow based on the WIP patch and try to simplify. [TODO] - revise flow & handle error cases > also allow flexibility for delegateFocus - improve perf - add pref to on/off - handle iframe in shadow tree / slot
Perhaps put the patch to my feedback queue to prioritize it?
Flags: needinfo?(bugs)
The patch skips 1) descendants of shadow host's frame during frame traversal, and 2) descendants of shadow host while getting next tab index when |nsDocument::IsWebComponentsEnabled| is true. Also leaves TODO comment in |nsFocusManager::GetNextTabbableContent|.
Attachment #8935630 - Attachment is obsolete: true
Attachment #8938235 - Flags: feedback?(bugs)
The patch traverses DOM tree for next content to focus, when |aStartContent| is 1) a shadow host or slot, or 2) |aStartContent|'s scope owner is not |aRootContent| === TODO: - handle iframe in shadow tree - simplify with flattened tree traversal instead of DOM tree
Attachment #8938236 - Flags: feedback?(bugs)
(In reply to Olli Pettay [:smaug] from comment #8) > Perhaps put the patch to my feedback queue to prioritize it? Set f? for comment 9 and comment 10 patches. Thanks!
(In reply to Ben Tian [:btian] from comment #10) > TODO: > - handle iframe in shadow tree > - simplify with flattened tree traversal instead of DOM tree One problem while simplifying with flat tree: |ExplicitChildIterator| iterates shadow host's all children including unassigned ones (i.e., not in flat tree). The inconsistency + flat tree traversal may not be simpler than DOM tree traversal. Will keep DOM tree traversal first.
(In reply to Ben Tian [:btian] from comment #12) > > - simplify with flattened tree traversal instead of DOM tree > > One problem while simplifying with flat tree: > > |ExplicitChildIterator| iterates shadow host's all children including > unassigned ones (i.e., not in flat tree). The inconsistency + flat tree > traversal may not be simpler than DOM tree traversal. Will keep DOM tree > traversal first. I may use the wrong child iterator and am trying FlattenedChildIterator instead. Thank Jessica for reminding.
(In reply to Ben Tian [:btian] from comment #13) > (In reply to Ben Tian [:btian] from comment #12) > > > - simplify with flattened tree traversal instead of DOM tree > I may use the wrong child iterator and am trying FlattenedChildIterator > instead. Thank Jessica for reminding. FlattenedChildIterator works! Continue revising...
I don't yet even understand what the spec tries to say. Especially "For each node tree TREE in DOCUMENT: " But trying to figure out...
Step 2 "Stop this algorithm." So we end up finding just one scope owner? And step 3 take _a_ SCOPE as input, but then "For each scope SCOPE: " still trying to understand the spec
https://github.com/w3c/webcomponents/issues/720 https://github.com/w3c/webcomponents/issues/721 But ok, I should give up with the spec, and just review the patch and see how they map to our implementation.
Comment on attachment 8938235 [details] [diff] [review] Part 1: Skip contents in shadow tree while traversing frames and getting next tab index for focus navigation Could you create patches with more context. -U 8 is the normal @@ -18,7 +18,8 @@ nsresult NS_NewFrameTraversal(nsIFrameEn bool aVisual, bool aLockInScrollView, bool aFollowOOFs, - bool aSkipPopupChecks); + bool aSkipPopupChecks, + bool aSkipShadow = false); Could we not have optional bool flag there. Otherwise one easily forgets to think about whether to pass true. I don't quite understand what happens when we start tabbing from an element inside Shadow DOM. Say, you have several <a> elements there and focus first one and then press <TAB>. But ok, this is a step 1
Attachment #8938235 - Flags: feedback?(bugs) → feedback+
Comment on attachment 8938236 [details] [diff] [review] [WIP] Part 2: Traverse DOM tree to get next tabbable content OK, so delegateFocus is missing. Hmm, I think we should try to use frame traversal in shadow DOM too, if we use it elsewhere. I know it doesn't follow what the spec tries to describe, but our behavior is different without Shadow DOM anyhow. Other option is to change all of focus traversing to use DOM. Neil might have an opinion on that. I've seen comments that frame tree traversal actually gives better behavior.
Flags: needinfo?(enndeakin)
Attachment #8938236 - Flags: feedback?(bugs)
(In reply to Olli Pettay [:smaug] from comment #16) > Step 2 > "Stop this algorithm." So we end up finding just one scope owner? > And step 3 take _a_ SCOPE as input, but then > "For each scope SCOPE: " > > still trying to understand the spec Examples I refer to: https://github.com/w3c/webcomponents/issues/496#issuecomment-227940954 https://github.com/w3c/webcomponents/issues/375#issuecomment-178989178 My understanding on spec is that - step 1 picks out documents, shadow roots, and slot elements as OWNERS. - step 2 categorizes elements into scopes of OWNERS. - step 3 sorts elements inside each scope based on focusable or not and tabindex. - step 4 merges elements in different scopes into a global ordered list of elements, as "document sequential focus navigation order."
(In reply to Olli Pettay [:smaug] from comment #19) > Comment on attachment 8938236 [details] [diff] [review] > [WIP] Part 2: Traverse DOM tree to get next tabbable content > > OK, so delegateFocus is missing. Yes. Not in the patches. > Hmm, I think we should try to use frame traversal in shadow DOM too, if we > use it elsewhere. My concern is slots have no frame and wouldn't appear in frame traversal. So extra effort is required to 1) check whether each frame's content is in another scope and 2) whether the slot's tabindex matches current tabindex (if not rollback to previous frame). These are doable but seems more complex and error-prone than DOM tree traversal to me. > I know it doesn't follow what the spec tries to describe, but our behavior > is different without Shadow DOM anyhow. > > Other option is to change all of focus traversing to use DOM. > Neil might have an opinion on that. That's another option. I didn't consider as it impacts behavior w/ shadow DOM. > > I've seen comments that frame tree traversal actually gives better behavior.
(In reply to Ben Tian [:btian] from comment #21) > That's another option. I didn't consider as it impacts behavior w/ shadow > DOM. typo: w/o shadow DOM
I'm worried about consistency. If most of the page uses frame traversal, but then Shadow DOM parts DOM traversal, that may lead to rather unexpected behavior.
And with consistency I mean the behavior user sees. User has no idea whether a page is using shadow DOM or not, there are just some focusable parts in the page, and tabbing through them should work in some consistent way. Given that the spec for sequential focus handling clearly needs some work, I wouldn't be very worried if we shipped first something closer to our current behavior. Others may of course have different opinion.
(In reply to Olli Pettay [:smaug] from comment #24) > And with consistency I mean the behavior user sees. User has no idea whether > a page is using shadow DOM or not, there are just some focusable parts in > the page, and tabbing through them should work in some consistent way. To confirm, do you suggest shadow DOM spec behavior [1] is inconsistent to current behavior w/o shadow DOM [2]? I don't get what kind of behavior is _consistent_ though. [1] https://www.w3.org/TR/shadow-dom/#sequential-focus-navigation [2] https://html.spec.whatwg.org/multipage/interaction.html#sequential-focus-navigation
Flags: needinfo?(bugs)
(btw, don't link to /TR/ specs. They are in general obsolete. Use latest editor's drafts) I mean that Gecko uses frame tree, and Shadow DOM relies on DOM tree. But now that I'm thinking again, perhaps it wouldn't be too bad to use DOM traversing inside shadow. In practice it shouldn't matter much.
Flags: needinfo?(bugs)
(In reply to Ben Tian [:btian] from comment #20) > My understanding on spec is that > - step 1 picks out documents, shadow roots, and slot elements as OWNERS. > - step 2 categorizes elements into scopes of OWNERS. > - step 3 sorts elements inside each scope based on focusable or not and > tabindex. > - step 4 merges elements in different scopes into a global ordered list of > elements, as "document sequential focus navigation order." oh, sure, but that just needs some guesswork ;)
The Shadow DOM spec assumes DOM traversal for light DOM too, but Gecko uses layout tree. But I think I'm changing my mind a bit. Perhaps we could just treat shadow DOM stuff as somewhat separate widgets in the page. I'll re-look at the patch.
Attachment #8938236 - Flags: feedback?(bugs)
I do think that frame order is the correct way and gives results that more closely match user expectation, but I'm not sure how the tabIndex handling with elements in different scopes would be handled.
Flags: needinfo?(enndeakin)
Comment on attachment 8938236 [details] [diff] [review] [WIP] Part 2: Traverse DOM tree to get next tabbable content >+ } >+ else { /* !aForward */ Nit, } else { >+/*====================================================================================*/ >+/*====================================================================================*/ Weird lines here. >+nsFocusManager::IsDocRootOrShadowRoot(nsIContent* aContent) >+{ >+ // Document root >+ nsIDocument* doc = aContent->GetUncomposedDoc(); >+ if (doc && aContent == doc->GetRootElement()) { >+ return true; >+ } >+ >+ // Shadow root >+ return !!ShadowRoot::FromNode(aContent); aContent->IsShadowRoot() ? >+bool >+nsFocusManager::NotOwnerOf(nsIContent* aMaybeOwner, nsIContent* aContent) >+{ >+ return aContent->IsInHTMLDocument() && We need to make this work in other documents too. I don't understand IsInHTMLDocument limitation. >+void >+nsFocusManager::GetNextTabbableContentInScope( >+ nsIContent* aOwner, >+ nsIContent* aStartContent, >+ bool aForward, >+ int32_t aCurrentTabIndex, >+ nsIContent** aResultContent) >+{ >+ ScopedContentTraversal iter(aStartContent, aOwner); >+ nsCOMPtr<nsIContent> iterContent = aStartContent; >+ >+ while (1) { >+ /** >+ * Iterate tab indexes to search find corresponding contents in scope >+ */ >+ >+ while (1) { >+ /** >+ * Iterate remaining contents in scope to find next content to focus >+ */ >+ >+ printf("InScope - ENTER INNER LOOP\n"); >+ >+ // Get next content >+ if (iterContent == iter.GetOwner()) { >+ aForward ? iter.First() : iter.Last(); >+ } else { >+ aForward ? iter.Next() : iter.Prev(); >+ } >+ >+ iterContent = iter.GetCurrent(); >+ if (!iterContent) { >+ break; >+ } >+ >+ // Get tab index >+ int32_t tabIndex = 0; >+ iterContent->IsFocusable(&tabIndex); >+ >+ printf("### Next Tabbable %p (frame %p), %s:\n", (void*)iterContent, iterContent->GetPrimaryFrame(), >+ NS_ConvertUTF16toUTF8(iterContent->NodeName()).get()); >+ printf("### with tabindex: %d expected: %d\n", tabIndex, aCurrentTabIndex); >+ >+ // [TODO] check if focusable? >+ if (tabIndex < 0 || tabIndex != aCurrentTabIndex) { >+ continue; >+ } >+ >+ // Found content to focus >+ if (!IsHostOrSlot(iterContent)) { >+ printf(">>> Find focusable content! tabindex %d\n", tabIndex); >+ *aResultContent = iterContent; >+ return; >+ } >+ >+ // Enter scope of |iterContent| to find content to focus >+ printf("Cross scope 2\n"); >+ GetNextTabbableContentCrossScope(iterContent, aForward, aCurrentTabIndex, aResultContent); Oh, this method enters scopes deeply? Then this method should have "Deep" in its name I guess. >+void >+nsFocusManager::GetNextTabbableContentCrossScope( >+ nsIContent* aStartContent, >+ bool aForward, >+ int32_t aCurrentTabIndex, >+ nsIContent** aResultContent) >+{ >+ // >+ // [TODO] handle document >+ // >+ >+ MOZ_ASSERT(IsHostOrSlot(aStartContent)); >+ >+ nsCOMPtr<nsIContent> startContent = >+ aStartContent->GetShadowRoot() ? aStartContent->GetShadowRoot() >+ : aStartContent; ok, so this method should be called something like, GetNextTabbableContentInShadowRootOrSlot >+void >+nsFocusManager::GetNextTabbableContentDeepCrossScope( >+ nsIContent* aStartContent, >+ bool aForward, >+ int32_t* aCurrentTabIndex, >+ nsIContent** aResultContent) >+{ >+ nsCOMPtr<nsIContent> startContent = aStartContent; >+ nsCOMPtr<nsIContent> owner = FindOwner(aStartContent); >+ >+ int32_t tabIndex = 0; >+ startContent->IsFocusable(&tabIndex); // [TODO] handle -1 >+ >+ printf("====== Enter scope [DEEP]: START %p nodename: %s\n", (void*)startContent, >+ NS_ConvertUTF16toUTF8(startContent->NodeName()).get()); >+ printf(" tabindex: %d\n", tabIndex); >+ >+ GetNextTabbableContentInScope(owner, startContent, aForward, tabIndex, aResultContent); >+ >+ printf("Leave scope [DEEP]\n"); >+ >+ if (*aResultContent) { >+ return; >+ } >+ >+ // not found -> >+ // LOOP: host/slot cross as content >+ while (1) { >+ if (ShadowRoot::FromNode(owner)) { >+ owner = ShadowRoot::FromNode(owner)->Host(); >+ } So this method doesn't go down in DOM tree, but possibly up. I wonder what the name should be. > /** >+ * Class to traverse contents within scope in tree order >+ */ >+ class ScopedContentTraversal { Nit, { goes to its own line after class definitions. >+ /** >+ * Find next content to focus in |aOwner|-owned scope. >+ * If a shadow root / slot is met, enter its scope recursively. >+ * >+ * [TODO] ignore tab index >+ */ >+ void GetNextTabbableContentInScope( >+ nsIContent* aOwner, >+ nsIContent* aStartContent, >+ bool aForward, >+ int32_t aCurrentTabIndex, >+ nsIContent** aResultContent); >+ >+ /** >+ * Configure arguments and find next content to focus in >+ * |aStartContent|-owned scope. >+ * >+ * [TODO] ignore tab index >+ */ >+ void GetNextTabbableContentCrossScope( >+ nsIContent* aStartContent, >+ bool aForward, >+ int32_t aCurrentTabIndex, >+ nsIContent** aResultContent); >+ >+ /** >+ * Configure arguments and find next content to focus, >+ * starting from |aStartContent|'s scope. If not found, >+ * find next content to focus for |aStartContent|'s owner >+ * iteratively. >+ * >+ * [TODO] ignore tab index >+ */ >+ void GetNextTabbableContentDeepCrossScope( >+ nsIContent* aStartContent, >+ bool aForward, >+ int32_t* aCurrentTabIndex, >+ nsIContent** aResultContent); I don't understand how these three methods are different. The first one says "If a shadow root / slot is met, enter its scope recursively." which sounds like something a method with Deep in it would do. And CrossScope sounds like that too. DeepCrossScope is the hardest to understand. Some method renaming would be good, but ok, I guess we can start going this direction. And please ping me if I'm being slow at giving feedback.
Attachment #8938236 - Flags: feedback?(bugs)
Hmm, how does the approach work with <input type="date"> inside Shadow DOM? Could you test? Do we need some special magic to deal with native anonymous content?
(In reply to Olli Pettay [:smaug] from comment #30) > >+ /** > >+ * Find next content to focus in |aOwner|-owned scope. > >+ * If a shadow root / slot is met, enter its scope recursively. > >+ * > >+ * [TODO] ignore tab index > >+ */ > >+ void GetNextTabbableContentInScope( > >+ nsIContent* aOwner, > >+ nsIContent* aStartContent, > >+ bool aForward, > >+ int32_t aCurrentTabIndex, > >+ nsIContent** aResultContent); > >+ > >+ /** > >+ * Configure arguments and find next content to focus in > >+ * |aStartContent|-owned scope. > >+ * > >+ * [TODO] ignore tab index > >+ */ > >+ void GetNextTabbableContentCrossScope( > >+ nsIContent* aStartContent, > >+ bool aForward, > >+ int32_t aCurrentTabIndex, > >+ nsIContent** aResultContent); > >+ > >+ /** > >+ * Configure arguments and find next content to focus, > >+ * starting from |aStartContent|'s scope. If not found, > >+ * find next content to focus for |aStartContent|'s owner > >+ * iteratively. > >+ * > >+ * [TODO] ignore tab index > >+ */ > >+ void GetNextTabbableContentDeepCrossScope( > >+ nsIContent* aStartContent, > >+ bool aForward, > >+ int32_t* aCurrentTabIndex, > >+ nsIContent** aResultContent); > I don't understand how these three methods are different. > The first one says "If a shadow root / slot is met, enter its scope > recursively." which sounds like something a method with Deep in it would do. > And CrossScope sounds like that too. DeepCrossScope is the hardest to > understand. I'm bad at naming:( - |InScope| searches only in the same scope. Owner doesn't change in this method. - |CrossScope| goes down and up DOM tree by 1-level. E.g., enter scope of a shadow root / slot and search inside. - |DeepCrossScope| goes down and up DOM tree by multi-level. E.g., in example [2] when |GetNextTabbableContent| is called with |aStartContent| as <input id=j2 slot=s2 tabindex=2></input> , we need to 1) search in s2's scope first and then 2) search next content to s2 with s2's tabindex in <x-bar>'s scope, next content to <x-bar> with <x-bar>'s tabindex in <x-foo>'s scope, and finally next content to <x-foo> in document tree. Part 1) is the first |CrossScope| call and part 2) is the loop to go up. [2] https://github.com/w3c/webcomponents/issues/375#issuecomment-178989178 > >+/*====================================================================================*/ > >+/*====================================================================================*/ > Weird lines here. Will remove in final patches. Separation line I referred in [1]. [1] https://searchfox.org/mozilla-central/rev/f42618c99dcb522fb674221acfbc68c2d92e7936/dom/base/nsContentIterator.cpp#1190 > >+bool > >+nsFocusManager::NotOwnerOf(nsIContent* aMaybeOwner, nsIContent* aContent) > >+{ > >+ return aContent->IsInHTMLDocument() && > We need to make this work in other documents too. I don't understand > IsInHTMLDocument limitation. Will revise it. > > >+void > >+nsFocusManager::GetNextTabbableContentInScope( > >+ nsIContent* aOwner, > >+ nsIContent* aStartContent, > >+ bool aForward, > >+ int32_t aCurrentTabIndex, > >+ nsIContent** aResultContent) > >+{ > >+ ScopedContentTraversal iter(aStartContent, aOwner); > >+ nsCOMPtr<nsIContent> iterContent = aStartContent; > >+ > >+ while (1) { > >+ /** > >+ * Iterate tab indexes to search find corresponding contents in scope > >+ */ > >+ > >+ while (1) { > >+ /** > >+ * Iterate remaining contents in scope to find next content to focus > >+ */ > >+ > >+ printf("InScope - ENTER INNER LOOP\n"); > >+ > >+ // Get next content > >+ if (iterContent == iter.GetOwner()) { > >+ aForward ? iter.First() : iter.Last(); > >+ } else { > >+ aForward ? iter.Next() : iter.Prev(); > >+ } > >+ > >+ iterContent = iter.GetCurrent(); > >+ if (!iterContent) { > >+ break; > >+ } > >+ > >+ // Get tab index > >+ int32_t tabIndex = 0; > >+ iterContent->IsFocusable(&tabIndex); > >+ > >+ printf("### Next Tabbable %p (frame %p), %s:\n", (void*)iterContent, iterContent->GetPrimaryFrame(), > >+ NS_ConvertUTF16toUTF8(iterContent->NodeName()).get()); > >+ printf("### with tabindex: %d expected: %d\n", tabIndex, aCurrentTabIndex); > >+ > >+ // [TODO] check if focusable? > >+ if (tabIndex < 0 || tabIndex != aCurrentTabIndex) { > >+ continue; > >+ } > >+ > >+ // Found content to focus > >+ if (!IsHostOrSlot(iterContent)) { > >+ printf(">>> Find focusable content! tabindex %d\n", tabIndex); > >+ *aResultContent = iterContent; > >+ return; > >+ } > >+ > >+ // Enter scope of |iterContent| to find content to focus > >+ printf("Cross scope 2\n"); > >+ GetNextTabbableContentCrossScope(iterContent, aForward, aCurrentTabIndex, aResultContent); > Oh, this method enters scopes deeply? Then this method should have "Deep" in > its name I guess. |CrossScope| as it goes down on DOM tree 1-level only. > > > >+void > >+nsFocusManager::GetNextTabbableContentCrossScope( > >+ nsIContent* aStartContent, > >+ bool aForward, > >+ int32_t aCurrentTabIndex, > >+ nsIContent** aResultContent) > >+{ > >+ // > >+ // [TODO] handle document > >+ // > >+ > >+ MOZ_ASSERT(IsHostOrSlot(aStartContent)); > >+ > >+ nsCOMPtr<nsIContent> startContent = > >+ aStartContent->GetShadowRoot() ? aStartContent->GetShadowRoot() > >+ : aStartContent; > > ok, so this method should be called something like, > GetNextTabbableContentInShadowRootOrSlot Yes. Will rename it. > Some method renaming would be good, but ok, I guess we can start going this > direction. And please ping me if I'm being slow at giving feedback. Sure. Thanks for the feedback.
(In reply to Ben Tian [:btian] from comment #32) > I'm bad at naming:( > > - |InScope| searches only in the same scope. Owner doesn't change in this > method. > - |CrossScope| goes down and up DOM tree by 1-level. E.g., enter scope of a > shadow root / slot and search inside. > - |DeepCrossScope| goes down and up DOM tree by multi-level. E.g., in > example [2] when |GetNextTabbableContent| is called with |aStartContent| as > <input id=j2 slot=s2 tabindex=2></input> > , we need to 1) search in s2's scope first and then 2) search next > content to s2 with s2's tabindex in <x-bar>'s scope, next content to <x-bar> > with <x-bar>'s tabindex in <x-foo>'s scope, and finally next content to > <x-foo> in document tree. Part 1) is the first |CrossScope| call and part 2) s/CrossScope/InScope > is the loop to go up. > > [2] https://github.com/w3c/webcomponents/issues/375#issuecomment-178989178
(In reply to Ben Tian [:btian] from comment #32) > > ok, so this method should be called something like, > > GetNextTabbableContentInShadowRootOrSlot > > Yes. Will rename it. Also iframe case here. Let me think about the name.
Attachment #8938236 - Attachment is obsolete: true
(In reply to Ben Tian [:btian] from comment #35) > Created attachment 8940601 [details] > Bug 1413834 - part 1: Skip contents in shadow tree while traversing frames > and getting next tab index for focus navigation, f=smaug > > Review commit: https://reviewboard.mozilla.org/r/210816/diff/#index_header > See other reviews: https://reviewboard.mozilla.org/r/210816/ Addressed comment 18 and carry f+ flag.
Attachment #8938235 - Attachment is obsolete: true
(In reply to Olli Pettay [:smaug] from comment #31) > Hmm, how does the approach work with <input type="date"> inside Shadow DOM? > Could you test? Doesn't work for |content->IsFocusable()| [1] gets tabIndex -1, though its tabindex attribute is not. [1] L3325 in https://reviewboard.mozilla.org/r/210820/diff/2#2.2 > Do we need some special magic to deal with native anonymous content? I'm not sure if tabIndex -1 above relates to native anonymous content. Checking.
(In reply to Ben Tian [:btian] from comment #36) > Created attachment 8940602 [details] > Bug 1413834 - part 2: Implement helper class ScopedContentTraversal to > iterate contents in scope by traversing flattened tree in tree order > > Review commit: https://reviewboard.mozilla.org/r/210818/diff/#index_header > See other reviews: https://reviewboard.mozilla.org/r/210818/ Traverse flattened tree instead of DOM tree to simplify traversing code. Changes: - An scope owner is either document root, shadow HOST, and slot. The definition differs from spec [1] but is similar to Chrome [2]. [1] https://w3c.github.io/webcomponents/spec/shadow/#sequential-focus-navigation [2] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/FocusController.cpp?type=cs&sq=package:chromium&l=143-144
(In reply to Ben Tian [:btian] from comment #39) > Comment on attachment 8940603 [details] > [WIP] Bug 1413834 - Traverse flattened tree to get next tabbable content > > Review request updated; see interdiff: > https://reviewboard.mozilla.org/r/210820/diff/1-2/ TODO: - support iframe in shadow DOM - address feedback comment 30 KNOWN BUGS: - method |InScope| skips host in shadow DOM - <input type="date"> gets tabIndex -1 as comment 40. - Check native anonymous content behavior
The thing with <input type=date> is that in light DOM we traverse frame tree, so we enter the layout objects of that element, including the objects for the native anonymous content. So, should we in shadow DOM case try to access the primary frame of an element, try to get nsIAnonymousContentCreator from it, and if such is got, use frame tree locally for the native anonymous tree to see if there is something to focus. (That all until we've possibly converted all of native anonymous content to use Shadow DOM. But even then we'd probably need some special handling for form control elements and such. And that all is future work.)
(In reply to Ben Tian [:btian] from comment #46) > Comment on attachment 8940603 [details] > Bug 1413834 - part 3: Traverse flattened tree to get next tabbable content > > Review request updated; see interdiff: > https://reviewboard.mozilla.org/r/210820/diff/2-3/ Changes: - rename methods and add comments in header > |GetNextTabbableContentInsideScope| searches next tabbable element inside one scope. > |GetNextTabbableContentAtOwner| configures argument and searches next tabbable element inside scope owned by aOwner.The method goes DOWN in flattened tree. > |GetNextTabbableContentAtContent| configures argument and searches next tabbable element inside scope including aStartContent. The method goes UP in flattened tree. - simplify |GetNextTabbableContentAtContent| (was|GetNextTabbableContentDeepCrossScope|) - in |GetNextTabbableContentInsideScope| return content only if its primary frame has tabindex >= 0 - add argument |aIncludeOwner| in methods to specify whether to include owner in focus navigation order - handle aIgnoreTabIndex in methods - remove method |NotOwnerOf|
Attachment #8940601 - Flags: review?(bugs)
Attachment #8940602 - Flags: review?(bugs)
Attachment #8940603 - Flags: review?(bugs)
(In reply to Ben Tian [:btian] from comment #52) > Comment on attachment 8940603 [details] > Bug 1413834 - part 3: Traverse flattened tree to get next tabbable content > > Review request updated; see interdiff: > https://reviewboard.mozilla.org/r/210820/diff/4-5/ The change is based on shadow DOM spec. Need to consider whether to check isFocusable of frame rather than content. Handling of iframe in shadow tree and NAC are missing. Will submit patches in follow-up bugs.
Comment on attachment 8940602 [details] Bug 1413834 - part 2: Implement helper class to iterate contents in scope, https://reviewboard.mozilla.org/r/210818/#review218008 ::: dom/base/nsFocusManager.cpp:3071 (Diff revision 4) > return NS_OK; > } > > +// Helper class to iterate contents in scope by traversing flattened tree > +// in tree order > +class ScopedContentTraversal Looks like this is used only on stack, so the class should be annotated with MOZ_STACK_CLASS ::: dom/base/nsFocusManager.cpp:3096 (Diff revision 4) > + void SetCurrent(nsIContent* aContent) > + { > + mCurrent = aContent; > + } > + > + nsCOMPtr<nsIContent> mCurrent; No need to use strong member variable, if this is stack only and it is guaranteed otherwise that mCurrent and mOwner stay alive. ::: dom/base/nsFocusManager.cpp:3106 (Diff revision 4) > +ScopedContentTraversal::Next() > +{ > + MOZ_ASSERT(mCurrent); > + > + // Get mCurrent's first child if it's in the same scope. > + if (!(mCurrent->GetShadowRoot() || mCurrent->IsHTMLElement(nsGkAtoms::slot)) || What if slot doesn't have any assigned nodes? ::: dom/base/nsFocusManager.cpp:3142 (Diff revision 4) > + if (parent == mOwner) { > + SetCurrent(nullptr); > + return; > + } > + > + // Find next sibing of parent sibling ::: dom/base/nsFocusManager.cpp:3152 (Diff revision 4) > +void > +ScopedContentTraversal::Prev() > +{ > + MOZ_ASSERT(mCurrent); > + > + nsIContent *parent, *last; Coding style is nsIContent* parent; nsIContent* last; ::: dom/base/nsFocusManager.cpp:3172 (Diff revision 4) > + } > + > + while (last) { > + parent = last; > + if (parent->GetShadowRoot() || > + parent->IsHTMLElement(nsGkAtoms::slot)) { Why is this right if slot doesn't have any assigned nodes? ::: dom/base/nsFocusManager.cpp:3201 (Diff revision 4) > + > + while (nsIContent* iter = GetCurrent()) { > + // Get tab index of mCurrent > + nsAutoString tabIndexStr; > + if (iter->IsElement()) { > + iter->AsElement()->GetAttr( tabindex should be checked on HTML/SVG elements only. And "When the attribute is omitted, the user agent applies defaults." for which we have https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/dom/base/Element.cpp#331 and https://searchfox.org/mozilla-central/search?q=symbol:_ZN7mozilla3dom7Element15TabIndexDefaultEv&redirect=false Why can't we use IsFocusable here?
Attachment #8940602 - Flags: review?(bugs) → review-
Comment on attachment 8940602 [details] Bug 1413834 - part 2: Implement helper class to iterate contents in scope, https://reviewboard.mozilla.org/r/210818/#review218024 ::: dom/base/nsFocusManager.cpp:3172 (Diff revision 4) > + } > + > + while (last) { > + parent = last; > + if (parent->GetShadowRoot() || > + parent->IsHTMLElement(nsGkAtoms::slot)) { so nevermind this comment ::: dom/base/nsFocusManager.cpp:3201 (Diff revision 4) > + > + while (nsIContent* iter = GetCurrent()) { > + // Get tab index of mCurrent > + nsAutoString tabIndexStr; > + if (iter->IsElement()) { > + iter->AsElement()->GetAttr( nevermind this comment
(In reply to Olli Pettay [:smaug] from comment #54) > > + // Get mCurrent's first child if it's in the same scope. > > + if (!(mCurrent->GetShadowRoot() || mCurrent->IsHTMLElement(nsGkAtoms::slot)) || > > What if slot doesn't have any assigned nodes? I guess that shouldn't matter > > tabindex should be checked on HTML/SVG elements only. oh, I see that tabindex reading is based on existing code in nsFocusManager.
Comment on attachment 8940603 [details] Bug 1413834 - part 3: Get next tabbable content in shadow DOM, https://reviewboard.mozilla.org/r/210820/#review218020 I'm slowly understanding this :) ::: dom/base/nsFocusManager.h:485 (Diff revision 5) > + * Retrieve the next tabbable element in scope including aStartContent > + * and the scope's ancestor scopes, using focusability and tabindex to > + * determine the tab order. The element is returned in aResultContent. > + * > + * aStartContent is the starting point for this call of this method, and > + * returns shadow host in light tree if the next tabbable element is not Returns shadow host? What if that is not focusable? ::: dom/base/nsFocusManager.h:498 (Diff revision 5) > + * searching in light tree. > + * > + * aIgnoreTabIndex to ignore the current tabindex and find the element > + * irrespective or the tab index. > + */ > + void GetNextTabbableContentInAncestorScopes(nsIContent** aStartContent, Why not just return the result as nsIContent* return value. ::: dom/base/nsFocusManager.cpp:3260 (Diff revision 5) > + > + return nullptr; > + } > + > + // Shadow host / Slot > + if (IsHostOrSlot(parent)) { I wonder... host isn't an owner, but ShadowRoot, I mean per spec. I guess we don't play with ShadowRoots here. But would be good to document somewhere. ::: dom/base/nsFocusManager.cpp:3264 (Diff revision 5) > + // Shadow host / Slot > + if (IsHostOrSlot(parent)) { > + return parent; > + } > + > + return FindOwner(parent); Could you make this method iterative and not recursive. ::: dom/base/nsFocusManager.cpp:3323 (Diff revision 5) > + continue; > + } > + > + if (!IsHostOrSlot(iterContent)) { > + // Found content to focus > + *aResultContent = iterContent; aResultContent is nsIContent** which in general should be AddRefed. So better to just change the return type to nsIContent* and do return foo; when needed. ::: dom/base/nsFocusManager.cpp:3368 (Diff revision 5) > + bool aForward, > + int32_t* aCurrentTabIndex, > + bool aIgnoreTabIndex, > + nsIContent** aResultContent) > +{ > + nsCOMPtr<nsIContent> startContent = *aStartContent; Something really weird here. Is aStartContent what kind of parameter? inout? Is it AddRefed or what?
Attachment #8940603 - Flags: review?(bugs) → review-
Comment on attachment 8940601 [details] Bug 1413834 - part 1: Limit frame traversal and getting of next tabindex inside scope owned by document root, https://reviewboard.mozilla.org/r/210816/#review218006 could you explain how this works when focus is already inside shadow DOM. ::: dom/base/nsFocusManager.cpp:3412 (Diff revision 3) > if (aForward) { > tabIndex = 0; > for (nsIContent* child = aParent->GetFirstChild(); > child; > child = child->GetNextSibling()) { > + // Skip child's descendants if child is a shadow host, as they are Shouldn't we skip also any elements in shadow dom or descendants of slot elements? ::: layout/base/nsFrameTraversal.cpp:418 (Diff revision 3) > { > + if (mSkipShadow) { > + // Skip frames rendered from contents in shadow tree, > + // primarily for focus navigation > + nsIContent* content = aFrame->GetContent(); > + if (content && content->GetShadowRoot()) { Can we enter here when we already are in shadow DOM. Should we check that content isn't in shadow DOM, or in a subtree which is somewhere assigned to a slot?
Attachment #8940601 - Flags: review?(bugs) → review-
I basically just need to re-read the tweaked patches. Took a bit time to understand what every method does.
Comment on attachment 8940602 [details] Bug 1413834 - part 2: Implement helper class to iterate contents in scope, https://reviewboard.mozilla.org/r/210818/#review218008 > What if slot doesn't have any assigned nodes? The owner of fallback content is also slot per [1]. So slot's children on flattened tree (no matter assigned nodes or fallback content) are not in the same scope including slot. [1] step 2.2.1.4 in https://w3c.github.io/webcomponents/spec/shadow/#sequential-focus-navigation /// 4. If CURRENT is a slot element and not equal to ELEMENT: 1. If CURRENT has any assigned nodes, do not append ELEMENT to any scope. 2. _Otherwise append ELEMENT to SCOPE-MAP[CURRENT]._ 3. Stop this algorithm. ///
Comment on attachment 8940603 [details] Bug 1413834 - part 3: Get next tabbable content in shadow DOM, https://reviewboard.mozilla.org/r/210820/#review218032 ::: dom/base/nsFocusManager.h:498 (Diff revision 5) > + * searching in light tree. > + * > + * aIgnoreTabIndex to ignore the current tabindex and find the element > + * irrespective or the tab index. > + */ > + void GetNextTabbableContentInAncestorScopes(nsIContent** aStartContent, Sure I can revise. Just wanted to update both aStartcontent and aCurrentTabIndex in the same way. ::: dom/base/nsFocusManager.cpp:3260 (Diff revision 5) > + > + return nullptr; > + } > + > + // Shadow host / Slot > + if (IsHostOrSlot(parent)) { Yes I adopt host instead of shadow root since shadow root isn't on flattened tree. Chrome also adopts host as [1]. [1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/FocusController.cpp?type=cs&sq=package:chromium&l=143-144 ::: dom/base/nsFocusManager.cpp:3264 (Diff revision 5) > + // Shadow host / Slot > + if (IsHostOrSlot(parent)) { > + return parent; > + } > + > + return FindOwner(parent); Sure. Will revise this method. ::: dom/base/nsFocusManager.cpp:3368 (Diff revision 5) > + bool aForward, > + int32_t* aCurrentTabIndex, > + bool aIgnoreTabIndex, > + nsIContent** aResultContent) > +{ > + nsCOMPtr<nsIContent> startContent = *aStartContent; aStartContent is an inout parameter. It's the start point in shadow tree (in) and also returns the shadow host in light tree (out) to keep searching the next content in light tree, if no next content to focus in shadow tree. Ah I should pass |startContent| instead as it's AddRefed. Then no need to use strong pointer here. Will revise.
Comment on attachment 8940601 [details] Bug 1413834 - part 1: Limit frame traversal and getting of next tabindex inside scope owned by document root, https://reviewboard.mozilla.org/r/210816/#review218036 ::: dom/base/nsFocusManager.cpp:3412 (Diff revision 3) > if (aForward) { > tabIndex = 0; > for (nsIContent* child = aParent->GetFirstChild(); > child; > child = child->GetNextSibling()) { > + // Skip child's descendants if child is a shadow host, as they are We stop traversing downwards from shadow host so elements in shadow dom would be omitted as well. For slot elements I'll add MOZ_ASSERT(!child->IsHTMLElement(nsGkAtoms::slot)) as this method is only called from light tree.
Comment on attachment 8940601 [details] Bug 1413834 - part 1: Limit frame traversal and getting of next tabindex inside scope owned by document root, https://reviewboard.mozilla.org/r/210816/#review218006 When focus is already inside shadow DOM, changes in patch 1 only affects AFTER we complete search in shadow DOM (i.e., step 3) below). When focus is already inside shadow DOM => |GetNextTabbableContent| is called with aStartContent inside shadow DOM, 1) |GetNextTabbableContentInAncestorScopes| is called since aRootContent differs from owner of aStartContent. So we search from scope including aStartContent. 2) |GetNextTabbableContentInAncestorScopes| searches in the scope including aStartContent and, if not found, search in the scope including the owner. Repeat searching until next content to focus is found or we are back to light tree. 3) When we are back to light tree, it means the shadow tree including aStartContent deep inside contains no next content to focus. So we keep searching in light tree with _startContent as the shadow host and aCurrentTabIndex as its tabindex_. An example: In [2], when |GetNextTabbableContent| is called with aStartContent as <input id=i2 slot=s1 tabindex=2></input>, 1) search in scope owned by s1 since s1 differs from document root. 2) search next content to s1 with s1's tabindex in scope owned by s2, next content to s2 with s2's tabindex in scope owned by <x-bar>, and then next content to <x-bar> with <x-bar>'s tabindex in scope owned by <x-foo>. 3) search next content to <x-foo> in document tree. [2] https://github.com/w3c/webcomponents/issues/375#issuecomment-178989178 > Can we enter here when we already are in shadow DOM. Should we check that content isn't in shadow DOM, or in a subtree which is somewhere assigned to a slot? > Can we enter here when we already are in shadow DOM? No. Frames of conents in shadow DOM should NOT enter here as |GetNextTabbableContentInAncestorScope| at the beginning of |GetNextTabbableContent| either found next content to focus and returned priorly OR updated aStartContent to shadow host that is in document tree. > Should we check that content isn't in shadow DOM, or in a subtree which is somewhere assigned to a slot? No. Skipping child frames of host's frame skips all descendant frames of host's frame, including subtree assigned to a slot.
Comment on attachment 8940603 [details] Bug 1413834 - part 3: Get next tabbable content in shadow DOM, https://reviewboard.mozilla.org/r/210820/#review218020 > I wonder... host isn't an owner, but ShadowRoot, I mean per spec. > I guess we don't play with ShadowRoots here. > But would be good to document somewhere. Yes I adopt host instead of shadow root since shadow root doesn't appear in flattened tree. Chrome also adopts host as owner per [1]. [1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/FocusController.cpp?type=cs&sq=package:chromium&l=143-144 > Something really weird here. Is aStartContent what kind of parameter? inout? Is it AddRefed or what? aStartContent is an inout parameter. It's the start point in shadow tree (in) and also returns the shadow host in light tree (out) to keep searching the next content in light tree, if no next content to focus in shadow tree. I should make this variable nsIContent\*. It's AddRefed outside this method by https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/dom/base/nsFocusManager.cpp#3079
Comment on attachment 8940601 [details] Bug 1413834 - part 1: Limit frame traversal and getting of next tabindex inside scope owned by document root, https://reviewboard.mozilla.org/r/210816/#review218006 > Shouldn't we skip also any elements in shadow dom or descendants of slot elements? We stop traversing downwards from shadow host so elements in shadow dom would be omitted as well. For slot elements I'll add MOZ_ASSERT(!child->IsHTMLElement(nsGkAtoms::slot)) as this method is only called from light tree.
Sorry I messed review comment and reply to them. Please refer to my reply comments on reviewboard directly.
Comment on attachment 8940603 [details] Bug 1413834 - part 3: Get next tabbable content in shadow DOM, https://reviewboard.mozilla.org/r/210820/#review218020 > Returns shadow host? What if that is not focusable? Doesn't matter. The shadow host in light DOM is NOT content to focus, but the starting point in light DOM to continue searching. As if GetNextTabbableContent starts at the host and searches next content in light DOM. > Why not just return the result as nsIContent* return value. Fixed in the latest revision. > Yes I adopt host instead of shadow root since shadow root doesn't appear in flattened tree. Chrome also adopts host as owner per [1]. > > [1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/page/FocusController.cpp?type=cs&sq=package:chromium&l=143-144 The comment of FindOwner in nsFocusManager.h already specifies it. Or do you mean to point out more obviously elsewhere? /** * Returns scope owner of aContent. * A scope owner is either a document root, shadow host, or slot. */ nsIContent* FindOwner(nsIContent* aContent); > Could you make this method iterative and not recursive. Fixed in the latest revision. > aResultContent is nsIContent** which in general should be AddRefed. So better to just change the return type to nsIContent* and do > return foo; when needed. Fixed in the latest revision. > aStartContent is an inout parameter. > > It's the start point in shadow tree (in) and also returns the shadow host in light tree (out) to keep searching the next content in light tree, if no next content to focus in shadow tree. > > I should make this variable nsIContent\*. It's AddRefed outside this method by > https://searchfox.org/mozilla-central/rev/7fb999d1d39418fd331284fab909df076b967ac6/dom/base/nsFocusManager.cpp#3079 Revised in the latest revision. Let me know for any feedback.
Updated patches per reviewer's comments. Olli, - Please refer to reviewboard for my replies on remaining issues. - Attached pdf illustrates purpose of new methods in patch 3. Hope it helps. Again, many thanks for the feedbacks.
Blocks: 1430020
Comment on attachment 8940601 [details] Bug 1413834 - part 1: Limit frame traversal and getting of next tabindex inside scope owned by document root, https://reviewboard.mozilla.org/r/210816/#review218154 ::: layout/base/nsFrameTraversal.cpp:441 (Diff revision 4) > nsFrameIterator::GetLastChild(nsIFrame* aFrame) > { > + if (mSkipShadow) { > + // Skip frames rendered from contents in shadow tree, > + // primarily for focus navigation > + nsIContent* content = aFrame->GetContent(); This looks wrong, what if the shadow host has display: contents (and thus has no frame)? We should arguably still avoid crossing shadow root boundaries, shouldn't we?
Comment on attachment 8940601 [details] Bug 1413834 - part 1: Limit frame traversal and getting of next tabindex inside scope owned by document root, https://reviewboard.mozilla.org/r/210816/#review218154 > This looks wrong, what if the shadow host has display: contents (and thus has no frame)? > > We should arguably still avoid crossing shadow root boundaries, shouldn't we? Yes we should. Let me add this check. Any other case am I missing?
Comment on attachment 8940601 [details] Bug 1413834 - part 1: Limit frame traversal and getting of next tabindex inside scope owned by document root, https://reviewboard.mozilla.org/r/210816/#review218302 I think that issue could be solved in a followup patch. (hopefully still in this bug) ::: layout/base/nsFrameTraversal.cpp:418 (Diff revision 4) > { > + if (mSkipShadow) { > + // Skip frames rendered from contents in shadow tree, > + // primarily for focus navigation > + nsIContent* content = aFrame->GetContent(); > + if (content && content->GetShadowRoot()) { ok, so we should skip more per display: contents issue. It would be handy to have some flag in nodes to tell whether they are somehow under a shadow root in flattened tree or something. But I guess if this all is just for sequential focusing, performance isn't too critical. We can easily check whether content is in shadow dom, and if not, need to go up in parent tree to see if some ancestor is assigned to a slot.
Attachment #8940601 - Flags: review?(bugs) → review+
Comment on attachment 8940602 [details] Bug 1413834 - part 2: Implement helper class to iterate contents in scope, https://reviewboard.mozilla.org/r/210818/#review218308 ::: dom/base/nsFocusManager.cpp:3199 (Diff revision 5) > + SetCurrent(mOwner); > + Next(); > + > + while (nsIContent* iter = GetCurrent()) { > + // Get tab index of mCurrent > + nsAutoString tabIndexStr; It would be nice to have a helper method for this tabindex checking and use that everywhere in nsFocusManager.
Attachment #8940602 - Flags: review?(bugs) → review+
Comment on attachment 8940603 [details] Bug 1413834 - part 3: Get next tabbable content in shadow DOM, https://reviewboard.mozilla.org/r/210820/#review218318 We need quite a bit tests here. ::: dom/base/nsFocusManager.cpp:3372 (Diff revision 6) > + int32_t* aCurrentTabIndex, > + bool aIgnoreTabIndex) > +{ > + nsIContent* startContent = *aStartContent; > + while (1) { > + nsCOMPtr<nsIContent> owner = FindOwner(startContent); why nsCOMPtr. No objects should die while iterating here, and extra AddRefs and Release are just slow. ::: dom/base/nsFocusManager.cpp:3673 (Diff revision 6) > else if (currentContent == aRootContent || > (currentContent != startContent && > (aForward || !GetRedirectedFocus(currentContent)))) { > + > + if (nsDocument::IsWebComponentsEnabled(aRootContent)) { > + MOZ_ASSERT(!currentContent->IsHTMLElement(nsGkAtoms::slot), What if documentElement is a slot element? (it wouldn't be inside ShadowRoot, but still a slot element)
Attachment #8940603 - Flags: review?(bugs) → review+
Comment on attachment 8940602 [details] Bug 1413834 - part 2: Implement helper class to iterate contents in scope, https://reviewboard.mozilla.org/r/210818/#review218308 > It would be nice to have a helper method for this tabindex checking and use that everywhere in nsFocusManager. Integrated into method |nsFocusManager::GetNextTabIndex| by traversing flat tree rather than DOM tree.
Comment on attachment 8940601 [details] Bug 1413834 - part 1: Limit frame traversal and getting of next tabindex inside scope owned by document root, https://reviewboard.mozilla.org/r/210816/#review218006 > We stop traversing downwards from shadow host so elements in shadow dom would be omitted as well. > > For slot elements I'll add > MOZ_ASSERT(!child->IsHTMLElement(nsGkAtoms::slot)) > as this method is only called from light tree. Removed the assertion as |nsFocusManager::GetNextTabIndex| may also be called from shadow DOM.
Comment on attachment 8940601 [details] Bug 1413834 - part 1: Limit frame traversal and getting of next tabindex inside scope owned by document root, https://reviewboard.mozilla.org/r/210816/#review218154 > Yes we should. Let me add this check. > > Any other case am I missing? Drop this issue and track below.
Comment on attachment 8940601 [details] Bug 1413834 - part 1: Limit frame traversal and getting of next tabindex inside scope owned by document root, https://reviewboard.mozilla.org/r/210816/#review218302 > ok, so we should skip more per display: contents issue. > It would be handy to have some flag in nodes to tell whether they are somehow under a shadow root in flattened tree or something. > But I guess if this all is just for sequential focusing, performance isn't too critical. > We can easily check whether content is in shadow dom, and if not, need to go up in parent tree to see if some ancestor is assigned to a slot. Patch 4 as the followup patch. Frame traversal doesn't enter scope of host with display: contents. Still one problem - all contents in the host's scope are omitted as well since the frameless host doesn't appear in frame traversal. Example: host3 in http://jsbin.com/vokafipoye/edit?html,output . Note Chrome doesn't have this problem. Need to think how to handle such case.
Blocks: 1430692
Comment on attachment 8940603 [details] Bug 1413834 - part 3: Get next tabbable content in shadow DOM, https://reviewboard.mozilla.org/r/210820/#review218318 > What if documentElement is a slot element? > (it wouldn't be inside ShadowRoot, but still a slot element) In this case we should not enter its scope for backward navigation since frame traversal already does so. Removed the assertion for such case in latest revision.
Blocks: 1430701
Attachment #8942591 - Flags: review?(emilio)
Attachment #8942591 - Flags: review?(bugs)
Comment on attachment 8942591 [details] Bug 1413834 - part 4: Limit frame traversal inside scope owned by document root when shadow host has no frame https://reviewboard.mozilla.org/r/212852/#review218816 ::: layout/base/nsFrameTraversal.cpp:456 (Diff revision 3) > - > nsIFrame* result = GetLastChildInner(aFrame); > if (mLockScroll && result && result->IsScrollFrame()) > return nullptr; > if (result && mFollowOOFs) { > result = nsPlaceholderFrame::GetRealFrameFor(result); emilio, could you check this placeholder frame handling makes sense with shadow DOM ::: layout/base/nsFrameTraversal.cpp:574 (Diff revision 3) > } > + > +// nsLightFrameIterator implementation > + > +nsIFrame* > +nsLightFrameIterator::GetFirstChildInner(nsIFrame* aFrame) { As the coding style says "Class and function definitions are not control structures; left brace goes by itself on the second line and without extra indentation, in general for C++." ::: layout/base/nsFrameTraversal.cpp:576 (Diff revision 3) > +// nsLightFrameIterator implementation > + > +nsIFrame* > +nsLightFrameIterator::GetFirstChildInner(nsIFrame* aFrame) { > + nsIFrame* child; > + for (child = nsFrameIterator::GetFirstChildInner(aFrame); Hmm, are there cases where some sibling is not light frame but some are, in cases we care it from focus handling point of view. Just thinking if we could optimize this a bit. This is very layout-ish code, so I defer to emilio's review there.
Attachment #8942591 - Flags: review?(bugs)
Comment on attachment 8942591 [details] Bug 1413834 - part 4: Limit frame traversal inside scope owned by document root when shadow host has no frame https://reviewboard.mozilla.org/r/212852/#review218822 r=me, though I still think that if we want to implement the spec the focus navigation code should be in terms of the DOM instead of the frame tree... Otherwise we're going to have annoying edge cases with display: contents forever :). But after discussing with smaug, this seems to be what we want for now. ::: layout/base/nsFrameTraversal.cpp:128 (Diff revision 3) > nsIFrame* GetPrevSiblingInner(nsIFrame* aFrame) override; > }; > > +// Frame iterator that walks only frames of light DOM contents without > +// ancestor assigned to a slot. Primarily for focus navigation. > +class nsLightFrameIterator: public nsFrameIterator nit: `final`? ::: layout/base/nsFrameTraversal.cpp:576 (Diff revision 3) > +// nsLightFrameIterator implementation > + > +nsIFrame* > +nsLightFrameIterator::GetFirstChildInner(nsIFrame* aFrame) { > + nsIFrame* child; > + for (child = nsFrameIterator::GetFirstChildInner(aFrame); Yeah, you need to handle display: contents unfortunately, so sibling frames can be basically anywhere in the light tree under the light tree sibling, which includes shadow root descendants. ::: layout/base/nsFrameTraversal.cpp:628 (Diff revision 3) > + return false; > + } > + > + // Return false if some ancestor is assigned to a slot > + for (; content; content = content->GetParent()) { > + if (content->GetAssignedSlot()) { It is kind of unfortunate to need to iterate the whole parent chain for every node here... :( Not that I have a particularly better idea. Maybe the iterator could do something like memoizing this... But maybe it doesn't matter anyway.
Attachment #8942591 - Flags: review?(emilio) → review+
Comment on attachment 8942591 [details] Bug 1413834 - part 4: Limit frame traversal inside scope owned by document root when shadow host has no frame https://reviewboard.mozilla.org/r/212852/#review218816 > As the coding style says > "Class and function definitions are not control structures; left brace goes by itself on the second line and without extra indentation, in general for C++." Revised in latest revision here and also other places in file.
Comment on attachment 8942591 [details] Bug 1413834 - part 4: Limit frame traversal inside scope owned by document root when shadow host has no frame https://reviewboard.mozilla.org/r/212852/#review218822 > nit: `final`? Revised in the latest revision.
(In reply to Emilio Cobos Álvarez [:emilio] from comment #89) > Comment on attachment 8942591 [details] > Bug 1413834 - part 4: Limit frame traversal inside scope owned by document > root when shadow host has no frame > > https://reviewboard.mozilla.org/r/212852/#review218822 > > r=me, though I still think that if we want to implement the spec the focus > navigation code should be in terms of the DOM instead of the frame tree... > Otherwise we're going to have annoying edge cases with display: contents > forever :). Or we should possibly rather change the spec if others agree. Using frame is after all conceptually the right thing to do since that is what user sees.
Comment on attachment 8942591 [details] Bug 1413834 - part 4: Limit frame traversal inside scope owned by document root when shadow host has no frame https://reviewboard.mozilla.org/r/212852/#review219250 ::: layout/base/nsFrameTraversal.cpp:633 (Diff revision 5) > + if (content->IsInShadowTree()) { > + return false; > + } > + > + // Return false if some ancestor is assigned to a slot > + for (; content; content = content->GetParent()) { Since this is rather slow, could you test this all by tabbing through some large document, say http://www.whatwg.org/specs/web-apps/current-work/ Perhaps even profile a bit where the time is spent during tabbing. Currently finding the new focusable element doesn't take too much time. Hmm, perhaps http://www.whatwg.org/specs/web-apps/current-work/ isn't good testcase, since it has so many focusable elements. Would need to profile a case where there are lots of elements, but only some focusables.
Attachment #8942591 - Flags: review?(bugs) → review+
Attached file a testcase —
profile from current Gecko https://perfht.ml/2DpC192
Comment on attachment 8940601 [details] Bug 1413834 - part 1: Limit frame traversal and getting of next tabindex inside scope owned by document root, https://reviewboard.mozilla.org/r/210816/#review218302 > Patch 4 as the followup patch. Frame traversal doesn't enter scope of host with display: contents. > > Still one problem - all contents in the host's scope are omitted as well since the frameless host doesn't appear in frame traversal. > Example: host3 in http://jsbin.com/vokafipoye/edit?html,output . Note Chrome doesn't have this problem. > > Need to think how to handle such case. Opened bug 1430701 to track the remaining problem as it involves larger change (comment 97).
Assignee: ben.tian → nobody
Assignee: nobody → bugs
FWIW, I profiled the patches and couldn't see anything too worrisome there.
ok, I thought this was causing OSX c3 failures, but they should be fixed by https://hg.mozilla.org/integration/mozilla-inbound/rev/617df9c9618b
Assigning back to Ben, since he did all the work. I'll just land so that it is easier to continue to fix other related bugs.
Assignee: bugs → ben.tian
Pushed by opettay@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/17d2cbc7bd3f part 1: Limit frame traversal and getting of next tabindex inside scope owned by document root, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/cc2457d116a4 part 2: Implement helper class to iterate contents in scope, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/f988728a10ae part 3: Get next tabbable content in shadow DOM, r=smaug https://hg.mozilla.org/integration/mozilla-inbound/rev/bf9cdce5d0b7 part 4: Limit frame traversal inside scope owned by document root when shadow host has no frame , r=smaug, emilio
Depends on: 1522092
No longer depends on: 1522092
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: