Bug 103477 – Make NodeRenderingContext::parentRenderer and nextRenderer top layer aware

• Component: New Bugs
• Status: RESOLVED FIXED

[Elliott Sprehn]

Make NodeRenderingContext::parentRenderer and nextRenderer htop layer aware

[Elliott Sprehn]

Created attachment 176385 [details]
Patch

[Elliott Sprehn]

Created attachment 176386 [details]
Patch

[WebKit Review Bot]

Comment on attachment 176386 [details]
Patch

Attachment 176386 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/15013319

New failing tests:
fast/dom/HTMLDialogElement/top-layer-display-none.html
fast/dom/HTMLDialogElement/top-layer-nesting.html

[Elliott Sprehn]

Created attachment 176395 [details]
Patch

[Elliott Sprehn]

(In reply to comment #4)
> Created an attachment (id=176395) [details]
> Patch

I fixed the brokenness but it seems there's much worse badness in top layer and flow threads and renderer hoisting since we do the normal parent renderer checks which seem wrong. :/

In general renderer hoisting seems to be very error prone, there's lots of oversights in the code.

[Julien Chaffraix]

Comment on attachment 176395 [details]
Patch

I really don't see the point of this change. You are peppering the knowledge of top layer elements all over NodeRenderingContext for no good reason I could see. Could you clarify what's the intent?

[Elliott Sprehn]

(In reply to comment #6)
> (From update of attachment 176395 [details])
> I really don't see the point of this change. You are peppering the knowledge of top layer elements all over NodeRenderingContext for no good reason I could see. Could you clarify what's the intent?

NodeRenderingContext(this).parentRenderer() and nextRenderer() return the wrong values for things that are in the top layer but are display none. This method that takes pointers by reference and reassigns them is also confusing to follow because it's not clear from the existing code that the parentRenderer is always the RenderView for top layer elements.

If you remove my comments about how the existing behavior that I didn't change is broken this is less code as well.

[Elliott Sprehn]

(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 176395 [details] [details])
> > I really don't see the point of this change. You are peppering the knowledge of top layer elements all over NodeRenderingContext for no good reason I could see. Could you clarify what's the intent?
> 
> NodeRenderingContext(this).parentRenderer() and nextRenderer() return the wrong values for things that are in the top layer but are display none. This method that takes pointers by reference and reassigns them is also confusing to follow because it's not clear from the existing code that the parentRenderer is always the RenderView for top layer elements.
> 
> If you remove my comments about how the existing behavior that I didn't change is broken this is less code as well.

Also note that as the code is currently written inside Element::rendererIsNeeded parentRenderer() and nextRenderer() return the wrong values for top layer elements and inside of NodeRenderingConext::shouldCreateRenderer() we use the wrong parent renderer for making decisions about if the renderer is needed.

[Hajime Morrita]

Comment on attachment 176395 [details]
Patch

Let's move isInTopLayer to NodeFlags from rareData. Then can we detect that non-toplayer case a bit earlier.
Also, I'd rather extract traversal code in separate function. It's apparently invasive.

By the way, is toplayer system already used by fullscreen?
If so, we can move fullscreen handling in createRendererForElementIfNeeded() to toplayer handling path.

[Hajime Morrita]

(In reply to comment #8)
> 
> Also note that as the code is currently written inside Element::rendererIsNeeded parentRenderer() and nextRenderer() return the wrong values for top layer elements and inside of NodeRenderingConext::shouldCreateRenderer() we use the wrong parent renderer for making decisions about if the renderer is needed.

Can we write any layout test that captures this case?

[Elliott Sprehn]

(In reply to comment #9)
> (From update of attachment 176395 [details])
> Let's move isInTopLayer to NodeFlags from rareData. Then can we detect that non-toplayer case a bit earlier.

Can you explain what you mean by earlier?

> Also, I'd rather extract traversal code in separate function. It's apparently invasive.

Okay, I'll extract it.

> 
> By the way, is toplayer system already used by fullscreen?
> If so, we can move fullscreen handling in createRendererForElementIfNeeded() to toplayer handling path.

No, it's not used there yet. We should migrate to it, but I don't know who owns that.

[Elliott Sprehn]

(In reply to comment #10)
> (In reply to comment #8)
> > 
> > Also note that as the code is currently written inside Element::rendererIsNeeded parentRenderer() and nextRenderer() return the wrong values for top layer elements and inside of NodeRenderingConext::shouldCreateRenderer() we use the wrong parent renderer for making decisions about if the renderer is needed.
> 
> Can we write any layout test that captures this case?

Yeah, it occurred to be that doing <col><dialog></col> should be broken right now or doing <div><dialog></div> and then div { content: url(...); }.

I'll add a test.

[Matt Falkenhagen]

(In reply to comment #11)
> > By the way, is toplayer system already used by fullscreen?
> > If so, we can move fullscreen handling in createRendererForElementIfNeeded() to toplayer handling path.
> 
> No, it's not used there yet. We should migrate to it, but I don't know who owns that.

I'm planning to get to that as part of bug 84796.

[Julien Chaffraix]

Comment on attachment 176395 [details]
Patch

As pointed out in comment #10, this change needs some testing. If this solves a bug then this change is not a refactoring (as you presented it).

[Matt Falkenhagen]

I think this is a real bug but I'm not sure how to write a layout test that covers it for the current code. I'm running into crashes in my patches for bug 105489 and bug 95946 because nextRenderer is not top layer aware. I'm not sure why the problem shows up in those patches and not for the current code.

We at least need a check for top layer in a similar way that we have a check for flow thread in nextRenderer:

// Renderers for elements attached to a flow thread should be skipped because they are parented differently.
if (renderer->node()->isElementNode() && renderer->style() && !renderer->style()->flowThread().isEmpty())
    continue;

[Elliott Sprehn]

(In reply to comment #15)
> I think this is a real bug but I'm not sure how to write a layout test that covers it for the current code. I'm running into crashes in my patches for bug 105489 and bug 95946 because nextRenderer is not top layer aware. I'm not sure why the problem shows up in those patches and not for the current code.
> 
> We at least need a check for top layer in a similar way that we have a check for flow thread in nextRenderer:
> 
> // Renderers for elements attached to a flow thread should be skipped because they are parented differently.
> if (renderer->node()->isElementNode() && renderer->style() && !renderer->style()->flowThread().isEmpty())
>     continue;

Yeah we need to fix that too. I'll upload another patch that contains that and a test for the original case we identified with <col>.

[Matt Falkenhagen]

Created attachment 181483 [details]
Patch

[Hajime Morrita]

Comment on attachment 181483 [details]
Patch

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

> Source/WebCore/dom/NodeRenderingContext.cpp:81
> +#if ENABLE(DIALOG_ELEMENT)

Could you move this to NodeRenderingTraversal?
It helps fast paths to go fast.
See http://trac.webkit.org/changeset/137715 for detail.

> Source/WebCore/dom/NodeRenderingContext.cpp:112
> +#endif

Ditto.

> Source/WebCore/dom/NodeRenderingContext.cpp:126
> +#if ENABLE(DIALOG_ELEMENT)

Ditto.

[Matt Falkenhagen]

(In reply to comment #16)
> Yeah we need to fix that too. I'll upload another patch that contains that and a test for the original case we identified with <col>.

Hi Elliott, I've went ahead and done this. What do you think?

I'd like to fix this so I can continue with bug 105489 and bug 95946.

BTW I couldn't immediately create a layout test that fails with <col><dialog></col>. Since <col> doesn't have a renderer, m_renderingParent and parentRenderer() is just the first ancestor with a renderer.

(In reply to comment #18)
> Could you move this to NodeRenderingTraversal?
> It helps fast paths to go fast.
> See http://trac.webkit.org/changeset/137715 for detail.

Thanks Morita-san! I'll look into this.

[Elliott Sprehn]

Comment on attachment 181483 [details]
Patch

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

Add a test with display: table-column on an element around a <dialog>. I think we should land this and get things working and then refactor this out into a separate class like TopLayerController where you'd document()->topLayer()->nextRendererForNode(...) and such and move all the complicated loops out of NodeRenderingContext.

>> Source/WebCore/dom/NodeRenderingContext.cpp:81
>> +#if ENABLE(DIALOG_ELEMENT)
> 
> Could you move this to NodeRenderingTraversal?
> It helps fast paths to go fast.
> See http://trac.webkit.org/changeset/137715 for detail.

This really doesn't make sense in NodeRenderingTraversal. If we want to factor things out we should do what Regions does and have a TopLayerController that manages all this stuff like RenderNamedFlowThread::nextRendererForNode.

>> Source/WebCore/dom/NodeRenderingContext.cpp:112
>> +#endif
> 
> Ditto.

@morrita: I'm not sure what you want moved here, but this code should be here. We want to continue in this loop and skip all renderers that are in a different "flow thread" like regions (which is already here) or top layer.

>> Source/WebCore/dom/NodeRenderingContext.cpp:126
>> +#if ENABLE(DIALOG_ELEMENT)
> 
> Ditto.

This assert should be here. Please don't move this stuff out.

[Hajime Morrita]

Comment on attachment 181483 [details]
Patch

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

>>> Source/WebCore/dom/NodeRenderingContext.cpp:81
>>> +#if ENABLE(DIALOG_ELEMENT)
>> 
>> Could you move this to NodeRenderingTraversal?
>> It helps fast paths to go fast.
>> See http://trac.webkit.org/changeset/137715 for detail.
> 
> This really doesn't make sense in NodeRenderingTraversal. If we want to factor things out we should do what Regions does and have a TopLayerController that manages all this stuff like RenderNamedFlowThread::nextRendererForNode.

Adding TopLayerController sounds good.
What I hope is to keep NodeRenderingContext maintainable and clarify where the happy path goes.

>>> Source/WebCore/dom/NodeRenderingContext.cpp:112
>>> +#endif
>> 
>> Ditto.
> 
> @morrita: I'm not sure what you want moved here, but this code should be here. We want to continue in this loop and skip all renderers that are in a different "flow thread" like regions (which is already here) or top layer.

Right. Then we could just extract the skip condition to a RenderObject method or some free floating function to make this part clearer.

>>> Source/WebCore/dom/NodeRenderingContext.cpp:126
>>> +#if ENABLE(DIALOG_ELEMENT)
>> 
>> Ditto.
> 
> This assert should be here. Please don't move this stuff out.

Right.

[Matt Falkenhagen]

Created attachment 181636 [details]
Patch

[Matt Falkenhagen]

Thanks for the comments. I uploaded a new patch with a layout test for display: table-column and a helper function for the skip condition in next/previousRenderer. Also I've removed the FIXME comment about <dialog> inside <col> since it seems to work.

[Elliott Sprehn]

Comment on attachment 181636 [details]
Patch

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

This looks good to me. We can refactor out the logic into a TopLayerController in a follow up patch.

> Source/WebCore/dom/NodeRenderingContext.cpp:76
> +static bool isRendererReparented(RenderObject* renderer)

Nice! const RenderObject* renderer for the argument.

[Matt Falkenhagen]

Created attachment 181812 [details]
Patch

[WebKit Review Bot]

Comment on attachment 181812 [details]
Patch

Clearing flags on attachment: 181812

Committed r139154: <http://trac.webkit.org/changeset/139154>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.