656036 - [meta] Attach position:fixed elements to the layout viewport (a.k.a. "fixed viewport"), not the visual viewport

Status: RESOLVED FIXED

(Core :: Layout, defect, P2)

Description

[Aakash Desai [:aakashd]]

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)]

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)]

don't see any problems with rottentomatoes.com (possibly I'm getting different version by default)

Comment 3

[Matt Brubeck (:mbrubeck)]

(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 (dead)]

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 (inactive)]

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)]

(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]

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 [:bzbarsky]]

position:fixed should in fact fix wrt CSS viewport.

Comment 9

[Boris Zbarsky [:bzbarsky]]

And in particular, consider position:fixed in a subframe...

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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]

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) (email my personal email if necessary)]

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]

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)]

Attachment: 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)]

Attachment: 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) (email my personal email if necessary)]

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)]

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) (email my personal email if necessary)]

That doesn't really help.

Comment 19

[Oleg Romashin (:romaxa)]

Attachment: 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) (email my personal email if necessary)]

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)]

(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)]

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]

That's been discussed, but users need to zoom in on position:fixed frames too.

Comment 24

[Matt Brubeck (:mbrubeck)]

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 26

[Kartikaya Gupta (email:[e-mail])]

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)]

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 29

[Tetsuharu OHZEKI [:tetsuharu] (UTC+9)]

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:[e-mail])]

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 [Ex-Mozilla]]

Renom this since it came up in some discussions. We are the odd rendering engine out in this behavior.

Comment 32

[James Willcox (:snorp) ([e-mail]) (he/him)]

Jet, is it possible to do something with this now that we're using APZ on Fennec?

Comment 33

[Jet Villegas (inactive)]

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:[e-mail])]

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.

Comment 36

[Botond Ballo [:botond]]

The consensus seems to be to attach position:fixed elements to the layout viewport (a.k.a. "fixed viewport" [1]) rather than the visual viewport. I believe all other browser engines do that now.

[1] https://github.com/bokand/bokand.github.io/blob/master/web_viewports_explainer.md#fixed-viewport-size

Comment 37

[Jonathan Watt [:jwatt]]

Mass bug change to replace various 'parity' whiteboard flags with the new canonical keywords. (See bug 1443764 comment 13.)

Comment 38

[Botond Ballo [:botond]]

Marking this as a meta bug, since I expect all the patches to land in dependent bugs.

Comment 41

[Karl Dubost💡 :karlcow]

The behavior difference here in between chrome and firefox is happening when the page is zoomed in.
https://webcompat.com/issues/15978

Comment 42

[Botond Ballo [:botond]]

This is fixed in Firefox >= 63 (the patches were in the dependent bugs, most notably bug 1423011 and bug 1465616).

I went through the webcompat issues linked to this bug. They seem to be resolved, with two exceptions:

https://webcompat.com/issues/17281
https://webcompat.com/issues/15978

for which I'll file follow-up bugs.

Comment 43

[Botond Ballo [:botond]]

Moved https://webcompat.com/issues/17281 over to bug 1508458.

Comment 44

[Botond Ballo [:botond]]

Moved https://webcompat.com/issues/15978 over to bug 1508459.