Bug 110294 – Out-of-view fixed position check should not be affected by page scale at all on Mac

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Beth Dakin]

https://bugs.webkit.org/show_bug.cgi?id=104303 and then https://bugs.webkit.org/show_bug.cgi?id=104943 introduced behavior where the out-of-view fixed position logic scales the layer bounds by the page scale factor before comparing it to the visible content rect (which is NOT scaled by the page scale). This is definitely wrong, at least on the Mac. It seems like it is very much intended behavior on Chromium based on comments in the layout tests that were added by that change.

These changes seem to assume that a fixed element that was offscreen might come onscreen when a user scales a page in or out…or that an onscreen element might go offscreen after a page scale in a way that is not accessible by scrolling. This is absolutely not the case on Mac. On Mac, the aim of page scaling is to totally preserve the page as it was…just to make it bigger or smaller. So fixed elements that were onscreen will still be onscreen, and elements that were off will still be off.

I am going to upload a patch that fixes this on the Mac. For the time being, I will assume that the Chromium behavior was intentional…but I do have to say that is strikes me as quite odd; so it would be nice to have some Chromium people confirm that that is indeed their desired behavior. 

<rdar://problem/13247582> Fixed elements disappear on zooming

[Beth Dakin]

Created attachment 189228 [details]
Patch

I especially don't know if settings()->applyPageScaleFactorInCompositor() is the right way to opt into this code for Chromium, so feedback would be great.

[WebKit Review Bot]

Comment on attachment 189228 [details]
Patch

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

New failing tests:
platform/chromium/virtual/softwarecompositing/layer-creation/fixed-position-out-of-view-scaled-scroll.html
platform/chromium/virtual/softwarecompositing/layer-creation/fixed-position-out-of-view-scaled.html
compositing/layer-creation/fixed-position-out-of-view-scaled-scroll.html
compositing/layer-creation/fixed-position-out-of-view-scaled.html

[Ojan Vafai]

enne, jamesr, do you know what behavior we (i.e. chromium) want here?

[James Robinson]

This is page scale, not page zoom - right?  We don't currently support changing the page scale on any desktop browsers.  I don't think it's yet entirely clear what the expected behavior would be if/when we do support that.  For now, we should preserve the existing behavior.

For mobile type browsers, I think it's expected for page scale changes (aka pinch gestures) to cause fixpos elements to enter or leave the viewport since these use fixed viewport style models.

[Build Bot]

Comment on attachment 189228 [details]
Patch

Attachment 189228 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://queues.webkit.org/results/16656058

New failing tests:
platform/mac-wk2/tiled-drawing/fixed/four-bars-zoomed.html

[Ryosuke Niwa]

Comment on attachment 189228 [details]
Patch

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

> Source/WebCore/ChangeLog:4
> +        Out-of-view fixed position check should not be affected by page scale at all on 
> +        Mac

It seems like we can fit this all in one line?

> Source/WebCore/ChangeLog:14
> +        * rendering/RenderLayerCompositor.cpp:

Blank line here?

> LayoutTests/ChangeLog:4
> +        Out-of-view fixed position check should not be affected by page scale at all on 
> +        Mac

Ditto.

[Beth Dakin]

