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
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.
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
enne, jamesr, do you know what behavior we (i.e. chromium) want here?
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.
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
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.
(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!
(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?
(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.
Created attachment 189389 [details] Patch Let's see how this one does with the bots.
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.
(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?
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().
(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?
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.
Created attachment 189439 [details] Patch Okay! Here is a patch that removes this line of code for all platforms given Alex's feedback.
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
Thanks, Simon! http://trac.webkit.org/changeset/143641