Bug 135075 - Put position:fixed elements into layers when a WK1 view is layer-backed
Summary: Put position:fixed elements into layers when a WK1 view is layer-backed
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2014-07-18 15:38 PDT by Beth Dakin
Modified: 2014-07-21 11:32 PDT (History)
4 users (show)

See Also:


Attachments
Patch (6.41 KB, patch)
2014-07-18 15:47 PDT, Beth Dakin
darin: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Beth Dakin 2014-07-18 15:38:43 PDT
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>
Comment 1 Beth Dakin 2014-07-18 15:47:30 PDT
Created attachment 235153 [details]
Patch
Comment 2 Darin Adler 2014-07-19 22:47:44 PDT
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;
Comment 3 Beth Dakin 2014-07-20 13:50:59 PDT
(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!
Comment 4 Beth Dakin 2014-07-21 11:32:53 PDT
http://trac.webkit.org/changeset/171308