(In reply to comment #4)
> This is page scale, not page zoom - right?  We don't currently support changing the page scale on any desktop browsers.  I don't think it's yet entirely clear what the expected behavior would be if/when we do support that.  For now, we should preserve the existing behavior.
> 
> For mobile type browsers, I think it's expected for page scale changes (aka pinch gestures) to cause fixpos elements to enter or leave the viewport since these use fixed viewport style models.

Yes, we are indeed talking about scale rather than zoom. Good to know that Chromium does not support this on the Desktop. Mac does, and of course iOS does too. As I said, Mac and iOS do not allow fixedpos elements to enter or leave on scaling. But thank you for clarifying that Chromium differs here!

[Beth Dakin]

(In reply to comment #2)
> (From update of attachment 189228 [details])
> Attachment 189228 [details] did not pass chromium-ews (chromium-xvfb):
> Output: http://queues.webkit.org/results/16648054
> 
> New failing tests:
> platform/chromium/virtual/softwarecompositing/layer-creation/fixed-position-out-of-view-scaled-scroll.html
> platform/chromium/virtual/softwarecompositing/layer-creation/fixed-position-out-of-view-scaled.html
> compositing/layer-creation/fixed-position-out-of-view-scaled-scroll.html
> compositing/layer-creation/fixed-position-out-of-view-scaled.html

I am trying to retain existing behavior on Chromium with my patch, so I guess my guess that applyPageScaleFactorInCompositor() would be a good way to do that was false based on these failing tests. Maybe a regular ifdef is in order here?

[Beth Dakin]

(In reply to comment #6)
> (From update of attachment 189228 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189228&action=review
> 
> > Source/WebCore/ChangeLog:4
> > +        Out-of-view fixed position check should not be affected by page scale at all on 
> > +        Mac
> 
> It seems like we can fit this all in one line?
> 
> > Source/WebCore/ChangeLog:14
> > +        * rendering/RenderLayerCompositor.cpp:
> 
> Blank line here?
> 
> > LayoutTests/ChangeLog:4
> > +        Out-of-view fixed position check should not be affected by page scale at all on 
> > +        Mac
> 
> Ditto.

Thanks, Ryosuke! Will fix.

[Beth Dakin]

Created attachment 189389 [details]
Patch

Let's see how this one does with the bots.

[Simon Fraser (smfr)]

Comment on attachment 189389 [details]
Patch

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

> Source/WebCore/rendering/RenderLayerCompositor.cpp:2107
> +#if PLATFORM(CHROMIUM)
>          layerBounds.scale(frameView->frame()->frameScaleFactor());
> +#endif

I'm not a fan of this #ifdef. If there's a real behavior change between platforms, it should either be a runtime-testable thing, or some meaningful #ifdef.

[James Robinson]

(In reply to comment #8)
> (In reply to comment #2)
> > (From update of attachment 189228 [details] [details])
> > Attachment 189228 [details] [details] did not pass chromium-ews (chromium-xvfb):
> > Output: http://queues.webkit.org/results/16648054
> > 
> > New failing tests:
> > platform/chromium/virtual/softwarecompositing/layer-creation/fixed-position-out-of-view-scaled-scroll.html
> > platform/chromium/virtual/softwarecompositing/layer-creation/fixed-position-out-of-view-scaled.html
> > compositing/layer-creation/fixed-position-out-of-view-scaled-scroll.html
> > compositing/layer-creation/fixed-position-out-of-view-scaled.html
> 
> I am trying to retain existing behavior on Chromium with my patch, so I guess my guess that applyPageScaleFactorInCompositor() would be a good way to do that was false based on these failing tests. Maybe a regular ifdef is in order here?

Alex, any idea what's going on here?

[James Robinson]

I'm not sure there's a platform policy difference here so much as a different in implementation strategies.  I definitely would prefer to figure out exactly where that difference is and add a check for the proper runtime guard rather than add an #if PLATFORM().

[Beth Dakin]

(In reply to comment #13)
> I'm not sure there's a platform policy difference here so much as a different in implementation strategies.  I definitely would prefer to figure out exactly where that difference is and add a check for the proper runtime guard rather than add an #if PLATFORM().

I'm not sure what you mean when you say, " I definitely would prefer to figure out exactly where that difference is…" because I think that I do understand what the difference is. Do you think there might be more the discover here? Or is this just a way of saying that we should take that knowledge and turn it into a good name for a runtime switch? 

And in case it was unclear from my previous comments, I understand the "difference" that I refer to above as whether fixed objects position themselves differently depending on the scale factor (what Chromium is opting for), or whether they are always in the same place relative to the viewport* (Mac/iOS). 

*Although maybe defining viewport is up for debate here too?

[Alexandre Elias]

Sorry for the confusion.  Taking a close look at it, we can delete this line for Chromium as well.  It's no longer needed because of our transition to pageScaleFactorAppliedInCompositor mode, introduced a few months after that line was added.  In the past our layers were in a different space from the viewport, but that difference should've gone away with the recent changes.

[Beth Dakin]

Created attachment 189439 [details]
Patch

Okay! Here is a patch that removes this line of code for all platforms given Alex's feedback.

[WebKit Review Bot]

Comment on attachment 189439 [details]
Patch

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

New failing tests:
platform/chromium/virtual/softwarecompositing/layer-creation/fixed-position-out-of-view-scaled-scroll.html
platform/chromium/virtual/softwarecompositing/layer-creation/fixed-position-out-of-view-scaled.html
compositing/layer-creation/fixed-position-out-of-view-scaled-scroll.html
compositing/layer-creation/fixed-position-out-of-view-scaled.html

[Beth Dakin]

Thanks, Simon! http://trac.webkit.org/changeset/143641