[ShadowDOM] Changing textContent on 'projected content' nodes makes them disappear

RESOLVED FIXED in mozilla34

Status

()

Core
DOM
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kgrandon, Assigned: wchen)

Tracking

unspecified
mozilla34
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
+++ This bug was initially created as a clone of Bug #1007743 +++

It seems the landing of bug 1007743 fixed one use-case, but broke another use case of updating textContent of content nodes. The specific case is changing the locale of the device when using the vertical homescreen.

I'm still working on getting together a reduced test case for this. A workaround I have for this is:

element.style.display = 'none';
element.clientTop;
element.style.display = '';
(Reporter)

Updated

4 years ago
Blocks: 1022423
(Reporter)

Comment 1

4 years ago
Created attachment 8437210 [details]
bug_1022866_test.zip

Here is a slightly reduced test case than "the entire homescreen" of FirefoxOS. If you open examples/index.html in nightly, you will see some text content that will disappear shortly after loading.

Let me know how this works out for you. I tried replicating our implementation into something like jsfiddle, but haven't had much luck yet.
I have created a reduced test case included in the main gaia-header example page.

http://gaia-components.github.io/gaia-header/

wchen: Could you give us some more info and possibly and roughly when we could expect a fix to land?
Flags: needinfo?(wchen)
Blocks: 1011602
kgrandon: Can you let me know how you worked around this bug in relation to data-l10n-id location.
No longer blocks: 1011602
Flags: needinfo?(kgrandon)
Blocks: 1011602
CORRECTION (comment 3): *'data-l10n-id localization'
(Reporter)

Comment 5

4 years ago
(In reply to Wilson Page [:wilsonpage] from comment #3)
> kgrandon: Can you let me know how you worked around this bug in relation to
> data-l10n-id location.

I believe we still need the terrible layout flush with clientTop workaround for this. For l10n things I think we generally just populate on insertion which mostly just works.
Flags: needinfo?(kgrandon)
(Assignee)

Comment 6

4 years ago
Created attachment 8463760 [details] [diff] [review]
Fix insertion point frame for content distributed to insertion points.

We are currently getting the wrong container frame for content that is distributed into insertion points.

This fixes the attached test case and the header demo linked in this bug.

The attached test case itself has a bug, there is an insertion point in the grid that selects on .icons, but the icons have class "icon". Nothing should be rendered, the fact that something does get rendered is a bug that is also fixed by this patch.

This patch also fixes GetFlattenedTreeParent() to handle a couple more cases that were missing before.
Flags: needinfo?(wchen)
Have we got anyone to review this? Where currently having to do quite a few reflow hacks across Gaia as a workaround.
Flags: needinfo?(wchen)
(Assignee)

Updated

4 years ago
Attachment #8463760 - Flags: review?(bzbarsky)
Flags: needinfo?(wchen)
Comment on attachment 8463760 [details] [diff] [review]
Fix insertion point frame for content distributed to insertion points.

So why are the changes to GetFlattenedTreeParent needed?  In which cases do they change what it returns?   Is it that it can now return null when it used to return GetParent(), if we're distributed but have no insertion point?  If so, that's worth documenting clearly.

>+  ShadowRoot* shadow = ShadowRoot::FromNode(const_cast<nsIContent*>(aContent));

Why not just have HasDistributedChildren() take a non-const pointer?  I see no reason for it to take a const one.

>+  if (aContent->IsHTML(nsGkAtoms::content)) {

Should probably just have a FromContent method on HTMLContentElement, like other HTML elements do.  We even have convenience macros for that sort of thing.

>+++ b/layout/base/nsCSSFrameConstructor.cpp
>+      nsIContent* flattenedParent = aChildContent->GetFlattenedTreeParent();

So here's a question.  Is this ever the _wrong thing to return from this code?  That is, can we replace this whole branch of the if with just returning the GetFlattenedTreeParent?  Followup bug would be ok for that, obviously.

>+      *aMultiple = true;

Shadow DOM has no concept of "default catch-all insertion point"?

r=me with the const thing fixed, at least.
Attachment #8463760 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 9

4 years ago
(In reply to On vacation Aug 5-18.  Please do not request review. from comment #8)
> Comment on attachment 8463760 [details] [diff] [review]
> they change what it returns?   Is it that it can now return null when it
> used to return GetParent(), if we're distributed but have no insertion
> point?  If so, that's worth documenting clearly.

Yes, that's the case.

> So here's a question.  Is this ever the _wrong thing to return from this
> code?  That is, can we replace this whole branch of the if with just
> returning the GetFlattenedTreeParent?  Followup bug would be ok for that,
> obviously.

I think it should be fine to just return GetFlattenedTreeParent, but it's hard to show whether the two are equivalent in this case.
 
> Shadow DOM has no concept of "default catch-all insertion point"?

No, it doesn't. It has a catch-all insertion point, but nothing that behaves like XBL's default insertion point. We could optimize the append multiple elements case for Shadow DOM in particular situations.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a2e93281d3f5
Assignee: nobody → wchen
Flags: in-testsuite+
https://hg.mozilla.org/mozilla-central/rev/a2e93281d3f5
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
wchen: The bug [1] still reproduces on today's Nightly.

[1] http://gaia-components.github.io/gaia-header/
Flags: needinfo?(wchen)
(In reply to Wilson Page [:wilsonpage] from comment #11)
> wchen: The bug [1] still reproduces on today's Nightly.
> 
> [1] http://gaia-components.github.io/gaia-header/

Now fixed in latest nightly :)
Flags: needinfo?(wchen)
You need to log in before you can comment on or make changes to this bug.