Last Comment Bug 656036 - position:fixed elements do not remain fixed to the CSS viewport when zooming with APZC
: position:fixed elements do not remain fixed to the CSS viewport when zooming ...
Status: NEW
[parity-ie][parity-blink]
: mobile, regression
Product: Core
Classification: Components
Component: Layout (show other bugs)
: Trunk
: All All
: P2 normal with 1 vote (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
http://news.google.com/news?edchanged...
: 962553 1156892 (view as bug list)
Depends on: 702656
Blocks: 1123938 607417 933556
  Show dependency treegraph
 
Reported: 2011-05-10 09:34 PDT by Aakash Desai [:aakashd]
Modified: 2016-03-31 11:30 PDT (History)
27 users (show)
bugs: needinfo? (mstange)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+


Attachments
test case (4.26 KB, text/html)
2011-10-25 09:57 PDT, Matt Brubeck (:mbrubeck)
no flags Details
Temporary scale accelerated position change fix (3.92 KB, patch)
2011-10-25 15:36 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Review
Remember viewConfig transform scale in order reverse it for fixed layers (3.07 KB, patch)
2011-10-25 19:20 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Review

Description Aakash Desai [:aakashd] 2011-05-10 09:34:04 PDT
Build Id:
Mozilla/5.0 (Android; Linux armv7l; rv:5.0a2) Gecko/20110510 Firefox/5.0a2 Fennec/5.0a2 ID:20110510055442

Steps to Reproduce:
1. Go to rottentomatoes.com or news.google.com (desktop versions)
2. Try to zoom into the pages

Actual Results:
On rottentomatoes.com, the position:fixed Ad does not change its viewport when zooming into that page.

On Google News, the position:fixed sidebar does not change its viewport when zooming into that page.

Expected Results:
The elements should zoom in naturally with the rest of the contents on the page.
Comment 1 Oleg Romashin (:romaxa) 2011-05-10 18:55:28 PDT
I've checked quickly news.google.com page, and found that zooming of fixed-positioned layer is actually correct, but problem is that page is allowed to be scrolled under fixed pos remote-Frame layer and by default we have fixed layer covering main page... if you scroll main content to the right, then you will be able to see whole page content.
Comment 2 Oleg Romashin (:romaxa) 2011-05-10 18:58:49 PDT
don't see any problems with rottentomatoes.com (possibly I'm getting different version by default)
Comment 3 Matt Brubeck (:mbrubeck) 2011-05-10 19:39:50 PDT
(In reply to comment #1)
> I've checked quickly news.google.com page, and found that zooming of
> fixed-positioned layer is actually correct, but problem is that page is
> allowed to be scrolled under fixed pos remote-Frame layer and by default we
> have fixed layer covering main page... if you scroll main content to the
> right, then you will be able to see whole page content.

The desktop version news.google.com is a complicated case because it sets position:fixed only if the page is scrolled down a certain amount *and* is scrolled all the way to the left.  Because of the way Fennec's viewport works, we might not send "scroll" events every time the user pans, so the page might not disable position:fixed at the intended time.

If I understand right, the problems stem from the fact that position:fixed elements in Fennec are fixed to the screen, while they should be fixed to the CSS viewport.  Ideally, panning in Fennec would pan around within the CSS viewport, and scroll the CSS viewport only when you hit one of its edges.  (There are some complications, like when you zoom out far enough that the CSS viewport is smaller than the screen.)
Comment 4 Martijn Wargers [:mwargers] (not working for Mozilla) 2011-05-16 09:50:01 PDT
This is fixed with the fix for bug 656167, but we'll keep this open for now, because that bug just is a temporary fix.
Comment 5 Jet Villegas (:jet) 2011-10-19 16:15:31 PDT
Need a product decision on this bug vs. 607417. It seems that 656036 is a lesser issue compared to 607417 where all fixed-pos layout is busted.
Comment 6 Matt Brubeck (:mbrubeck) 2011-10-24 21:53:18 PDT
(In reply to Jet Villegas from comment #5)
> Need a product decision on this bug vs. 607417. It seems that 656036 is a
> lesser issue compared to 607417 where all fixed-pos layout is busted.

I disagree; see bug 607417 comment 113 for details.

Ideally, of course, we should land bug 607417 *and* fix this bug.  Oleg, Timothy, Robert: Can anyone comment on how much effort this would take?  Does the solution described in comment 3 make sense, and is it feasible?
Comment 7 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-24 22:03:21 PDT
It would be relatively easy to specify that a layer subtree should be positioned relative to an arbitrary ancestor container, instead of only either to the parent or the screen (top-level).  So the backend work for your proposal is implementable.

As for the proposal itself, it sounds reasonable to me, and is closer to the CSS spec AIUI.  CC'ing dbaron and bz who can give a much more informed opinion than I can.
Comment 8 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-24 22:15:34 PDT
position:fixed should in fact fix wrt CSS viewport.
Comment 9 Boris Zbarsky [:bz] (Out June 25-July 6) 2011-10-24 22:16:00 PDT
And in particular, consider position:fixed in a subframe...
Comment 10 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-10-24 22:18:45 PDT
It sounds reasonable to me, yes. But why would layers need to be changed to implement it? Seems like only the code that manipulates the ShadowLayers in response to panning would have to change.
Comment 11 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-24 22:31:22 PDT
It's just a matter of whether we want the frontend(s) to implement position:fixed behavior or we want Gecko to do it automatically.  (I don't remember what the current code does.)  At the moment, it's looking like a better long-term bet to implement features like this in Gecko, since we have under development multiple frontends that need to use this feature.  But having the frontends implement this is an option too.
Comment 12 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-10-25 02:25:53 PDT
OK, but I think the layer tree layout currently builds is OK. It should be putting the layers for fixed-pos elements in the right place in the CSS viewport --- the displayport doesn't affect layout and I don't think the actual screen size does either. So whatever's happening is caused by the front-end scrolling and panning code.
Comment 13 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-25 02:31:40 PDT
That makes sense.  Given comment 3 then, it sounds like fixed layers might be stuck in the wrong place in the layer tree or being enabled "scrollable" and frobbed when they shouldn't be "scrollable" wrt frontend.
Comment 14 Matt Brubeck (:mbrubeck) 2011-10-25 09:57:18 PDT
Created attachment 569410 [details]
test case

This test case illustrates the problem.  The fixed elements (blue boxes in the corners) should not obscure the content as you change the window size or zoom level (except at extremely small window sizes, less than 150px).

In the stock Android 3.2 (Honeycomb) browser, they do obscure the content when you zoom in, because they are fixed to the screen rather than the CSS viewport -- like in Fennec with bug 607417.  I consider this a bug.  (The fact that Android ships with this bug might be evidence that it is not a huge problem in practice, though they also only ship with it on tablets where the problem is not as noticeable.)

In Fennec with bug 607417 applied, only the top left box is visible.  The other boxes are hidden (possibly a different bug).  When you zoom in, it is not fixed to the correct position while zooming, but it jumps back into position when zooming stops.  If you scroll while zoomed in, it obscures the content.
Comment 15 Oleg Romashin (:romaxa) 2011-10-25 15:36:03 PDT
Created attachment 569525 [details] [diff] [review]
Temporary scale accelerated position change fix

Ok, here is one important change which is fixing layers position during temporary scaling.
Comment 16 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-10-25 16:39:16 PDT
Comment on attachment 569525 [details] [diff] [review]
Temporary scale accelerated position change fix

Review of attachment 569525 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/ipc/RenderFrameParent.cpp
@@ +109,3 @@
>  {
> +  aTransform._41 -= aViewTransform.mTranslation.x / aXScale;
> +  aTransform._42 -= aViewTransform.mTranslation.y / aYScale;

This doesn't seem correct to me. Can you explain it?

@@ +267,5 @@
>  static void
>  TransformShadowTree(nsDisplayListBuilder* aBuilder, nsFrameLoader* aFrameLoader,
>                      nsIFrame* aFrame, Layer* aLayer,
> +                    const ViewTransform& aTransform,
> +                    float scaleX = 1.0, float scaleY = 1.0)

aScaleX, aScaleY
Comment 17 Oleg Romashin (:romaxa) 2011-10-25 16:54:55 PDT
Problem with invisible right boxes, seems related to wrong nsDisplayList.cpp GetDisplayPortBounds calculation, function call nsLayoutUtils::TransformRectToBoundsInAncestor.

If I simply inflate rect returned by that function, then right boxes become visible
Comment 18 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-10-25 18:00:23 PDT
That doesn't really help.
Comment 19 Oleg Romashin (:romaxa) 2011-10-25 19:20:11 PDT
Created attachment 569587 [details] [diff] [review]
Remember viewConfig transform scale in order reverse it for fixed layers

Ok, reverse scale was originally stored into view transform., and used in order to reverse translation for fixed layers.
After some changes in layout resolution system this part was messed up.

Current implementation calculating viewTransform and moving it's translation part into layerTransform and scaling applying from container.... here I see some problem.

Also in post layer resolution change fix (https://bugzilla.mozilla.org/attachment.cgi?id=549222), I've changed reverseTranslate function... and that was seems not very good idea..

So now, we should do next:
ComputeShadowTreeTransform into layerTransform, and just pass it down in order to use that for fixed layers translation reversing.

Also restore back division in ReverseTranslation function.

probably we can remember 1/scale under metrics->IsScrollable() condition, so we divide once for container, and multiply for all fixed pos layers..
Comment 20 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2011-10-25 20:58:24 PDT
This patch effectively ignores aTransform for scrolled layers. That doesn't seem right either.

This code is far too confusing :-(.

One confusing thing is that ViewTransform applies its scale to its translation when converting to a matrix. Normal transform matrices don't do that :-(.

But given we do that, surely
    nsIntPoint scrollCompensation(
      scrollOffset.x * aInverseScaleX - metricsScrollOffset.x * aConfig.mXScale,
      scrollOffset.y * aInverseScaleY - metricsScrollOffset.y * aConfig.mYScale);
    return ViewTransform(-scrollCompensation, aConfig.mXScale, aConfig.mYScale);
is wrong, since we end up multiplying by aConfig.mXScale twice?

I think RenderFrameParent mostly needs to be rewritten or we're just going to be thrashing around.
Comment 21 Tatiana Meshkova (:tatiana) 2011-11-09 15:51:36 PST
(In reply to Oleg Romashin (:romaxa) from comment #17)
> Problem with invisible right boxes, seems related to wrong nsDisplayList.cpp
> GetDisplayPortBounds calculation, function call
> nsLayoutUtils::TransformRectToBoundsInAncestor.

TransformRectToBoundsInAncestor() uses nsIFrame::GetTransformMatrix(), that includes frame offset translation. This translation moves display port far away from fixed item bounds. Actually we need to apply CSS transforms only.
filed bug 701190

> 
> If I simply inflate rect returned by that function, then right boxes become
> visible
Comment 22 Oleg Romashin (:romaxa) 2011-11-11 09:10:46 PST
about zooming... I'm wondering should not we disable scaling for fixed layers? so in that case those elements would stay always in visible part of the screen, with original size, woud be accessible all the time and don't cover more screen?
Comment 23 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-11-11 16:04:22 PST
That's been discussed, but users need to zoom in on position:fixed frames too.
Comment 24 Matt Brubeck (:mbrubeck) 2013-11-12 12:15:58 PST
For reference, IE10 and 11 implement the solution I described in bug 607417 comment 5.  Other mobile browsers behave ore or less like Firefox today.  IE is the only one that works well on "desktop" sites with position:fixed sidebars.
Comment 25 Matt Brubeck (:mbrubeck) 2014-01-22 10:39:33 PST
*** Bug 962553 has been marked as a duplicate of this bug. ***
Comment 26 Kartikaya Gupta (email:kats@mozilla.com) 2014-08-09 07:04:28 PDT
I'm not sure what the state of the bug here is. I think it should probably be resolved as wontfix or incomplete. If we do plan to implement what Matt is suggesting then it would be a layout-side change, as the fixed-position layers' anchor point would probably have to change.
Comment 27 Matt Brubeck (:mbrubeck) 2014-08-11 21:45:13 PDT
Blink is also working on fixing this bug and aligning with IE11's behavior.  This change is implemented in Chrome 36 but disabled by default.  (It can be enabled with the chrome://flags/#enable-pinch-virtual-viewport flag.)
Comment 28 Kartikaya Gupta (email:kats@mozilla.com) 2015-04-21 11:51:47 PDT
*** Bug 1156892 has been marked as a duplicate of this bug. ***
Comment 29 Tetsuharu OHZEKI [:tetsuharu] [UTC+9] 2015-08-09 11:44:02 PDT
What is the current status of this?

cf. This change is implemented in Chrome 40: https://developers.google.com/web/updates/2015/01/virtual-viewport
Comment 30 Kartikaya Gupta (email:kats@mozilla.com) 2015-08-10 08:14:00 PDT
As far as I know nobody is actively working on this in Gecko. FWIW I tried out the behaviour in a recent Chrome build (perhaps not the very latest) and some parts of it seemed a little counterintuitive, but I haven't looked at it too closely.

There's some details on the Blink implementation at https://docs.google.com/document/d/1WZBy5JnwgHNQKbqsGMpgCEsiS2t5VaHFZ47xfX8VYiU/edit#heading=h.vsbcemkhokce
Comment 31 Kevin Brosnan [:kbrosnan] 2016-03-25 16:01:20 PDT
Renom this since it came up in some discussions. We are the odd rendering engine out in this behavior.
Comment 32 James Willcox (:snorp) (jwillcox@mozilla.com) 2016-03-31 10:04:23 PDT
Jet, is it possible to do something with this now that we're using APZ on Fennec?
Comment 33 Jet Villegas (:jet) 2016-03-31 10:31:36 PDT
Markus is working on CSS background-position-x / -y for Q2. Let's ask him if he can look into this one while he's in the neighborhood.
Comment 34 Kartikaya Gupta (email:kats@mozilla.com) 2016-03-31 11:30:29 PDT
This is probably a good topic for discussion with the Blink folks that we might be meeting with in a couple of weeks in Toronto.

Note You need to log in before you can comment on or make changes to this bug.