Closed Bug 1430692 Opened 2 years ago Closed 2 years ago

Follow-up of bug 1413834: Handle focus navigation on NAC in shadow DOM

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla61
Tracking Status
firefox61 --- fixed

People

(Reporter: ben.tian, Assigned: smaug)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

Per bug 1413834 comment 43:

"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."
Priority: -- → P2
Assignee: nobody → bugs
remote: View your change here:
remote:   https://hg.mozilla.org/try/rev/189516f166d9ca51fe9eb9083394b73649a88306
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=189516f166d9ca51fe9eb9083394b73649a88306
remote: recorded changegroup in replication log in 0.098s


Ok, this is a bit painful. Currently we use frame tree for tab navigation in light DOM, since that has been our behavior historically and it actually gives the sanest behavior. But for Shadow DOM we follow the Shadow DOM spec. However, DOM tree traversal doesn't know anything about native anonymous content, so this patch makes us use frame tree again for NAC.

We can get rid of this stuff once we convert date and such elements to use shadow DOM internally.
Attachment #8970680 - Flags: review?(mrbkap)
Comment on attachment 8970680 [details] [diff] [review]
nac_in_shadow_focus_with_test.diff

oops, this needs rebasing.
Attachment #8970680 - Flags: review?(mrbkap)
rebased
Attachment #8970680 - Attachment is obsolete: true
Attachment #8970696 - Flags: review?(mrbkap)
remote: View the pushlog for these changes here:
remote:   https://hg.mozilla.org/try/pushloghtml?changeset=2262e39a9d94348e6c8ffbbab6a65be6f2ee924c
remote: 
remote: Follow the progress of your build on Treeherder:
remote:   https://treeherder.mozilla.org/#/jobs?repo=try&revision=2262e39a9d94348e6c8ffbbab6a65be6f2ee924c
remote: recorded changegroup in replication log in 0.100s
Comment on attachment 8970696 [details] [diff] [review]
nac_in_shadow_focus_with_test_2.diff

Review of attachment 8970696 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsFocusManager.cpp
@@ +3275,5 @@
> +
> +      if (firstNonChromeOnly && firstNonChromeOnly == iterContent) {
> +        // We just broke out from the native anonynous content, so move
> +        // to the previous/next node of the native anonymous owner.
> +        aForward ? contentTraversal.Next() : contentTraversal.Prev();

Please write this as a normal if/else.

@@ +3314,5 @@
> +            nsIFrame* last = frame;
> +            while (last) {
> +              frame = last;
> +              last = frame->PrincipalChildList().LastChild();
> +            };

Nit: exta ; after the }s.

(also, this could be a for loop: for (nsIFrame* last = frame; last; last = frame->PrincipalChildList().LastChild()) frame = last;)
Attachment #8970696 - Flags: review?(mrbkap) → review+
oops, that while used to be do-while.
better to compile too
Attachment #8971507 - Attachment is obsolete: true
Pushed by opettay@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b51c5cf8035
Handle focus navigation on NAC in shadow DOM, r=mrbkap
https://hg.mozilla.org/mozilla-central/rev/4b51c5cf8035
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1507101
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.