Bug 89384 – Looks like wrong position of isSVGForeignObject checks in RenderObject::container

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Abhishek Arya]

RenderObject* RenderObject::container(const RenderBoxModelObject* repaintContainer, bool* repaintContainerSkipped) const
{
...................

    EPosition pos = m_style->position();
    if (pos == FixedPosition) {
        // container() can be called on an object that is not in the
        // tree yet.  We don't call view() since it will assert if it
        // can't get back to the canvas.  Instead we just walk as high up
        // as we can.  If we're in the tree, we'll get the root.  If we
        // aren't we'll get the root of our little subtree (most likely
        // we'll just return 0).
        // FIXME: The definition of view() has changed to not crawl up the render tree.  It might
        // be safe now to use it.
        while (o && o->parent() && !(o->hasTransform() && o->isRenderBlock())) {
            if (repaintContainerSkipped && o == repaintContainer)
                *repaintContainerSkipped = true;
#if ENABLE(SVG)
            // foreignObject is the containing block for its contents.
            if (o->isSVGForeignObject())
                break;
#endif
            o = o->parent();
        }
    } else if (pos == AbsolutePosition) {
        // Same goes here.  We technically just want our containing block, but
        // we may not have one if we're part of an uninstalled subtree.  We'll
        // climb as high as we can though.
        while (o && o->style()->position() == StaticPosition && !o->isRenderView() && !(o->hasTransform() && o->isRenderBlock())) {
            if (repaintContainerSkipped && o == repaintContainer)
                *repaintContainerSkipped = true;
#if ENABLE(SVG)
            if (o->isSVGForeignObject()) // foreignObject is the containing block for contents inside it
                break;


---------------------------
Shouldn't 'if (o->isSVGForeignObject()' be before the 'if (repaintContainerSkipped && o == repaintContainer)' check for both fixed positioned objects, absolute positioned objects. Can someone in the SVG team take a look and fix this.

[Stephen Chenney]

It definitely seems wrong. We are not skipping the object if it's SVG - we're breaking out. So we shouldn't be setting the flag as skipped.

Go ahead and fix, or ask pdr or fmalita. I'm not bug fixing while traveling.

[Abhishek Arya]

Created attachment 148200 [details]
Patch

[Dirk Schulze]

Comment on attachment 148200 [details]
Patch

Is it possible to test it with a DRT test?

[Abhishek Arya]

(In reply to comment #3)
> (From update of attachment 148200 [details])
> Is it possible to test it with a DRT test?

I don't know the painting pipeline well enough to write a failing test.

[Simon Fraser (smfr)]

Patch seems reasonable; we might never hit this on real content.

repaintContainerSkipped kicks in when something like opacity creates a compositing layer between an abspos div and its container ancestor:

<div style="position: relative">
  <div style="opacity: 0.5">
    <div style="-webkit-transform: translateZ(0);"> <!-- kick the next div into compositing -->
    <div style="position: absolute;">

For isSVGForeignObject to kick in, you need to be inside an svg <foreignObject> and to have the repaintContainer in the SVG. Since svg doesn't do compositing, I'm not sure this is possible.

[Abhishek Arya]

(In reply to comment #5)
> Patch seems reasonable; we might never hit this on real content.
> 
> repaintContainerSkipped kicks in when something like opacity creates a compositing layer between an abspos div and its container ancestor:
> 
> <div style="position: relative">
>   <div style="opacity: 0.5">
>     <div style="-webkit-transform: translateZ(0);"> <!-- kick the next div into compositing -->
>     <div style="position: absolute;">
> 
> For isSVGForeignObject to kick in, you need to be inside an svg <foreignObject> and to have the repaintContainer in the SVG. Since svg doesn't do compositing, I'm not sure this is possible.

Thanks Simon.  So, looks like there is not way to test this. Can you please review this so that atleast we don't regress in the future.

[WebKit Review Bot]

Comment on attachment 148200 [details]
Patch

Clearing flags on attachment: 148200

Committed r120732: <http://trac.webkit.org/changeset/120732>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.