Bug 63977 – [ShadowContentElement] Redundant RenderText objects are created on the content boundaries

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Hajime Morrita]

Running small test suite for Bug 63504 found that
some content child pattern makes redundant RenderText node.

[Hajime Morrita]

Created attachment 99797 [details]
Patch

[Hajime Morrita]

Hi Dimitri, could you take a look?

After this fix, I'm planning to make the "flatten tree" concept clear in the code
by extracting a FlattenTreeTraverser or something.
Currently the concept is mixed into NodeRenderingContext, which is sad situation.

[Dominic Cooney]

Comment on attachment 99797 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99797&action=review

Trivial comments inline.

> LayoutTests/ChangeLog:9
> +        One is for shadow tree which has one content elmement,

nit: elmement → element

> LayoutTests/ChangeLog:12
> +        Then the test makes 2 complete DOM trees (dom tree with shadow and

nit: dom → DOM
for consistency

> LayoutTests/ChangeLog:14
> +        compare their layout result.

I love this testing approach.

> LayoutTests/fast/dom/shadow/content-element-renderers-expected.txt:1
> +This test compares a shadow-based render tree with one for a refrence DOM tree.

nit: refrence → reference

> LayoutTests/fast/dom/shadow/content-element-renderers.html:13
> +    document.getElementById("console").innerText += (message + "\n");

WebKit zeitgeist seems to prefer textContent over innerText?

> LayoutTests/fast/dom/shadow/content-element-renderers.html:57
> +    //var enhancedHtml = html.replace(/<span\/>/g, "<span>#span</span>").replace(/<div\/>/g, "<div>#div</div>").replace(/<content\/>/g, "<content></content>");

Don’t need it? Delete it?

> LayoutTests/fast/dom/shadow/content-element-renderers.html:58
> +    var enhancedHtml = html.replace(/<span\/>/g, "<span> </span>").replace(/<div\/>/g, "<div> </div>").replace(/<content\/>/g, "<content></content>").replace(/#text/g, " ");

I am curious… why the whitespace in span and div but not content?

> LayoutTests/fast/dom/shadow/content-element-renderers.html:61
> +    var contentPlaceholder = root.getElementsByTagName("CONTENT")[0];

Is root.querySelector('content') equivalent and more succinct?

> LayoutTests/fast/dom/shadow/content-element-renderers.html:83
> +    moveChildren(content, host);

Why do you create a blockquote in createTreeFrom(contentHtml) and then move the nodes into another blockquote that is host?

> LayoutTests/fast/dom/shadow/content-element-renderers.html:93
> +    container.removeChild

Oops.

> LayoutTests/fast/dom/shadow/content-element-renderers.html:121
> +    

Delete this blank line.

> LayoutTests/fast/dom/shadow/content-element-renderers.html:133
> +    layoutTestController.dumpAsText();

Maybe add a description explaining that this test can not be run in the browser?

> LayoutTests/fast/dom/shadow/content-element-renderers.html:136
> +

Delete this blank line.

[Dimitri Glazkov (Google)]

Comment on attachment 99797 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99797&action=review

pls address the nits.

>> LayoutTests/fast/dom/shadow/content-element-renderers.html:13
>> +    document.getElementById("console").innerText += (message + "\n");
> 
> WebKit zeitgeist seems to prefer textContent over innerText?

yup -- and don't use +=, as abbreviated as it sounds, it's highly inefficient.

How's this:

var log = document.getElementById("console");
log.info = function(message)
{
    this.appendChild(document.createElement("div")).textContent = message;
}
// ...
log.info('foo');

[Dimitri Glazkov (Google)]

(In reply to comment #2)
> Hi Dimitri, could you take a look?
> 
> After this fix, I'm planning to make the "flatten tree" concept clear in the code
> by extracting a FlattenTreeTraverser or something.
> Currently the concept is mixed into NodeRenderingContext, which is sad situation.

Let's design this first. I would like to understand the approach better before landing patches.

[Hajime Morrita]

Created attachment 99921 [details]
Asking cq

[Hajime Morrita]

Comment on attachment 99797 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=99797&action=review

Hi Domini, Dimitri, thanks for reviewing! I updated the patch and asking cq to land it.

>> LayoutTests/ChangeLog:9
>> +        One is for shadow tree which has one content elmement,
> 
> nit: elmement → element

fixed.

>> LayoutTests/ChangeLog:12
>> +        Then the test makes 2 complete DOM trees (dom tree with shadow and
> 
> nit: dom → DOM
> for consistency

fixed.

>> LayoutTests/fast/dom/shadow/content-element-renderers-expected.txt:1
>> +This test compares a shadow-based render tree with one for a refrence DOM tree.
> 
> nit: refrence → reference

fixed.

>>> LayoutTests/fast/dom/shadow/content-element-renderers.html:13
>>> +    document.getElementById("console").innerText += (message + "\n");
>> 
>> WebKit zeitgeist seems to prefer textContent over innerText?
> 
> yup -- and don't use +=, as abbreviated as it sounds, it's highly inefficient.
> 
> How's this:
> 
> var log = document.getElementById("console");
> log.info = function(message)
> {
>     this.appendChild(document.createElement("div")).textContent = message;
> }
> // ...
> log.info('foo');

Rewrote to Dimitri version.

>> LayoutTests/fast/dom/shadow/content-element-renderers.html:61
>> +    var contentPlaceholder = root.getElementsByTagName("CONTENT")[0];
> 
> Is root.querySelector('content') equivalent and more succinct?

No strong reason. Just because I'm from pre-CSS era...

>> LayoutTests/fast/dom/shadow/content-element-renderers.html:83
>> +    moveChildren(content, host);
> 
> Why do you create a blockquote in createTreeFrom(contentHtml) and then move the nodes into another blockquote that is host?

It's because ShadowRoot doesn't support innerHTML but I'd like to define test data as HTML text.

>> LayoutTests/fast/dom/shadow/content-element-renderers.html:93
>> +    container.removeChild
> 
> Oops.

Whoa...

>> LayoutTests/fast/dom/shadow/content-element-renderers.html:121
>> +    
> 
> Delete this blank line.

Fixed.

>> LayoutTests/fast/dom/shadow/content-element-renderers.html:133
>> +    layoutTestController.dumpAsText();
> 
> Maybe add a description explaining that this test can not be run in the browser?

Added.

>> LayoutTests/fast/dom/shadow/content-element-renderers.html:136
>> +
> 
> Delete this blank line.

Done.

[Hajime Morrita]

(In reply to comment #5)
> (In reply to comment #2)
> > Hi Dimitri, could you take a look?
> > 
> > After this fix, I'm planning to make the "flatten tree" concept clear in the code
> > by extracting a FlattenTreeTraverser or something.
> > Currently the concept is mixed into NodeRenderingContext, which is sad situation.
> 
> Let's design this first. I would like to understand the approach better before landing patches.
Okay, I have no concrete idea yet. So I'll write a note for this.

[WebKit Review Bot]

Comment on attachment 99921 [details]
Asking cq

Clearing flags on attachment: 99921

Committed r90536: <http://trac.webkit.org/changeset/90536>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Hajime Morrita]

> > > After this fix, I'm planning to make the "flatten tree" concept clear in the code
> > > by extracting a FlattenTreeTraverser or something.
> > > Currently the concept is mixed into NodeRenderingContext, which is sad situation.
> > 
> > Let's design this first. I would like to understand the approach better before landing patches.
> Okay, I have no concrete idea yet. So I'll write a note for this.

Hi Dimitri, I put the note on Bug 64072.
Although the idea is very rough, I'd love to hear your opinion!