Bug 135075 – Put position:fixed elements into layers when a WK1 view is layer-backed

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Beth Dakin]

We should put position:fixed elements into layers when a WK1 view is layer-backed. Otherwise they currently do not repaint correctly.

<rdar://problem/17682335>

[Beth Dakin]

Created attachment 235153 [details]
Patch

[Darin Adler]

Comment on attachment 235153 [details]
Patch

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

Looks like it will work.

> Source/WebCore/css/StyleResolver.cpp:1261
>          || (style.position() == FixedPosition && e && e->document().page() && e->document().page()->settings().fixedPositionCreatesStackingContext())
> +        || (style.position() == FixedPosition && e && e->document().page() && e->document().page()->chrome().client().requiresAcceleratedCompositingForViewportConstrainedPosition())

Could we add a helper function to make this code easier to read?

    || (style.position() == FixedPosition && shouldSomething(e))

Then the code in the helper function would say:

    if (!element)
        return false;
    Page* page = element->document().page();
    if (!page)
        return false;
    return page->settings().fixedPositionCreatesStackingContext()
        || page->chrome().client().requiresAcceleratedCompositingForViewportConstrainedPosition();

And we could even put some “why” comment in there. It can be an inline function if needed so the generated code could be the same.

> Source/WebCore/page/ChromeClient.h:430
> +    virtual bool requiresAcceleratedCompositingForViewportConstrainedPosition() const { return false; }

Seems a inconsistent that for Settings we go with ”fixed position” but here we say “viewport constrained position”.

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2594
>      const Settings& settings = m_renderView.frameView().frame().settings();
> -    if (!settings.acceleratedCompositingForFixedPositionEnabled())
> +    Page* page = this->page();
> +    bool clientRequiresCompositing = page && page->chrome().client().requiresAcceleratedCompositingForViewportConstrainedPosition();
> +    if (!(settings.acceleratedCompositingForFixedPositionEnabled() || clientRequiresCompositing))
>          return false;

Sure would be nice if this shared code with StyleResolver.cpp. The code is identical, except for how we get to the Page.

> Source/WebKit/mac/WebCoreSupport/WebChromeClient.h:202
> +    virtual bool requiresAcceleratedCompositingForViewportConstrainedPosition() const override;

I suggest making this private rather than public. Same for most of the other virtual overrides. I don’t think we ever want to call these except polymorphically through the base class, so it’s kind of nice to make them private so we don’t do it by accident. A really minor thing.

> Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:1017
> +    if ([documentView isKindOfClass:[WebHTMLView class]])
> +        return documentView.layer;
> +    return false;

I think this would read more logically as an &&:

    return [documentView isKindOfClass:[WebHTMLView class]] && documentView.layer;

[Beth Dakin]

(In reply to comment #2)
> (From update of attachment 235153 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=235153&action=review
> 
> Looks like it will work.
> 
> > Source/WebCore/css/StyleResolver.cpp:1261
> >          || (style.position() == FixedPosition && e && e->document().page() && e->document().page()->settings().fixedPositionCreatesStackingContext())
> > +        || (style.position() == FixedPosition && e && e->document().page() && e->document().page()->chrome().client().requiresAcceleratedCompositingForViewportConstrainedPosition())
> 
> Could we add a helper function to make this code easier to read?
> 
>     || (style.position() == FixedPosition && shouldSomething(e))
> 
> Then the code in the helper function would say:
> 
>     if (!element)
>         return false;
>     Page* page = element->document().page();
>     if (!page)
>         return false;
>     return page->settings().fixedPositionCreatesStackingContext()
>         || page->chrome().client().requiresAcceleratedCompositingForViewportConstrainedPosition();
> 
> And we could even put some “why” comment in there. It can be an inline function if needed so the generated code could be the same.
> 

I added a helper function. It is much cleaner.

> > Source/WebCore/page/ChromeClient.h:430
> > +    virtual bool requiresAcceleratedCompositingForViewportConstrainedPosition() const { return false; }
> 
> Seems a inconsistent that for Settings we go with ”fixed position” but here we say “viewport constrained position”.
> 

I used "viewport constrained position" instead of "fixed position" because of the comment on line 2589 in RenderLayerCompositor that reads:

// FIXME: acceleratedCompositingForFixedPositionEnabled should probably be renamed acceleratedCompositingForViewportConstrainedPositionEnabled().

So "viewport constrained position" seems to be the way of the future, and maybe this patch should have introduced that re-name, but I decided to save that for a future clean-up.

> > Source/WebCore/rendering/RenderLayerCompositor.cpp:2594
> >      const Settings& settings = m_renderView.frameView().frame().settings();
> > -    if (!settings.acceleratedCompositingForFixedPositionEnabled())
> > +    Page* page = this->page();
> > +    bool clientRequiresCompositing = page && page->chrome().client().requiresAcceleratedCompositingForViewportConstrainedPosition();
> > +    if (!(settings.acceleratedCompositingForFixedPositionEnabled() || clientRequiresCompositing))
> >          return false;
> 
> Sure would be nice if this shared code with StyleResolver.cpp. The code is identical, except for how we get to the Page.
> 

The code is actually slightly different, in a rather confusing way. This code consults the Setting acceleratedCompositingForFixedPositionEnabled() and StyleResolver consults a different Setting: fixedPositionCreatesStackingContext(). I'm guessing we made them two different settings because we expected enabling these settings to cause regressions way back when we first enabled them, and having two different settings would make it easier to track down the problem? Maybe? But it's terribly confusing now because if you ever had acceleratedCompositingForFixedPositionEnabled() set but not fixedPositionCreatesStackingContext(), then that could definitely lead to broken content. We should clean this up.

In the meantime, I did not attempt to share this code. I think the cleanup should happen first.

> > Source/WebKit/mac/WebCoreSupport/WebChromeClient.h:202
> > +    virtual bool requiresAcceleratedCompositingForViewportConstrainedPosition() const override;
> 
> I suggest making this private rather than public. Same for most of the other virtual overrides. I don’t think we ever want to call these except polymorphically through the base class, so it’s kind of nice to make them private so we don’t do it by accident. A really minor thing.
> 

Done.

> > Source/WebKit/mac/WebCoreSupport/WebChromeClient.mm:1017
> > +    if ([documentView isKindOfClass:[WebHTMLView class]])
> > +        return documentView.layer;
> > +    return false;
> 
> I think this would read more logically as an &&:
> 
>     return [documentView isKindOfClass:[WebHTMLView class]] && documentView.layer;

Done.

Thanks Darin!

[Beth Dakin]

http://trac.webkit.org/changeset/171308