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

Status: RESOLVED FIXED

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

Description

[Kevin Grandon :kgrandon]

+++ 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 = '';

Comment 1

[Kevin Grandon :kgrandon]

Attachment: 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.

Comment 2

[Wilson Page [:wilsonpage]]

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?

Comment 3

[Wilson Page [:wilsonpage]]

kgrandon: Can you let me know how you worked around this bug in relation to data-l10n-id location.

Comment 4

[Wilson Page [:wilsonpage]]

CORRECTION (comment 3): *'data-l10n-id localization'

Comment 5

[Kevin Grandon :kgrandon]

(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.

Comment 6

[William Chen [:wchen]]

Attachment: 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.

Comment 7

[Wilson Page [:wilsonpage]]

Have we got anyone to review this? Where currently having to do quite a few reflow hacks across Gaia as a workaround.

Comment 8

[Boris Zbarsky [:bzbarsky]]

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.

Comment 9

[William Chen [:wchen]]

(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

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/a2e93281d3f5

Comment 11

[Wilson Page [:wilsonpage]]

wchen: The bug [1] still reproduces on today's Nightly.

[1] http://gaia-components.github.io/gaia-header/

Comment 12

[Wilson Page [:wilsonpage]]

(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 :)