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

VERIFIED FIXED in mozilla34

Status

()

Core
DOM
VERIFIED FIXED
3 years ago
3 years ago

People

(Reporter: wilsonpage, Assigned: smaug)

Tracking

(Blocks: 1 bug)

unspecified
mozilla34
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-moztrap -

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

3 years ago
EXAMPLE

http://jsbin.com/xihiy/1/edit
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: 1026047
(Assignee)

Updated

3 years ago
Assignee: nobody → bugs
(Assignee)

Comment 2

3 years ago
Created attachment 8463637 [details] [diff] [review]
patch

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?
(Assignee)

Comment 5

3 years ago
Wait, I attached the patch to wrong bug.
(Assignee)

Updated

3 years ago
Attachment #8463637 - Attachment is obsolete: true

Updated

3 years ago
QA Whiteboard: [2.1-feature-qa+]

Updated

3 years ago
QA Whiteboard: [2.1-feature-qa+] → [2.1-feature-qa-]
(Assignee)

Comment 6

3 years ago
Getting close with this. Focus, tab navigation, selection and caret are now working locally, need
to sort out :focus handling still.
(Assignee)

Comment 7

3 years ago
Created attachment 8467895 [details] [diff] [review]
patch

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+
(Assignee)

Comment 9

3 years ago
(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.
(Assignee)

Comment 10

3 years ago
Created attachment 8468111 [details] [diff] [review]
v3

Didn't change focusmanager part.
(Assignee)

Comment 11

3 years ago
(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!
(Assignee)

Comment 12

3 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/a0778b3a1c04
(Assignee)

Comment 13

3 years ago
And I'll write test cases once we have event handling working.
https://hg.mozilla.org/mozilla-central/rev/a0778b3a1c04
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34

Updated

3 years ago
QA Whiteboard: [2.1-feature-qa-]
Whiteboard: [2.1-feature-qa+]

Updated

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