Closed Bug 1026164 Opened 10 years ago Closed 10 years ago

[WebComponents] Text inputs can't be focused when in shadow dom

Categories

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

x86
macOS
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla34

People

(Reporter: wilsonpage, Assigned: smaug)

References

(Blocks 1 open bug)

Details

(Whiteboard: [2.1-feature-qa+])

Attachments

(2 files, 1 obsolete file)

Yes, pretty much anything that involves focus, tabbing, event targeting, layout, geometry information, etc, is broken in shadow DOM due to bug 1026047.
Depends on: shadowcurrentdoc
Assignee: nobody → bugs
Attached patch patch (obsolete) — Splinter Review
I'll be going through more Get*Doc usage, but this is certainly needed.

https://tbpl.mozilla.org/?tree=Try&rev=72526d803c62
Attachment #8463637 - Flags: review?(bzbarsky)
Comment on attachment 8463637 [details] [diff] [review]
patch

r=me
Attachment #8463637 - Flags: review?(bzbarsky) → review+
Came across this and noticed that the test was for bug 1025933? Is this the same issue, if so - should we block that bug on this?
Wait, I attached the patch to wrong bug.
Attachment #8463637 - Attachment is obsolete: true
QA Whiteboard: [2.1-feature-qa+]
QA Whiteboard: [2.1-feature-qa+] → [2.1-feature-qa-]
Getting close with this. Focus, tab navigation, selection and caret are now working locally, need
to sort out :focus handling still.
Attached patch patchSplinter Review
Mostly just changing GetCurrentDoc to GetComposedDoc in case we're dealing with something which should work also in shadow dom.

CSSFC change is required to make input element work. Input/Editor/nsTextEditorState really can't handle the case when frames are recreated all the time for native anonymous content. So filtering out NAC, since those shouldn't be relevant for the shadow dom rendering.
(I tried several other approaches but they just end up being very complicated or not working properly)

There are few GetCurrentDoc()->GetComposedDoc() changes which aren't needed for
this bug, but would need to be changed eventually anyway.

https://tbpl.mozilla.org/?tree=Try&rev=62c73c49347c
Attachment #8467895 - Flags: review?(wchen)
Comment on attachment 8467895 [details] [diff] [review]
patch

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

::: dom/base/nsFocusManager.cpp
@@ +3166,5 @@
>    nsCOMPtr<nsIDocument> doc;
>    nsCOMPtr<nsIDocShell> startDocShell;
>  
>    if (aStartContent) {
> +    doc = aStartContent->GetComposedDoc();

Later in the function, we are walking the parent chain using GetParent(), which probably doesn't break anything right now because we don't put any XUL panels in Shadow DOM, but the different parent chain between XBL and Shadow DOM is another thing to keep in mind when switching from Get/InDoc to Get/InComposedDoc. I'm guessing in most cases we do this, we also want to switch to walking the flattened tree parent.

::: layout/base/nsCSSFrameConstructor.cpp
@@ +6746,4 @@
>      // Recreate frames if content is appended into a ShadowRoot
>      // because children of ShadowRoot are rendered in place of children
>      // of the host.
> +    //XXXsmaug This is super unefficient!

I have a WIP to get rid of this.

::: layout/base/nsPresShell.cpp
@@ +3529,5 @@
>                                   nsIPresShell::ScrollAxis aHorizontal,
>                                   uint32_t                 aFlags)
>  {
>    NS_ENSURE_TRUE(aContent, NS_ERROR_NULL_POINTER);
> +  nsCOMPtr<nsIDocument> currentDoc = aContent->GetComposedDoc();

We probably want to rename this variable.

::: layout/style/nsComputedDOMStyle.cpp
@@ +525,5 @@
>  /* static */
>  nsIPresShell*
>  nsComputedDOMStyle::GetPresShellForContent(nsIContent* aContent)
>  {
> +  nsIDocument* currentDoc = aContent->GetComposedDoc();

We probably want to rename this variable as well.
Attachment #8467895 - Flags: review?(wchen) → review+
(In reply to William Chen [:wchen] from comment #8)
> Later in the function, we are walking the parent chain using GetParent(),
Yeah, we need to audit also _all_ the GetParent and GetParentNode calls.
Attached patch v3Splinter Review
Didn't change focusmanager part.
(In reply to William Chen [:wchen] from comment #8)
> @@ +6746,4 @@
> >      // Recreate frames if content is appended into a ShadowRoot
> >      // because children of ShadowRoot are rendered in place of children
> >      // of the host.
> > +    //XXXsmaug This is super unefficient!
> 
> I have a WIP to get rid of this.
Great!
And I'll write test cases once we have event handling working.
https://hg.mozilla.org/mozilla-central/rev/a0778b3a1c04
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
QA Whiteboard: [2.1-feature-qa-]
Whiteboard: [2.1-feature-qa+]
Flags: in-moztrap-
Marking verified since there's no QA support planned to be done here, so verification isn't needed here.
Status: RESOLVED → VERIFIED
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: