The default bug view has changed. See this FAQ.

Reconcile position:fixed with async scrolling and displayport+meta-viewport

RESOLVED FIXED in mozilla15

Status

()

Core
Layout
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: cjones, Assigned: cwiiis)

Tracking

(Depends on: 1 bug, Blocks: 6 bugs, {mobile})

unspecified
mozilla15
mobile
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(blocking-fennec1.0 -, fennec15+)

Details

(Whiteboard: [fennec-4.1?][layout][gfx.relnote.15])

Attachments

(16 attachments, 34 obsolete attachments)

1.44 KB, text/html
Details
1.30 KB, text/html
Details
416 bytes, text/html
Details
25.88 KB, image/png
Details
1.64 KB, patch
Details | Diff | Splinter Review
6.03 KB, text/plain
Details
5.24 KB, patch
stechz
: review+
Details | Diff | Splinter Review
10.56 KB, patch
roc
: review+
Details | Diff | Splinter Review
18.01 KB, patch
Details | Diff | Splinter Review
2.73 KB, patch
roc
: review+
cjones
: review+
Details | Diff | Splinter Review
3.72 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.08 KB, text/plain
Details
11.10 KB, patch
roc
: review+
Details | Diff | Splinter Review
2.68 KB, patch
ajuma
: review+
Details | Diff | Splinter Review
6.78 KB, patch
ajuma
: review+
Details | Diff | Splinter Review
1.93 KB, patch
ajuma
: review+
Details | Diff | Splinter Review
We should decide how position:fixed should be interpreted and then implement it.  Layers stamped with frame metrics provides platform infrastructure to implement pretty much anything we want.

Currently fennec basically ignores position:fixed elements.

Desiderata
 - position:fixed elements stay fixed in chrome window while chrome pans independently of content
 - except on zoomed-in pages, where position:fixed elements (perhaps) shouldn't occupy "too much" of the chrome window as compared to the page un-zoomed

Questions
 - how are position:fixed elements to be interpreted in the chrome process while chrome is pre-scaling shadow layers before sending a re-render request to content?
 - what to do with position:fixed elements that are within the CSS viewport but outside the screen's visible rect

stechz proposed honoring position:fixed for sites that use <meta viewport> to prevent zooming.  This is a good plan, but I'm hesitant about this because it would force sites to use <meta viewport> to turn on core CSS features.
I think this should block final.
tracking-fennec: --- → ?

Updated

7 years ago
tracking-fennec: ? → 2.0+

Comment 2

7 years ago
CC'ing myself.
The way I see it:
* position:fixed should behave like position:absolute for zoomable pages
* position:fixed works as advertised with for not zoomable pages

This approach allows for web pages to do toolbars, but doesn't have any complicated edge cases for desktop sites. I think this is similar to the way Android's mobile browser does it in 2.2.
Created attachment 486294 [details]
simple testcase

this testcase show that we don't change CSS viewport position at all in remote fennec. Footer scrolls up and never stick to bottom of screen
(In reply to comment #3)
> * position:fixed should behave like position:absolute for zoomable pages

I think we can do better than this for zoomable pages.  I propose that we scroll the CSS viewport as needed to keep the visible region inside the CSS viewport.  So a position:fixed footer might not be on screen if you are zoomed in to a region smaller than the viewport.  But if you pan toward the bottom of the page, then when you reach the footer it would stick to the bottom of the screen while you panned down further.

This might be harder to implement, and maybe we should go with stechz's plan as a first step.  But I think this is worth doing because many sites with fixed footers are rather broken in Fennec, as the footer ends up covering up text on the page.  For example:  http://news.cnet.com/8301-30685_3-20019002-264.html
> I think we can do better than this for zoomable pages.  I propose that we
> scroll the CSS viewport as needed to keep the visible region inside the CSS
> viewport.  So a position:fixed footer might not be on screen if you are zoomed
> in to a region smaller than the viewport.  But if you pan toward the bottom of
> the page, then when you reach the footer it would stick to the bottom of the
> screen while you panned down further.

I'm worried about being this clever.
1) What about zooming out? In this case, bars at the bottom or top or sides are just going to look ugly and unintentional.
2) I worry about how this will feel. When I zoom in, I guess I will first always pan the visible region before panning the CSS viewport, so it will kind of act like an iframe if the elements are at the margins of the visible area. In the other case, it will appear that these position:fixed elements sometimes scroll and other times don't.

> This might be harder to implement, and maybe we should go with stechz's plan as
> a first step.  But I think this is worth doing because many sites with fixed
> footers are rather broken in Fennec, as the footer ends up covering up text on
> the page.  For example:  http://news.cnet.com/8301-30685_3-20019002-264.html

We could also do them as not displayable, or hide them when the zoom level is not 1. Maybe that would be frustrating though, if you were trying to zoom into the fixed element. :)

Comment 7

7 years ago
Oleg has suggested in another bug moving the fixed elements into their own layer. Would this be the first step to do or is it even the right direction? Could someone give pointers to code in where this would be done please?
(In reply to comment #7)
> Oleg has suggested in another bug moving the fixed elements into their own
> layer. Would this be the first step to do or is it even the right direction?
> Could someone give pointers to code in where this would be done please?

I'm new to the layout code, but here's my roadmap. My first suggestion is to go to nsLayoutUtils and turn this on and go to a page with position:fixed:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#1074

You will need to find which frame and display item represents the position: fixed element. The frame code will have a neat function called BuildDisplayList:
http://mxr.mozilla.org/mozilla-central/search?string=BuildDisplayList

A display list is responsible for figuring out what to paint and hit testing (e.g., what element did a user click on?). It is also the code that determines whether the frame it is representing (and everything under it) should be in its own layer or not. nsDisplayOwnLayer does what it says on the tin. You should be able to find where a position: fixed display item is created and just make sure it is always an nsDisplayOwnLayer.

I think e10s should take care of everything else. If it doesn't seem to be working, pass the environment var NSPR_LOG_MODULES=Layers:5 and inspect the layer tree.
(In reply to comment #8)
> A display list is responsible for figuring out what to paint and hit testing
> (e.g., what element did a user click on?). It is also the code that determines
> whether the frame it is representing (and everything under it) should be in its
> own layer or not. nsDisplayOwnLayer does what it says on the tin. You should be
> able to find where a position: fixed display item is created and just make sure
> it is always an nsDisplayOwnLayer.

That'll work, but it may be overkill.

On desktop, fixed-pos elements are already placed on separate layer(s) to the scrolled content. FrameLayerBuilder defines an "active scrolled root" for each display item --- the nearest ancestor frame which, when scrolled, will cause that display item to move. Two display items can only be placed in the same ThebesLayer if they have the same active scrolled root. Your best approach may be to extend the active scrolled root concept for your needs:
http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#696
Scrolling in the viewport of the toplevel content document is always considered "active" (even on mobile where you don't scroll the toplevel document --- layout doesn't take advantage of that currently). Therefore fixed-pos elements in the toplevel viewport should already be in their own layer(s). Fixed-pos children of IFRAMEs might not be, because scrolling in the viewport of content subdocuments is not always considered active, but I guess you don't want to change their behaviour anyway.

The only change you might need in layers/layout is if you want some fixed-pos elements to move independently of other fixed-pos elements; in that case you might want to ensure they're in different layers.
Then it's probably just a matter of making sure that the offset for the fixed position layer doesn't change in the parent process.
Created attachment 491800 [details] [diff] [review]
API part
Attachment #491800 - Flags: review?(roc)
Comment on attachment 491800 [details] [diff] [review]
API part

I think feedback is better here
Attachment #491800 - Flags: review?(roc) → feedback?(roc)
hmmm ok, this seems works only for thebes layer, but we should also setup fixed flag for ImageLayer, Color layers e.t.c which are related to fixed positioned frames.
Created attachment 491821 [details] [diff] [review]
Mark all available layers as fixed when it's parent viewport
Attachment #491800 - Attachment is obsolete: true
Attachment #491821 - Flags: feedback?(roc)
Attachment #491800 - Flags: feedback?(roc)
Created attachment 491891 [details] [diff] [review]
Hackish handling Fixed layer

With hacky transform-translate change I got Thebeslayer staying on the same position, similar change needed for ShadowImage/Canvas/Color layer..
It looks really hacky, and I  would like to find better way to change offset for such layers. something like track translate for main content ShadowThebesLayer, check that offset, calc properly offset caused by UI layers (panels and bars) et.c.

And I  really need some advice what is the better way to do that.

PS: with this hack I  got fixed frames scrolling with mega fast speed (2 textures composite on GPU and it is really fast on Maemo device)
Attachment #491891 - Flags: feedback?(roc)
Created attachment 492206 [details] [diff] [review]
Mark all available layers as fixed when it's parent viewport v2 (colorLayer)
Attachment #491821 - Attachment is obsolete: true
Attachment #492206 - Flags: feedback?(roc)
Attachment #491821 - Flags: feedback?(roc)
Comment on attachment 491891 [details] [diff] [review]
Hackish handling Fixed layer

I'm not exactly sure what you want to do here, but whatever you do should be using the shadow layer "effective transforms" machinery, I think.
Attachment #491891 - Flags: feedback?(roc) → feedback-
Comment on attachment 492206 [details] [diff] [review]
Mark all available layers as fixed when it's parent viewport v2 (colorLayer)

What exactly does "IsFixed" mean here?

I'm guessing it should mean "fixed-position relative to the viewport of the layer tree".

But I think the infrastructure in bug 605618 will be relevant here. Let's wait for that to be settled.
> What exactly does "IsFixed" mean here?
> 
> I'm guessing it should mean "fixed-position relative to the viewport of the
> layer tree".
yes

> But I think the infrastructure in bug 605618 will be relevant here. Let's wait
> for that to be settled.
ok, I see
Blocks: 616348
Assignee: nobody → romaxa
Attachment #492206 - Flags: feedback?(roc)
bug 605618 landed a while ago, any update here?
Blocks: 628648
Duplicate of this bug: 628648
Created attachment 514718 [details] [diff] [review]
Part1: Fixed layers detection part, updated to tip
Attachment #491891 - Attachment is obsolete: true
Attachment #492206 - Attachment is obsolete: true
I found two ways of making another part working somehow:
1) Disable transform apply in ContainerLayer::ComputeEffectiveTransformsForChildren, if layer if Fixed  and shadow... but fixed layer has wrong position in that case (middle of the screen)
2) Disable translate transform in BasicLayerManager::PaintLayer when layer if Fixed  and shadow..., but that is need to be done for Basic and all other layer managers, also I need to -translate clip region...

roc: any suggestion in which direction I should proceed investigation?
Created attachment 514967 [details] [diff] [review]
Quick fixed layers suspender

This almost works with attached testcase. Idea is to do reverse translate transform for fixed layer to make opposite translation comparing to viewport scroll
1) With long scrolling fixed layer clipped out (don't know, sounds like some global clipping, possibly global remote viewport clipping)
2) by some reason I need to divide parent tranform translate by 2, have not found why it is needed.
3) Some rounding errors happen, and while scrolling fixed layer changing it's position a bit.
Attachment #514967 - Flags: feedback?
Feels like trick with this transformation is wrong... fixed element still moving on yahoo.com/sign in...
Created attachment 515350 [details] [diff] [review]
Reverse translate in RenderFrameParent, make it mostly work

Ok, this version mostly works fine. Scale factor was missing
The only missing some strange fixed layer disappear effect if we scrolling for too long..
Attachment #514967 - Attachment is obsolete: true
Attachment #514967 - Flags: feedback?
Fixed thebes layer seems clipped by something else... don't yet understand what is going on.
Easy visible on:
http://meyerweb.com/eric/css/edge/complexspiral/demo.html
Love to have, not blocking.  Renom if you disagree.
tracking-fennec: 2.0+ → 2.0-
Duplicate of this bug: 637967

Updated

6 years ago
Duplicate of this bug: 500732
Whiteboard: [fennec-4.1?]
Created attachment 520385 [details] [diff] [review]
DisplayList tricks WIP

Tatiana has created some hacks in DisplayList... IIUC this is about keeping fixed frames in displayList when it is going out from displayViewPort.
O have tested this patch, and it works fine with attached testcase, also http://news.cnet.com/8301-30685_3-20019002-264.html

still some problems with http://meyerweb.com/eric/css/edge/complexspiral/demo.html
also I've found that on dp.ru heater is not keeping it's position.
Also if attached testcase change to be heater, then awesomebar covering heater
Created attachment 520386 [details]
Heater simple testcase
Created attachment 520479 [details]
dp.ru - simple testcase
Created attachment 520480 [details]
More simple testcase dp.ru

sounds like fixed layer is not create or not marked when we have just position:fixed div without any "text" content
Attachment #520479 - Attachment is obsolete: true
Created attachment 520903 [details] [diff] [review]
DisplayList tricks

also works for http://meyerweb.com/eric/css/edge/complexspiral/demo.html now
Attachment #520385 - Attachment is obsolete: true
Created attachment 520929 [details] [diff] [review]
Part3: DisplayList tricks
Attachment #520903 - Attachment is obsolete: true
Comment on attachment 520929 [details] [diff] [review]
Part3: DisplayList tricks

Yep, tested this, and it works great.
roc do you have any feedback on these tricks? do you see any better way to implement the same?
Attachment #520929 - Flags: feedback?(roc)
What exactly are you trying to do here? Why isn't normal display list construction and visibility computation working?
I tried to explain these tricks in 607417#c32,
First attached patch is marking shadow fixed position layers
Second patch does reverse-translation for shadowLayers which are marked as fixed.
Third patch (DisplayList tricks), seems fixing visibility for fixed frames in order to prevent their disappearing (clipping) when Shadow display port moving out of the screen

But I think Tatiana can explain better third patch.
Yeah, I meant the third patch.
(In reply to comment #40)
> What exactly are you trying to do here? Why isn't normal display list
> construction and visibility computation working?

when using display port for scrolling and:

1) Building display list: fixed position items are wrongly excluded from display list by display port rectangle, as result footer's layer disappear on long scroll (see attachment #486294 [details] testcase):

Painting --- before optimization (dirty 0,-61050,48000,69863):
CanvasBackground 0xb33deb80(Canvas(html)(-1)) (0,-101700,48000,117240)(0,0,0,0) opaque uniform
Text 0xb213ca90(Text(70)) (480,-61320,2820,1140)(0,0,0,0)
Text 0xb213cb48(Text(74)) (480,-59040,2820,1140)(0,0,0,0)
Text 0xb213cc00(Text(78)) (480,-56760,2820,1140)(0,0,0,0)
Text 0xb213ccb8(Text(82)) (480,-54480,2820,1140)(0,0,0,0)
Text 0xb213cd70(Text(86)) (480,-52200,2820,1140)(0,0,0,0)
Text 0xb213ce28(Text(90)) (480,-49920,2820,1140)(0,0,0,0)
Text 0xb213cee0(Text(94)) (480,-47640,2820,1140)(0,0,0,0)
Text 0xb213cf98(Text(98)) (480,-45360,2820,1140)(0,0,0,0)
Text 0xb213e080(Text(102)) (480,-43080,2820,1140)(0,0,0,0)
Text 0xb213e138(Text(106)) (480,-40800,2820,1140)(0,0,0,0)
Text 0xb213e1f0(Text(110)) (480,-38520,2820,1140)(0,0,0,0)
Text 0xb213e2a8(Text(114)) (480,-36240,2820,1140)(0,0,0,0)
Text 0xb213e360(Text(118)) (480,-33960,2820,1140)(0,0,0,0)
Text 0xb213e418(Text(122)) (480,-31680,2820,1140)(0,0,0,0)
Text 0xb213e4d0(Text(126)) (480,-29400,2820,1140)(0,0,0,0)
Text 0xb213e588(Text(130)) (480,-27120,2820,1140)(0,0,0,0)
Text 0xb213e640(Text(134)) (480,-24840,2820,1140)(0,0,0,0)
Text 0xb213e6f8(Text(138)) (480,-22560,2820,1140)(0,0,0,0)
Text 0xb213e7b0(Text(142)) (480,-20280,2820,1140)(0,0,0,0)
Text 0xb213e868(Text(146)) (480,-18000,2820,1140)(0,0,0,0)
Text 0xb213e920(Text(150)) (480,-15720,2820,1140)(0,0,0,0)
Text 0xb213e9d8(Text(154)) (480,-13440,2820,1140)(0,0,0,0)
Text 0xb213ea90(Text(158)) (480,-11160,2820,1140)(0,0,0,0)
Text 0xb213eb48(Text(162)) (480,-8880,2820,1140)(0,0,0,0)
Text 0xb213ec00(Text(166)) (480,-6600,2820,1140)(0,0,0,0)
Text 0xb213ecb8(Text(170)) (480,-4320,2820,1140)(0,0,0,0)
Text 0xb213ed70(Text(174)) (480,-2040,2820,1140)(0,0,0,0)
Text 0xb213ee28(Text(178)) (480,240,2820,1140)(0,0,0,0)
Text 0xb213eee0(Text(182)) (480,2520,2820,1140)(0,0,0,0)
Text 0xb213ef98(Text(186)) (480,4800,2820,1140)(0,0,0,0)
Text 0xb2140080(Text(190)) (480,7080,2820,1140)(0,0,0,0)
Painting --- after optimization:
CanvasBackground 0xb33deb80(Canvas(html)(-1)) (0,-101700,48000,117240)(0,-61080,48000,600) opaque uniform layer=0xb721c8c0
Text 0xb213ca90(Text(70)) (480,-61320,2820,1140)(480,-61080,2820,600) layer=0xb721c8c0
Painting --- retained layer tree:
BasicLayerManager (0xb5da6470)
  BasicContainerLayer (0xb5efc220) [visible=< (x=0, y=-1017, w=800, h=1164); >] [opaqueContent] [metrics={ viewport=(x=0, y=-1017, w=800, h=1164) viewportScroll=(x=0, y=1695) displayport=(x=0, y=-1017, w=800, h=1164) scrollId=1 }]
    BasicThebesLayer (0xb721c8c0) [transform=[ 1 0; 0 1; 0 -1695; ]] [visible=< (x=0, y=677, w=800, h=1165); >] [opaqueContent] [valid=< (x=0, y=677, w=800, h=1165); >] [xres=1.6 yres=1.6]

normally content layout includes footer's (layer=0xb721bbc0) items:

Painting --- before optimization (dirty 0,-34012,48000,69863):
CanvasBackground 0xb33deb80(Canvas(html)(-1)) (0,-81360,48000,117240)(0,0,0,0) opaque uniform
Text 0xb213ccb8(Text(82)) (480,-34140,2820,1140)(0,0,0,0)
Text 0xb213cd70(Text(86)) (480,-31860,2820,1140)(0,0,0,0)
Text 0xb213ce28(Text(90)) (480,-29580,2820,1140)(0,0,0,0)
Text 0xb213cee0(Text(94)) (480,-27300,2820,1140)(0,0,0,0)
Text 0xb213cf98(Text(98)) (480,-25020,2820,1140)(0,0,0,0)
Text 0xb213e080(Text(102)) (480,-22740,2820,1140)(0,0,0,0)
Text 0xb213e138(Text(106)) (480,-20460,2820,1140)(0,0,0,0)
Text 0xb213e1f0(Text(110)) (480,-18180,2820,1140)(0,0,0,0)
Text 0xb213e2a8(Text(114)) (480,-15900,2820,1140)(0,0,0,0)
Text 0xb213e360(Text(118)) (480,-13620,2820,1140)(0,0,0,0)
Text 0xb213e418(Text(122)) (480,-11340,2820,1140)(0,0,0,0)
Text 0xb213e4d0(Text(126)) (480,-9060,2820,1140)(0,0,0,0)
Text 0xb213e588(Text(130)) (480,-6780,2820,1140)(0,0,0,0)
Text 0xb213e640(Text(134)) (480,-4500,2820,1140)(0,0,0,0)
Text 0xb213e6f8(Text(138)) (480,-2220,2820,1140)(0,0,0,0)
Text 0xb213e7b0(Text(142)) (480,60,2820,1140)(0,0,0,0)
Text 0xb213e868(Text(146)) (480,2340,2820,1140)(0,0,0,0)
Text 0xb213e920(Text(150)) (480,4620,2820,1140)(0,0,0,0)
Text 0xb213e9d8(Text(154)) (480,6900,2820,1140)(0,0,0,0)
Text 0xb213ea90(Text(158)) (480,9180,2820,1140)(0,0,0,0)
Text 0xb213eb48(Text(162)) (480,11460,2820,1140)(0,0,0,0)
Text 0xb213ec00(Text(166)) (480,13740,2820,1140)(0,0,0,0)
Text 0xb213ecb8(Text(170)) (480,16020,2820,1140)(0,0,0,0)
Text 0xb213ed70(Text(174)) (480,18300,2820,1140)(0,0,0,0)
Text 0xb213ee28(Text(178)) (480,20580,2820,1140)(0,0,0,0)
Text 0xb213eee0(Text(182)) (480,22860,2820,1140)(0,0,0,0)
Text 0xb213ef98(Text(186)) (480,25140,2820,1140)(0,0,0,0)
Text 0xb2140080(Text(190)) (480,27420,2820,1140)(0,0,0,0)
Text 0xb2140138(Text(194)) (480,29700,2820,1140)(0,0,0,0)
Text 0xb21401f0(Text(198)) (480,31980,2820,1140)(0,0,0,0)
Text 0xb21402a8(Text(202)) (480,34260,2820,1140)(0,0,0,0)
WrapList 0xb21403c8(Block(div)(205)) (0,10140,48000,5400)(0,0,0,0)
    Background 0xb21403c8(Block(div)(205)) (0,10140,48000,5400)(0,0,0,0) opaque uniform
    Text 0xb2140498(Text(0)) (20280,10725,7440,780)(0,0,0,0)
Painting --- after optimization:
CanvasBackground 0xb33deb80(Canvas(html)(-1)) (0,-81360,48000,117240)(0,0,48000,15540) opaque uniform layer=0xb721c8c0
Text 0xb213e7b0(Text(142)) (480,60,2820,1140)(480,60,2820,1140) layer=0xb721c8c0
Text 0xb213e868(Text(146)) (480,2340,2820,1140)(480,2340,2820,1140) layer=0xb721c8c0
Text 0xb213e920(Text(150)) (480,4620,2820,1140)(480,4620,2820,1140) layer=0xb721c8c0
Text 0xb213e9d8(Text(154)) (480,6900,2820,1140)(480,6900,2820,1140) layer=0xb721c8c0
Text 0xb213ea90(Text(158)) (480,9180,2820,1140)(480,9180,2820,1140) layer=0xb721c8c0
Text 0xb213eb48(Text(162)) (480,11460,2820,1140)(480,11460,2820,1140) layer=0xb721c8c0
Text 0xb213ec00(Text(166)) (480,13740,2820,1140)(480,13740,2820,1140) layer=0xb721c8c0
Background 0xb21403c8(Block(div)(205)) (0,10140,48000,5400)(0,10140,48000,5400) opaque uniform layer=0xb721bbc0
Text 0xb2140498(Text(0)) (20280,10725,7440,780)(20280,10725,7440,780) layer=0xb721bbc0
Painting --- retained layer tree:
BasicLayerManager (0xb5da6470)
  BasicContainerLayer (0xb5efc220) [visible=< (x=0, y=-567, w=800, h=1165); >] [opaqueContent] [metrics={ viewport=(x=0, y=-567, w=800, h=1165) viewportScroll=(x=0, y=1356) displayport=(x=0, y=-567, w=800, h=1165) scrollId=1 }]
    BasicThebesLayer (0xb721c8c0) [transform=[ 1 0; 0 1; 0 -1356; ]] [visible=< (x=0, y=789, w=800, h=1165); >] [opaqueContent] [valid=< (x=0, y=789, w=800, h=1165); >] [xres=1.6 yres=1.6]
    BasicThebesLayer (0xb721bbc0) [visible=< (x=0, y=169, w=800, h=90); >] [opaqueContent] [isFixed] [valid=< (x=0, y=169, w=800, h=90); >] [xres=1.6 yres=1.6]


2) Computing/Recomputing visibility: mVisibleRect for each item is restricted by display port rectangle, that is wrong for fixed items. As result footer's layer visible region is incorrect (h=49, h=90 is expected):

Painting --- after optimization:
CanvasBackground 0xb34d6b80(Canvas(html)(-1)) (0,-101460,48000,117240)(0,-57660,48000,900) opaque uniform layer=0xb731cf40
Background 0xb22353c8(Block(div)(205)) (0,10320,48000,5400)(0,10320,48000,2940) opaque uniform layer=0xb731d0e0
Text 0xb2235498(Text(0)) (20280,10937,7440,780)(20280,10937,7440,780) layer=0xb731d0e0
Painting --- retained layer tree:
BasicLayerManager (0xb5eac5c0)
  BasicContainerLayer (0xb5fea220) [visible=< (x=0, y=-961, w=800, h=1181); >] [opaqueContent] [metrics={ viewport=(x=0, y=-961, w=800, h=1181) viewportScroll=(x=0, y=1691) displayport=(x=0, y=-961, w=800, h=1181) scrollId=1 }]
    BasicThebesLayer (0xb731cf40) [transform=[ 1 0; 0 1; 0 -1691; ]] [visible=< (x=0, y=730, w=800, h=1133); >] [opaqueContent] [valid=< (x=0, y=730, w=800, h=1133); >] [xres=1.445 yres=1.445]
    BasicThebesLayer (0xb731d0e0) [visible=< (x=0, y=172, w=800, h=49); >] [opaqueContent] [isFixed] [valid=< (x=0, y=172, w=800, h=49); >] [xres=1.445 yres=1.445]
Created attachment 521506 [details] [diff] [review]
Part 2: Reverse translate in RenderFrameParent

(In reply to comment #33)

> Also if attached testcase change to be heater, then awesomebar covering heater

you need to ignore GetRootFrameOffset() value. I've updated your patch.
Attachment #515350 - Attachment is obsolete: true
Attachment #520929 - Attachment description: DisplayList tricks → Part3: DisplayList tricks
Attachment #514718 - Attachment description: Fixed layers detection part, updated to tip → Part1: Fixed layers detection part, updated to tip
Looking at part 1 again:

+      nsIFrame* activeScrolledRoot =
+        nsLayoutUtils::GetActiveScrolledRootFor(aContainerFrame, mBuilder->ReferenceFrame());
+      colorLayer->SetIsFixed(activeScrolledRoot && nsGkAtoms::viewportFrame == activeScrolledRoot->GetType());

This is wrong. Instead of getting the active scrolled root for the container frame, get it from data->mActiveScrolledRoot --- that's the active scrolled root for this layer.

And instead of checking for viewportFrame, check that the active scrolled root equals mBuilder->ReferenceFrame. We don't want viewport frames of subdocuments to trigger this.

In part 3, make the same change to IsFixedFrame.

Please rename nsDisplayItem::IsFixedAndCoveringViewport to nsDisplayItem::ShouldFixToViewport.

We need to factor out the code in IsFixedItem that computes the active scrolled root to share it with the same code in ContainerState::ProcessDisplayItems. I think that the best approach would be to make nsLayoutUtils::GetActiveScrolledRootFor actually take a display item as a parameter and do the fixup for ShouldFixToViewport itself.

Why is the change to MarkOutOfFlowFrameForDisplay needed? It seems to me that the changes in nsDisplayList::ComputeVisibilityForSublist and nsDisplayItem::RecomputeVisibility should be enough to ensure that fixed-pos frames' display lists are built correctly.

The new code in nsDisplayList::ComputeVisibilityForSublist and nsDisplayItem::RecomputeVisibility should be shared. Perhaps a boolean helper function "ForceVisiblityForFixedItem(nsDisplayListBuilder*, nsDisplayItem*)".
Created attachment 522207 [details] [diff] [review]
Part1: Fixed layers detection part, checks if active scrolled root is equal to builder reference frame
Attachment #514718 - Attachment is obsolete: true
Created attachment 522208 [details] [diff] [review]
Part 3: DisplayList tricks

introduces GetActiveScrolledRootFor(item, builder) in order to share active scrolled root selection code
Attachment #520929 - Attachment is obsolete: true
Attachment #520929 - Flags: feedback?(roc)
Created attachment 522213 [details]
Screenshot1-fennec-long-scroll_footer-is-missing-without-MarkOutOfFlowFrameForDisplay-tricks.png

(In reply to comment #45)

> Why is the change to MarkOutOfFlowFrameForDisplay needed? It seems to me that
> the changes in nsDisplayList::ComputeVisibilityForSublist and
> nsDisplayItem::RecomputeVisibility should be enough to ensure that fixed-pos
> frames' display lists are built correctly.

Unfortunately it's not enough, without this change fixed display items can be excluded from display list before ComputeVisibility. It happens when building display list for viewport frame. Attached Screenshot1 illustrates this case as well as "Painting --- before optimization" logs from comment #43 1)
Created attachment 522217 [details] [diff] [review]
Part 3: DisplayList tricks (v4)

introduced GetActiveScrolledRootFor(item, builder) in order to share active
scrolled root selection code
introduced ForceVisiblityForFixedItem(builder, item) in order to share new Compute/Recompute visibility code
Attachment #522208 - Attachment is obsolete: true
Comment on attachment 522207 [details] [diff] [review]
Part1: Fixed layers detection part, checks if active scrolled root is equal to builder reference frame

Looks good.
Attachment #522207 - Flags: review+
(In reply to comment #48)
> Unfortunately it's not enough, without this change fixed display items can be
> excluded from display list before ComputeVisibility. It happens when building
> display list for viewport frame. Attached Screenshot1 illustrates this case as
> well as "Painting --- before optimization" logs from comment #43 1)

OK I see that now. Thanks.
+  if (nsGkAtoms::viewportFrame == aDirtyFrame->GetType() &&
+      IsFixedFrame(aFrame->GetParent(), aDirtyFrame)) {
+    nsRect displayport;
+    if (GetDisplayPort(aFrame, displayport)) {
+      dirty.MoveBy(-displayport.TopLeft());
+    }
+  }
   if (!dirty.IntersectRect(dirty, overflowRect))
     return;

Here we have the only place where display list code makes assumptions about how the fixed-pos content will actually be drawn. I think we could remove that dependency by simply checking IsFixedFrame() && GetDisplayPort() and if so, just set dirty to overflowRect. That would be less than optimal in theory but perform just the same in almost all cases.

Then you can change GetDisplayPort to HasDisplayPort(), a bit simpler.

Also I think you can simplify IsFixedFrame() here to just check aFrame->GetParent() && !aFrame->GetParent()->GetParent(), i.e. aFrame's parent is the viewport. That's true for all fixed-pos frames.

Come to think of it, I think ForceVisiblityForFixedItem and IsFixedItem probably should not be calling nsLayoutUtils::GetActiveScrolledRootFor to see if the item is fixed. That will return true for background-attachment:fixed CSS backgrounds, which we do not want to be affected by the displayport or marked as "Fixed" layers ... right? Please test those...
Created attachment 522280 [details] [diff] [review]
Fixed layers detection part

Rename method to PosFixed.
Fixed copy/paste typo bug return value float->bool
Attachment #522207 - Attachment is obsolete: true
Attachment #522280 - Flags: review?(roc)
Set/GetIsPosFixed should be "Set/GetIsFixedPosition".
Created attachment 522296 [details] [diff] [review]
Part 1: Fixed layers detection part, FixedPosition name (v5)
Attachment #522280 - Attachment is obsolete: true
Attachment #522296 - Flags: review?(roc)
Attachment #522280 - Flags: review?(roc)
Hmm, this is actually going to make background-attachment:fixed layers be marked with SetIsFixedPosition(true). As in comment #52, I suspect that's not actually what we want.

But we might have a background-attachment:fixed background and a position:fixed element with their contents assigned to the same layer.

How do we want background-attachment:fixed elements to behave here?
Created attachment 522325 [details] [diff] [review]
Part 2: Reverse translate in RenderFrameParent (v5)

updated after Part 1 renamings
Attachment #521506 - Attachment is obsolete: true
Created attachment 522326 [details] [diff] [review]
Part 3: Visibility tricks (v5)
Attachment #522217 - Attachment is obsolete: true
Attachment #522296 - Attachment description: Fixed layers detection part, FixedPosition name → Part 1: Fixed layers detection part, FixedPosition name (v5)
Created attachment 522558 [details] [diff] [review]
Part 2: Reverse translate in RenderFrameParent (v6)

Fixed case with simple fixed position color layer:
<div style="position:fixed;width:100%;height:10px;background-color:green;"></div>
<br>
....
<br>

ClipRect need to be transformed
Attachment #522325 - Attachment is obsolete: true
Attachment #522558 - Flags: review?(roc)
Created attachment 522591 [details] [diff] [review]
One more test to fix

Ok, here is one more test coming from simplified dp.ru page,
BasicShadowContainerLayer (0xb3c3782c) [shadow-transform=[ 3.185 0; 0 3.185; 0 80; ]] [shadow-visible=< (x=0, y=0, w=800, h=1207); >] [visible=< (x=0, y=0, w=800, h=1207); >] [opaqueContent] [metrics={ viewport=(x=0, y=0, w=800, h=1207) viewportScroll=(x=0, y=0) displayport=(x=0, y=0, w=800, h=1207) scrollId=1 }]
  BasicShadowThebesLayer (0xad0afc8c) [shadow-clip=(x=0, y=0, w=0, h=0)] [clip=(x=0, y=0, w=0, h=0)]
  BasicShadowColorLayer (0xb3caa84c) [shadow-clip=(x=0, y=0, w=800, h=1207)] [shadow-visible=< (x=0, y=0, w=800, h=1207); >] [clip=(x=0, y=0, w=800, h=1207)] [visible=< (x=0, y=0, w=800, h=1207); >] [opaqueContent] [color=rgba(255, 255, 255, 1)]
  BasicShadowThebesLayer (0xad0b008c) [shadow-clip=(x=0, y=0, w=0, h=0)] [clip=(x=0, y=0, w=0, h=0)] [isFixedPosition]
  BasicShadowColorLayer (0xb3caa68c) [shadow-clip=(x=8, y=8, w=10, h=10)] [shadow-visible=< (x=8, y=8, w=10, h=10); >] [clip=(x=8, y=8, w=10, h=10)] [visible=< (x=8, y=8, w=10, h=10); >] [opaqueContent] [isFixedPosition] [color=rgba(255, 255, 0, 1)]
  BasicShadowContainerLayer (0xb3c37a0c) [shadow-clip=(x=8, y=8, w=10, h=10)] [shadow-visible=< (x=8, y=8, w=10, h=10); >] [clip=(x=8, y=8, w=10, h=10)] [visible=< (x=8, y=8, w=10, h=10); >] [opaqueContent] [metrics={ viewport=(x=8, y=8, w=10, h=10) viewportScroll=(x=0, y=0) displayport=(x=0, y=0, w=0, h=0) scrollId=3 }]
    BasicShadowThebesLayer (0xad0b048c) [shadow-clip=(x=0, y=0, w=0, h=0)] [shadow-transform=[ 1 0; 0 1; 8 8; ]] [clip=(x=0, y=0, w=0, h=0)] [transform=[ 1 0; 0 1; 8 8; ]]
    BasicShadowColorLayer (0xb3caa4cc) [shadow-clip=(x=8, y=8, w=10, h=10)] [shadow-transform=[ 1 0; 0 1; 8 8; ]] [shadow-visible=< (x=0, y=0, w=10, h=10); >] [clip=(x=8, y=8, w=10, h=10)] [transform=[ 1 0; 0 1; 8 8; ]] [visible=< (x=0, y=0, w=10, h=10); >] [opaqueContent] [color=rgba(0, 0, 0, 1)]

looks like layers inside second shadowContainer not marked as fixed.
Created attachment 522616 [details] [diff] [review]
Part1, Fixed layers detection, Fixed dp.ru case completely
Attachment #522296 - Attachment is obsolete: true
Attachment #522616 - Flags: review?(roc)
Attachment #522296 - Flags: review?(roc)
> How do we want background-attachment:fixed elements to behave here?

with current patches attachment:fixed works the same way as in Desktop Firefox,
so I guess it is ok for now
Attachment #522558 - Flags: review?(roc) → review?(ben)
(In reply to comment #61)
> Created attachment 522616 [details] [diff] [review]
> Part1, Fixed layers detection, Fixed dp.ru case completely

I don't understand why you changed this. It looks to me like your new code will mark every non-ThebesLayer fixed.
For testcase
https://bug607417.bugzilla.mozilla.org/attachment.cgi?id=522591
we have layer structure dumped in comment 607417#c60

and we have there:
BasicShadowContainerLayer(1)
  Fixed Layers1
  BasicShadowContainerLayer(2):
    Layers2 (not marked as fixed with current changes, but should be pos-fixed)

So with this additional change I'm marking BasicShadowContainerLayer(2) as fixed (this is ownLayer) in order to get it's child in fixed position.

I tried to mark as fixed all Layers2, but that is totally breaking layout... so I stopped on version with own container layer mark only
In that case, shouldn't we be marking BasicShadowContainerLayer(2) as fixed? Then the RenderFrameParent code can do the right thing.
Created attachment 522920 [details]
Old version vs new version LayersLOG

>In that case, shouldn't we be marking BasicShadowContainerLayer(2) as fixed?
>Then the RenderFrameParent code can do the right thing.

This is exactly what this fix doing, see Layers Log in attachment:
First one with old version, second one with new patch version.
in second log only BasicShadowContainerLayer(2) marked as fixed (ownLayer)
OK that makes sense. My question now is, why doesn't your patch make *every* ContainerLayer SetIsFixedPosition(true)? Because ultimately if you follow the chain of active scrolled roots like this code does, you will reach the reference frame.

I think you need to be following these chains and checking to see whether you pass through the root scrollframe for the toplevel document. If you do, the content is not fixed-position.

Although that's probably overcomplicated here. Here's what I think the code should be:

For the dedicated layers case:
+      nsIFrame* activeScrolledRoot =
+          nsLayoutUtils::GetActiveScrolledRootFor(item,
+                                                  mBuilder->ReferenceFrame());
+      ownLayer->SetIsFixedPosition(!ScrolledByViewportScrolling (activeScrolledRoot));

For the thebesLayer case:
+      thebesLayer->SetIsFixedPosition(!ScrolledByViewportScrolling (activeScrolledRoot));

For the colorLayer case just copy the ThebesLayer state:
+      colorLayer->SetIsFixedPosition(data->mLayer->GetIsFixedPosition());

where ScrolledByViewportScrolling is defined as
  if (aActiveScrolledRoot == aActiveScrolledRoot->PresContext()->GetPresShell()->GetRootScrollFrame())
    return PR_TRUE;
  if (!aActiveScrolledRoot->GetParent())
    return PR_FALSE;
  return ScrollsWithViewport(GetActiveScrolledRootFor(aActiveScrolledRoot->GetParent()));
Actually I think ScrolledByViewportScrolling should take an aBuilder parameter and be

  nsIFrame* rootScrollFrame = aBuilder->ReferenceFrame()->PresContext()->GetPresShell()->GetRootScrollFrame();
  return nsLayoutUtils::IsAncestorFrame(aActiveScrolledRoot, rootScrollFrame);
Created attachment 523150 [details] [diff] [review]
Part1, Fixed layers detection, v6

Ok, this seems to work, but required to disable reverse translation for fixed frames which are having parent shadow container with fixed position
Attachment #522616 - Attachment is obsolete: true
Attachment #523150 - Flags: review?(roc)
Attachment #522616 - Flags: review?(roc)
Created attachment 523151 [details] [diff] [review]
Part 2: Reverse translate in RenderFrameParent (v7)
Attachment #522558 - Attachment is obsolete: true
Attachment #523151 - Flags: review?(roc)
Attachment #522558 - Flags: review?(ben)
Created attachment 523152 [details] [diff] [review]
Part 3: Visibility tricks (v5.1)

Fixed rejects
Attachment #522326 - Attachment is obsolete: true
Attachment #523152 - Flags: review?(roc)
+  return nsLayoutUtils::IsProperAncestorFrame(rootScrollFrame, aActiveScrolledRoot);

Not "Proper", as we discussed.

+      nsIFrame* f = item->GetUnderlyingFrame();
+      nsIFrame* activeScrolledRoot =
+          nsLayoutUtils::GetActiveScrolledRootFor(f, mBuilder->ReferenceFrame());

GetActiveScrolledRootFor(item)
Attachment #523151 - Flags: review?(roc) → review?(ben)
Comment on attachment 523152 [details] [diff] [review]
Part 3: Visibility tricks (v5.1)

> GetActiveScrolledRootFor(item)

I guess you can move that change to part 3.

+  if (IsFixedItem(aItem, aBuilder)) {
+    if (HasDisplayPort(aItem->GetUnderlyingFrame())) {
+      return PR_TRUE;
+    }
+  }
+  return PR_FALSE;

return IsFixedItem(aItem, aBuilder) && HasDisplayPort(aItem->GetUnderlyingFrame())
Attachment #523152 - Flags: review?(roc) → review+
Created attachment 523235 [details] [diff] [review]
Part1, Fixed layers detection, v7

Fixed comments
Attachment #523150 - Attachment is obsolete: true
Attachment #523235 - Flags: review?(roc)
Attachment #523150 - Flags: review?(roc)
Attachment #523235 - Flags: review?(roc) → review+
(In reply to comment #74)
> Created attachment 523235 [details] [diff] [review]
> Part1, Fixed layers detection, v7
> 
> Fixed comments

doesn't compile without part 3, let's move all needed API changes to part 1

I need to share ScrolledByViewportScrolling() for part3 IsFixedItem(), dp.ru heater is clipped on long scroll otherwise
Created attachment 523286 [details] [diff] [review]
Part1: Fixed layers detection (v8)

moved GetActiveScrolledRootFor(item, builder) and ScrolledByViewportScrolling(activeScrolledRoot, builder) to nsLayoutUtils API

I guess it's safe to move r+ from v7 here
Attachment #523235 - Attachment is obsolete: true
Created attachment 523287 [details] [diff] [review]
Part 3: Visibility tricks (v6)

fixed dp.ru visibility

I guess it's safe to move r+ from v5.1 here
Attachment #523152 - Attachment is obsolete: true
Comment on attachment 523286 [details] [diff] [review]
Part1: Fixed layers detection (v8)

Need r, new layout API moved into this patch
Attachment #523286 - Flags: review?(roc)
Comment on attachment 523287 [details] [diff] [review]
Part 3: Visibility tricks (v6)

Tested this version quickly, and fixed display items stay visible now on testcase https://bugzilla.mozilla.org/attachment.cgi?id=522591
Attachment #523287 - Flags: review?(roc)
Attachment #523286 - Flags: review?(roc) → review+
Comment on attachment 523151 [details] [diff] [review]
Part 2: Reverse translate in RenderFrameParent (v7)

>-                    float aXScale = 1, float aYScale = 1)
>+                    float aXScale = 1, float aYScale = 1,
>+                    float translateX = 0, float translateY = 0)

Hm, so you need to track the translations that have been applied so far? It's starting to sound like scale and translation for this function can be replaced with a ViewTransform.

>+  if (aLayer->GetIsFixedPosition() &&
>+      !aLayer->GetParent()->GetIsFixedPosition()) {
>+    shadowTransform._41 -= translateX / aXScale;
>+    shadowTransform._42 -= translateY / aYScale;
>+    const nsIntRect* clipRect = shadow->GetShadowClipRect();

Make functions for accesses to _41 and _42 like we do for scale:
http://mxr.mozilla.org/mozilla-central/source/layout/ipc/RenderFrameParent.cpp#87


>+    if (clipRect) {
>+      nsIntRect transformedClipRect(*clipRect);
>+      transformedClipRect.MoveBy(shadowTransform._41, shadowTransform._42);
>+      shadow->SetShadowClipRect(&transformedClipRect);
>+    }
>+  }
>+

I wouldn't expect the clip rect transformation to be necessary. I'm a little fuzzy on what space the clip rectangle is defined in, but for the other transformations that happen we never set the shadow clip rect. If we have to here, don't we need to for generic transformations? Including scale?
Attachment #523151 - Flags: review?(ben)
Attachment #523287 - Flags: review?(roc) → review+
Comment on attachment 523287 [details] [diff] [review]
Part 3: Visibility tricks (v6)

actually, HasDisplayPort should be checking the ReferenceFrame()'s prescontext's scrollframe, not aFrame's, because we always want to be checking for a displayport in the toplevel document.
Attachment #523287 - Flags: review+ → review-
Also I'm a bit worried about the performance impact of IsFixedItem. nsLayoutUtils::GetActiveScrolledRootFor requires a walk up the frame tree.

At the least, we should store a flag in nsDisplayListBuilder representing the result of HasDisplayPort, so we can just check that flag for almost-free. Then we should check HasDisplayPort first in ForceVisiblityForFixedItem. That will at least prevent desktop from regressing, and it's simple. We should definitely do this.

To speed up IsFixedItem, I suggest optimizing for the common case where there is no fixed item. so I'd add a flag to nsDisplayListBuilder "mHasFixedItems" which we set to true when mReferenceFrame is a viewport frame with fixed-pos children, or when we construct an item which will return true from ShouldFixToViewport. But you might want to check with profiling to see if this is a real issue. I suggest profiling painting of a page with lots of very small visible elements.
> I wouldn't expect the clip rect transformation to be necessary. I'm a little

In order to render fixed-pos colored div, it is ctx->rectangle (cliprect) + ctx->fill

So transformation does not make any sense with color layers.
In that case, could you fix the other layers' clip rects as well?
Created attachment 523515 [details] [diff] [review]
Part 2: Reverse translate in RenderFrameParent (v8)

> In that case, could you fix the other layers' clip rects as well?
Not sure about it, would be nice to find testcase if that is the problem
Attachment #523151 - Attachment is obsolete: true
Attachment #523515 - Flags: review?(ben)
Comment on attachment 523515 [details] [diff] [review]
Part 2: Reverse translate in RenderFrameParent (v8)

>+  ViewTransform(nsIntPoint aTranslation = nsIntPoint(0, 0), float aXScale = 1, float aYScale = 1)

Can't tell if this is over 80 characters, but please make sure it is. :)

> // Go down shadow layer tree and apply transformations for scrollable layers.
> static void
> TransformShadowTree(nsDisplayListBuilder* aBuilder, nsFrameLoader* aFrameLoader,
>                     nsIFrame* aFrame, Layer* aLayer,
>-                    float aXScale = 1, float aYScale = 1)
>+                    ViewTransform& aTransform)

Small nit: is there any reason this is passed by reference? Callers will need to be a little more careful than necessary. Since this is just a helper parameter for the recursive call, I'd rather the caller not have to deal with this. Could we either make this by-value and set it to a default view transform, or change this to a recursive helper function and have a small function that starts the recursive helper? IMO by value is just fine.

>+  if (aLayer->GetIsFixedPosition() &&
>+      !aLayer->GetParent()->GetIsFixedPosition()) {
>+    ReverseTranslate(shadowTransform, aTransform);
>+    const nsIntRect* clipRect = shadow->GetShadowClipRect();
>+    if (clipRect) {
>+      nsIntRect transformedClipRect(*clipRect);
>+      transformedClipRect.MoveBy(shadowTransform._41, shadowTransform._42);
>+      shadow->SetShadowClipRect(&transformedClipRect);
>+    }
>+  }

OK, as we discussed on IRC this makes sense to me now, because we are dealing with a leaf node, not a container layer. Add a comment here to explicitly note we are dealing with a leaf node, and that's why we have to transform the clipping rectangle.

r+ with these addressed! Great job!
Attachment #523515 - Flags: review?(ben) → review+
Created attachment 523863 [details] [diff] [review]
Part 3: Visibility tricks (v7)
Attachment #523287 - Attachment is obsolete: true
Attachment #523863 - Flags: review?(roc)
You need to set HasFixedItems if we construct a background-attachment:fixed nsDisplayBackground item.
Created attachment 523958 [details] [diff] [review]
Part 3: Visibility tricks (v8)
Attachment #523863 - Attachment is obsolete: true
Attachment #523958 - Flags: review?(roc)
Attachment #523863 - Flags: review?(roc)
Try build with latest patches, tested on android - works great:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/romaxa@gmail.com-cf00d3e22402/
Attachment #523958 - Flags: review?(roc) → review+
BTW we definitely *have* to have some tests here. reftests can test displayport.
Created attachment 524136 [details] [diff] [review]
Part1: Fixed layers detection (v8) (latest mc tip rejects - safe)
Keywords: checkin-needed

Updated

6 years ago
Attachment #523286 - Attachment is obsolete: true
I couldn't figure out which patches here should land.  Please attach an hg bundle with individual commits for each patch (with the correct summary/author info) (or put the attachment IDs of the patches which should land in the whiteboard field, in the correct order.)

Thanks!
Keywords: checkin-needed
(In reply to comment #91)
> BTW we definitely *have* to have some tests here. reftests can test
> displayport.

trying to run IPC tests locally, and see "can't drawWindow remote content" unexpected fail... is there are some problems? or anything missing in my environment?
oleg, check bug 648104.
Created attachment 524288 [details] [diff] [review]
Reftest for this feature
Attachment #524288 - Flags: review?(jones.chris.g)
Attachment #524288 - Flags: review?(jones.chris.g) → review?(dbaron)
Comment on attachment 524288 [details] [diff] [review]
Reftest for this feature

not sure if David around now, maybe Chris can look at this reftest?
Attachment #524288 - Flags: review?(jones.chris.g)
Comment on attachment 524288 [details] [diff] [review]
Reftest for this feature

reftest changes and the test itself look OK.  The semantics of position:fixed in that test are interesting, but makes sense I guess.  I don't think dbaron needs to review this too.
Attachment #524288 - Flags: review?(jones.chris.g)
Attachment #524288 - Flags: review?(dbaron)
Attachment #524288 - Flags: review?
Attachment #524288 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/0f077c086750
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Depends on: 650759
Attachment #524288 - Flags: review? → review+
Flags: in-testsuite+
without zooming, it works.  with zooming found bug 52234 	

Mozilla/5.0 (Android; Linux armv71; rv6.0a1) Gecko/20110422 Firefox/6.0a1 Fennec/6.0a1
Device: Thunderbolt
OS: Android 2.2
correction : bug 652234

(In reply to comment #100)
> without zooming, it works.  with zooming found bug 652234     
> 
> Mozilla/5.0 (Android; Linux armv71; rv6.0a1) Gecko/20110422 Firefox/6.0a1
> Fennec/6.0a1
> Device: Thunderbolt
> OS: Android 2.2
Status: RESOLVED → VERIFIED
Depends on: 653133
Duplicate of this bug: 500700
Depends on: 656036
Depends on: 656114
Depends on: 656167
This feature disabled in bug 656167, so we need to reopen this bug
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Depends on: 668698
tracking-fennec: - → ?

Updated

6 years ago
Duplicate of this bug: 673724
Duplicate of this bug: 601832
tracking-fennec: ? → +
also see bug 668698

Updated

6 years ago
Keywords: mobile
Duplicate of this bug: 691074
Resurrecting this bug thread...

Is bug 656036 our only injection from this fix? That seems to be a is a lesser issue compared having no fixed-pos layout at all. Need a Product Team call on this one.

Comment 109

6 years ago
IMO, worse is better. We're completely broken in too many places today. I'd take that regression if we could get this working for the not-zoomed case and try to tackle the regression later.
Created attachment 568262 [details] [diff] [review]
Enable fixed position content support for async scrolling

Not sure who should review this
Attachment #568262 - Flags: review?(roc)
(In reply to Oleg Romashin (:romaxa) from comment #110)
> Created attachment 568262 [details] [diff] [review] [diff] [details] [review]
> Enable fixed position content support for async scrolling

what about tests?
Created attachment 568292 [details] [diff] [review]
Enable fixed position content support for async scrolling

Enabled test back
Attachment #568262 - Attachment is obsolete: true
Attachment #568292 - Flags: review?(roc)
Attachment #568262 - Flags: review?(roc)
Attachment #568292 - Flags: review?(roc) → review+
(In reply to Asa Dotzler [:asa] from comment #109)
> IMO, worse is better. We're completely broken in too many places today.

I disagree.  I think that our current behavior (updating the layout when scrolling finishes) is ugly, but at least not "broken" -- pages render correctly and with the same layout as desktop Firefox and other browsers.

If we land this bug without fixing bug 656036, then I think many pages *will* be truly broken, because they will lay out incorrectly.  In many cases this can cause fixed headers/footers/sidebars to obscure content when you zoom in to read it, making pages unusable.

I do think that making fixed-position layers work smoothly in Fennec is a high priority, but I really think we need to decide first on how to resolve bug 656036, and figure out how much effort that will take.  Also note that this bug does not apply at all to the non-e10s Fennec that we plan to ship very soon on Android.
(In reply to Matt Brubeck (:mbrubeck) from comment #113)
> Also note that
> this bug does not apply at all to the non-e10s Fennec that we plan to ship
> very soon on Android.

We've decided to trade this off for the first stood-up version, but it's going to come up again very soon.  In-process/out-of-process isn't the meaningful distinction, it's off-main-content-thread compositing, which we're sort of hacking into the first iteration of the new native UI.
Problem with broken position while zooming probably easily fixable.
Problem with obscured content is more like design problem.... for example on news.goolge.com, in very high zoom level left "Top Stories" sidebar covering whole screen...
Yes, it is the problem, but we need to finally decide what to do in that case:
1) Keep fixed content with original zoom level
or
2) Hide fixed content if user zoom to level different than initial page zoom level.
or
3) Add some UI option to hide fixed content.

but we should also remember that fixed content sometime is actual main content of the page... for example maps.yandex.ru (desktop version, require to ovverride UA with desktop UA)
try build with enable patch applied
https://tbpl.mozilla.org/?tree=Try&rev=1a9ee6935ae5
BTw, IIUC problem with obscured content will be reproducible also with Native browser and FF browser running on small screen
(In reply to Oleg Romashin (:romaxa) from comment #117)
> BTw, IIUC problem with obscured content will be reproducible also with
> Native browser and FF browser running on small screen

That's not quite true.  See bug 656036 comment 14 for a test case where content is obscured in Fennec but not in desktop Firefox.
Depends on: 699262
Depends on: 701190
Ok, last important bugs has been fixed, can we enable it? Matt could you check latest builds?
Any updates, can we push fixed frames enabler?
It's up to Matt.
I have no real objection to landing this as-is, though I'd still like to see us moving forward soon on fixing elements to the CSS viewport rather than the screen, when zoomed in (bug 656036).  This would fix a fairly major difference in fennec vs. desktop rendering that was introduced by these patches.

Currently we are using shadow layers only in XUL Fennec, and by Firefox 12 we should be shipping XUL Fennec only on tablets.  The viewport issues aren't as critical on tablets because the screen size is usually much closer to the CSS viewport size, compared to a phone.  But when we enable this code for our other front-ends, it will become more important again.
The new fennec frontend will be using this code soon after its first release, and b2g will be using it in the near future.

Updated

5 years ago
Blocks: 705506
Duplicate of this bug: 743447
(Assignee)

Comment 125

5 years ago
It'd be really great to have this in fennec 1.0 - currently, if you go to a site like desktop facebook, fixed position elements jump around the screen as you pan. With these patches, they should stay in place (I believe).

I'll see if these patches still apply, and possibly rebase if not.
blocking-fennec1.0: --- → ?

Comment 126

5 years ago
Chris, can you fire a try build with these patches?

Romaxa, do you know how these patches interact with OMTC?

Wondering if this helps bug 743247
All these patches already landed as part of RenderFrameParent...
These should be easy to apply on top of CompositorParent creature and use same approach there.
(Assignee)

Comment 128

5 years ago
(In reply to Oleg Romashin (:romaxa) from comment #127)
> All these patches already landed as part of RenderFrameParent...
> These should be easy to apply on top of CompositorParent creature and use
> same approach there.

If this is fixed, why is the bug open? I'll have a look at employing the same logic in CompositorParent as RenderFrameParent.
(Assignee)

Comment 129

5 years ago
I wasted a day looking into getting this working for native android fennec - the patch to CompositorParent is easy enough, but there are issues in layout that are stopping it from working.

It seems every layer created, except for the root layer, is marked as fixed position. My hunch is that it's because our root content scroll frame doesn't correspond to the reference frame's root scroll frame in nsLayoutUtils::ScrolledByViewportScrolling - I don't know if this is the case yet, but I can't easily see anything else going wrong...

Need someone from layout to advise.
(Assignee)

Comment 130

5 years ago
Created attachment 617047 [details] [diff] [review]
Reconcile async scrolling on native android fennec

This patch sort of fixes it for OMTC - I think there are more tweaks needed, as I can sometimes see fixed position elements painted twice - once on the fixed position layer and again underneath it (which you can expose via overscroll).

Also, the part in nsLayoutUtils is clearly a hack, and so I'm asking for feedback on how to fix this properly.
Attachment #617047 - Flags: feedback?(tnikkel)
Attachment #617047 - Flags: feedback?(romaxa)
Attachment #617047 - Flags: feedback?(roc)
(Assignee)

Comment 131

5 years ago
For reference, I find http://bradfrostweb.com/blog/mobile/fixed-position/ to be a great site to test this.
Created attachment 617052 [details]
OMTC embedding with MOZ_ENABLE_FIXED_POSITION_LAYERS=1

I have build of Gecko Embedding with OMTC enabled
and by loading test https://bugzilla.mozilla.org/attachment.cgi?id=486294
I this output in attachment:
I don't see all layers marked as fixed, only expected one.
Created attachment 617132 [details]
OMTC vs IPC layers log tree

Ok, OMTC and IPC layers tree looks different
and something get confused and setted up Fixed flags not correctly
Attachment #617052 - Attachment is obsolete: true
oh, display port for scroll=1 container is 0.
Probably we should set display port for that correctly
Ok, I manually called SetDisplayPort on Embedding paint event
And got proper layer tree ready to work:
1034738752[3dc647f0]: OGLLayerManager (0x3dc8d880)
1034738752[3dc647f0]:   OGLShadowContainerLayer (0x3dc31bc0) [shadow-visible=< (x=0, y=0, w=854, h=960); >] [visible=< (x=0, y=0, w=854, h=960); >] [opaqueContent] [metrics={ viewport=(x=0, y=0, w=854, h=960) viewportScroll=(x=0, y=0) displayport=(x=0, y=0, w=854, h=960) scrollId=1 }]
1034738752[3dc647f0]:     OGLShadowThebesLayer (0x46dea890) [shadow-visible=< (x=0, y=0, w=854, h=960); >] [visible=< (x=0, y=0, w=854, h=960); >] [opaqueContent] [valid=< (x=0, y=0, w=854, h=960); >]
1034738752[3dc647f0]:     OGLShadowThebesLayer (0x46deac90) [shadow-visible=< (x=0, y=390, w=854, h=90); >] [visible=< (x=0, y=390, w=854, h=90); >] [opaqueContent] [isFixedPosition] [valid=< (x=0, y=390, w=854, h=90); >]

So with patch above it probably should work fine...
Chris check Layers log and if you set properly DisplayPort for OMTC child domWindow element
(In reply to Chris Lord [:cwiiis] from comment #129)
> It seems every layer created, except for the root layer, is marked as fixed
> position. My hunch is that it's because our root content scroll frame
> doesn't correspond to the reference frame's root scroll frame in
> nsLayoutUtils::ScrolledByViewportScrolling - I don't know if this is the
> case yet, but I can't easily see anything else going wrong...

That's right. A layer is marked fixed-position if it doesn't move when the root document's root scrollframe scrolls. But with the current mobile fennec front end, that's not what you want.
So there's really an API question here: how should we indicate to layout which scrollframe you want to be the one that determines fixedness?

Adding a new API like we did in bug 732016 might be the way to go.
blocking-fennec1.0: ? → +

Updated

5 years ago
Duplicate of this bug: 748641
Comment on attachment 617047 [details] [diff] [review]
Reconcile async scrolling on native android fennec

Tested this patch in my omtc embedding environment and it works ok (haven't tested zoom values)
Attachment #617047 - Flags: feedback?(romaxa) → feedback+
(Assignee)

Comment 140

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) (offline April 22-25) from comment #137)
> So there's really an API question here: how should we indicate to layout
> which scrollframe you want to be the one that determines fixedness?
> 
> Adding a new API like we did in bug 732016 might be the way to go.

I was discussing this with Brad, rather than adding an API, could we not determine the relative scroll-frame by walking up the frame-tree (not cross-document) and taking the first frame with a display-port set?

If there's none set, we likely don't want to mark any of the layers as fixed, as this won't break layers in iframes.

This would work with both xul-fennec and native android fennec without adding any API.
(In reply to Chris Lord [:cwiiis] from comment #140)
> I was discussing this with Brad, rather than adding an API, could we not
> determine the relative scroll-frame by walking up the frame-tree (not
> cross-document) and taking the first frame with a display-port set?

Why not walk cross-document?

The real question here is what the API contract is going to be between Gecko and the compositor client that's using setDisplayport and reading the fixed flag on the layers.

As I see it, the problem is there's no clear way to cross-reference displayports (which are on elements) with layers.
(Assignee)

Comment 142

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #141)
> (In reply to Chris Lord [:cwiiis] from comment #140)
> > I was discussing this with Brad, rather than adding an API, could we not
> > determine the relative scroll-frame by walking up the frame-tree (not
> > cross-document) and taking the first frame with a display-port set?
> 
> Why not walk cross-document?

So that fixed position elements in iframes don't get marked as fixed. The rationale here is that if a display-port is set on a scroll-frame, we're doing async scrolling on that frame (I don't like this, but I think it's better than the current assumption of 'if the primary child of the presshell is a scroll-frame with a display-port, we're doing async scrolling on that frame').

> The real question here is what the API contract is going to be between Gecko
> and the compositor client that's using setDisplayport and reading the fixed
> flag on the layers.
> 
> As I see it, the problem is there's no clear way to cross-reference
> displayports (which are on elements) with layers.

I think you understand the domain a lot better than I do, so I'm likely making ill-informed assumptions. Is it worth me cooking up a patch doing what I'm suggesting, or do you think we should hash out a better solution?

I figure what's there is currently a hack (and undocumented at that), so replacing it with a hack that works in more of the cases we want it to (and documenting it) would be a reasonable thing to do.
(Assignee)

Updated

5 years ago
Depends on: 749357
How about this, then:
Mark a layer fixed if and only if:
-- It belongs to a content document
-- Its topmost content document ancestor has a root scroll frame with a displayport set
-- The layer does not move when that scroll frame scrolls

You won't be able to do anything clever with position:fixed frames in IFRAMEs, but it sounds like that's OK for you.

OK?
(Assignee)

Comment 144

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #143)
> How about this, then:
> Mark a layer fixed if and only if:
> -- It belongs to a content document
> -- Its topmost content document ancestor has a root scroll frame with a
> displayport set
> -- The layer does not move when that scroll frame scrolls
> 
> You won't be able to do anything clever with position:fixed frames in
> IFRAMEs, but it sounds like that's OK for you.
> 
> OK?

This sounds fine, I think. I'll give it a go (though any further help/suggestions are appreciated).
Stop, I'm working on a patch :-)
Created attachment 618899 [details] [diff] [review]
fix

Try this.
(Assignee)

Comment 147

5 years ago
Comment on attachment 618899 [details] [diff] [review]
fix

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

This looks like it'll do what we want, I'll try it out tomorrow. Thanks!

::: layout/base/nsLayoutUtils.h
@@ +342,5 @@
>  
> +  /**
> +   * Returns true if aActiveScrolledRoot is in a content document,
> +   * and its topmost content document ancestor has a root scroll frame with
> +   * a displayport set, and that

I guess this sentence is meant to end? :)
(Assignee)

Comment 148

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #146)
> Created attachment 618899 [details] [diff] [review]
> fix
> 
> Try this.

To follow up, this does indeed work :) Can you suggest a good person to review this?
(Assignee)

Comment 149

5 years ago
A further layout problem I'm having with this, is that fixed position elements aren't laid out with respect to the render resolution (so zooming in causes elements fixed to the lower/right edges of the page to disappear), and it seems the content size is always the size of the parent and not the element itself? (so even if it did lay out correctly, I can't reconcile the difference when async zooming)

To simplify:
On http://bradfrostweb.com/demo/fixed/index.html, after zooming in, the fixed bottom bar is off-screen - I would expect this to remain fixed to the lower edge of the screen. I would hope that its size remains the same, however, or our async zoom will look bad (have a look at the browser on a Galaxy Nexus for an example of what I'd like to achieve).

I would also expect some way to find out the size and position of the element on the layer-side. I thought that I'd be able to run the content size from the parent's FrameMetrics through the layer transform (or the effective layer transform?) to figure out where it is on screen, but it appears the content size is always the size of the page in this example. I may be misunderstanding what content size is.

Any advice/comments?
(Assignee)

Comment 150

5 years ago
Created attachment 619155 [details] [diff] [review]
Reconcile async scrolling on native android fennec

This depends on attachment #618899 [details] [diff] [review]. Further to my first patch, this makes sure fixed position layers stay within the page boundaries, which looks much better during overscroll.
Attachment #617047 - Attachment is obsolete: true
Attachment #619155 - Flags: review?(roc)
Attachment #617047 - Flags: feedback?(tnikkel)
Attachment #617047 - Flags: feedback?(roc)
Attachment #618899 - Flags: review?(tnikkel)
Comment on attachment 619155 [details] [diff] [review]
Reconcile async scrolling on native android fennec

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

Looks reasonable but I don't know the CompositorParent code. Presumably someone does... someone who wrote it?
Attachment #619155 - Flags: feedback+
(In reply to Chris Lord [:cwiiis] from comment #149)
> To simplify:
> On http://bradfrostweb.com/demo/fixed/index.html, after zooming in, the
> fixed bottom bar is off-screen - I would expect this to remain fixed to the
> lower edge of the screen.

Layout positions and sizes position:fixed elements based on the edges of the CSS viewport. When you zoom in, I assume some edges of the CSS viewport go offscreen. We could add another API to let mobile UI tell layout about a different rect to size and position position:fixed elements with respect to --- the actual screen rect I guess. Does that make sense?

> I would also expect some way to find out the size and position of the
> element on the layer-side. I thought that I'd be able to run the content
> size from the parent's FrameMetrics through the layer transform (or the
> effective layer transform?) to figure out where it is on screen, but it
> appears the content size is always the size of the page in this example. I
> may be misunderstanding what content size is.

The parent's FrameMetrics are just the FrameMetrics for the root scroll frame, which is the size of the CSS viewport.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #152)
> Layout positions and sizes position:fixed elements based on the edges of the
> CSS viewport. When you zoom in, I assume some edges of the CSS viewport go
> offscreen. We could add another API to let mobile UI tell layout about a
> different rect to size and position position:fixed elements with respect to
> --- the actual screen rect I guess. Does that make sense?
> 

Jumping in here, but if I understand correctly, this rect will always have the size we pass in to the newly-added SetScrollPositionClampingScrollPortSize method on DOMWindowUtils. (And the offset would be the same as window scroll position).
(In reply to Kartikaya Gupta (:kats) from comment #153)
> Jumping in here, but if I understand correctly, this rect will always have
> the size we pass in to the newly-added
> SetScrollPositionClampingScrollPortSize method on DOMWindowUtils. (And the
> offset would be the same as window scroll position).

No. Currently SetScrollPositionClampingScrollPortSize only changes the viewport size used for clamping of scroll position. Everything else is unaffected.
(In reply to Timothy Nikkel (:tn) from comment #154)
> (In reply to Kartikaya Gupta (:kats) from comment #153)
> No. Currently SetScrollPositionClampingScrollPortSize only changes the
> viewport size used for clamping of scroll position. Everything else is
> unaffected.

I think you misunderstood what I was saying. I know that SetScrollPositionClampingScrollPortSize only changes the viewport size used for clamping of scroll position. What I'm saying is that I believe the rect that roc was describing as "the actual screen rect" in comment 152 could be created from the information provided to layout via scrollTo and SetScrollPositionClampingScrollPortSize.
I think it might be better to have an explicit way to set the rectangle for fixed-position elements, rather than overloading the scroll-clamping-viewport-size.
(Assignee)

Comment 157

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #156)
> I think it might be better to have an explicit way to set the rectangle for
> fixed-position elements, rather than overloading the
> scroll-clamping-viewport-size.

This sounds fine to me, especially if we decide we'd like to attach fixed position elements to the CSS viewport instead of the screen (an idea I don't like, but can understand the reasoning behind)
Just to be clear: with these two patches, fixed-pos stuff works OK when you're not zoomed (so the CSS viewport exactly fills the screen)?
(Assignee)

Comment 159

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #158)
> Just to be clear: with these two patches, fixed-pos stuff works OK when
> you're not zoomed (so the CSS viewport exactly fills the screen)?

That's correct.

If fixed positioned layers were laid out to the (zoomed) screen, we'd also need a way to reconcile that when async zooming - can you think of a good way to get the size/position of the element a fixed position layer represents? (or some other way to reconcile this difference?)

Note, the result with these two patches may be good enough for fennec 1.0, but it'd be good if we can match the Android ICS browser.
(In reply to Chris Lord [:cwiiis] from comment #159)
> If fixed positioned layers were laid out to the (zoomed) screen, we'd also
> need a way to reconcile that when async zooming - can you think of a good
> way to get the size/position of the element a fixed position layer
> represents? (or some other way to reconcile this difference?)

I'm not sure what you mean by "reconcile".

If you want the fixed-pos elements to stay in the right place during async zooming or panning, we need to expose information about their layout constraints to the async zooming/panning code. Is that what you want?

Let's get these two patches landed ASAP in any case.
(Assignee)

Comment 161

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #160)
> (In reply to Chris Lord [:cwiiis] from comment #159)
> > If fixed positioned layers were laid out to the (zoomed) screen, we'd also
> > need a way to reconcile that when async zooming - can you think of a good
> > way to get the size/position of the element a fixed position layer
> > represents? (or some other way to reconcile this difference?)
> 
> I'm not sure what you mean by "reconcile".
> 
> If you want the fixed-pos elements to stay in the right place during async
> zooming or panning, we need to expose information about their layout
> constraints to the async zooming/panning code. Is that what you want?

That's exactly right.

> Let's get these two patches landed ASAP in any case.

Agreed.
(Assignee)

Updated

5 years ago
Attachment #619155 - Flags: review?(roc) → review?(ajuma)
(In reply to Chris Lord [:cwiiis] from comment #161)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment
> > If you want the fixed-pos elements to stay in the right place during async
> > zooming or panning, we need to expose information about their layout
> > constraints to the async zooming/panning code. Is that what you want?
> 
> That's exactly right.

If we do what I suggested in comment #152, so that position:fixed elements are laid out relative to the screen edges, then when async zooming all you have to do is not asynchronously zoom the fixed layers, and they'll stay in the right place.
(Assignee)

Comment 163

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #162)
> (In reply to Chris Lord [:cwiiis] from comment #161)
> > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment
> > > If you want the fixed-pos elements to stay in the right place during async
> > > zooming or panning, we need to expose information about their layout
> > > constraints to the async zooming/panning code. Is that what you want?
> > 
> > That's exactly right.
> 
> If we do what I suggested in comment #152, so that position:fixed elements
> are laid out relative to the screen edges, then when async zooming all you
> have to do is not asynchronously zoom the fixed layers, and they'll stay in
> the right place.

This isn't true, as a layer's transform and valid region are relative to 0,0 - asynchronously zooming layers (as we currently do) causes them to expand from the bottom-right edge. We would need to know their position so that we can correct the transformation to be relative to the correct coordinates.
(Assignee)

Comment 164

5 years ago
(In reply to Chris Lord [:cwiiis] from comment #163)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment
> #162)
> > (In reply to Chris Lord [:cwiiis] from comment #161)
> > > (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment
> > > > If you want the fixed-pos elements to stay in the right place during async
> > > > zooming or panning, we need to expose information about their layout
> > > > constraints to the async zooming/panning code. Is that what you want?
> > > 
> > > That's exactly right.
> > 
> > If we do what I suggested in comment #152, so that position:fixed elements
> > are laid out relative to the screen edges, then when async zooming all you
> > have to do is not asynchronously zoom the fixed layers, and they'll stay in
> > the right place.
> 
> This isn't true, as a layer's transform and valid region are relative to 0,0
> - asynchronously zooming layers (as we currently do) causes them to expand
> from the bottom-right edge. We would need to know their position so that we
> can correct the transformation to be relative to the correct coordinates.

Sorry, this isn't quite right either - but a layer's 0,0 is the 0,0 of the window - so a fixed position element at the bottom of the screen, when scaled, moves downwards, as its valid region doesn't begin at 0,0.

e.g. - a fixed position element of size 100x100 at 100,100 has a valid region of 100,100-100x100 - so when scaled x2, its new origin is at 200,200. Layers, as far as I know, don't have any idea of position or size as such - so we currently can't correct a fixed layer's position when zooming.

Comment 165

5 years ago
Comment on attachment 619155 [details] [diff] [review]
Reconcile async scrolling on native android fennec

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

This looks good overall; r- just for the thread-safety issue.

::: gfx/layers/ipc/CompositorParent.cpp
@@ +293,5 @@
>    return root;
>  }
> +
> +static void
> +ReverseViewTransform(gfx3DMatrix& aTransform,

Since this is just reversing the translation, how about calling this ReverseViewTransformTranslation?

@@ +407,5 @@
> +
> +    // Alter the scroll offset so that fixed position layers remain within
> +    // the page area.
> +    nsIntRect widgetBounds;
> +    mWidget->GetBounds(widgetBounds);

This isn't thread-safe (see Bug 717925). But we can still get the information we need: in CompositorParent::AllocPLayers, we get the initial size, and whenever the the size changes, we get a call to CompositorParent::ResumeCompositionAndResize with the new size. So just store the size in an instance variable.

@@ -411,5 @@
>        new LayerManagerOGL(mWidget, rect.width, rect.height, true);
>  #else
>      nsRefPtr<LayerManagerOGL> layerManager = new LayerManagerOGL(mWidget);
>  #endif
> -    mWidget = NULL;

This is done intentionally to prevent (thread-unsafe) access to the widget from the compositor. Please leave this in. (It's worth adding a comment explaining that this shouldn't be removed.)

::: gfx/layers/ipc/CompositorParent.h
@@ +133,5 @@
>     */
>    Layer* GetPrimaryScrollableLayer();
> +
> +  /**
> +   * Recursively reverses the given ViewTransform on all fixed position layers

"reverses the translation portion of the given ViewTransform"?
Attachment #619155 - Flags: review?(ajuma) → review-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #152)
> Layout positions and sizes position:fixed elements based on the edges of the
> CSS viewport. When you zoom in, I assume some edges of the CSS viewport go
> offscreen. We could add another API to let mobile UI tell layout about a
> different rect to size and position position:fixed elements with respect to
> --- the actual screen rect I guess. Does that make sense?

For compatibility reasons* I think it is better to fix elements to the CSS viewport, but also allow panning while zoomed in to pan to any part of the CSS viewport.  There's much more discussion of this in bug 656036.

*If you fix elements relative to the screen, then you end up with fixed sidebars obscuring the main column when you zoom in to read, for example.  We had this problem in XUL Fennec nightlies for a while.
(Assignee)

Comment 167

5 years ago
(In reply to Matt Brubeck (:mbrubeck) from comment #166)
> (In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment
> #152)
> > Layout positions and sizes position:fixed elements based on the edges of the
> > CSS viewport. When you zoom in, I assume some edges of the CSS viewport go
> > offscreen. We could add another API to let mobile UI tell layout about a
> > different rect to size and position position:fixed elements with respect to
> > --- the actual screen rect I guess. Does that make sense?
> 
> For compatibility reasons* I think it is better to fix elements to the CSS
> viewport, but also allow panning while zoomed in to pan to any part of the
> CSS viewport.  There's much more discussion of this in bug 656036.
> 
> *If you fix elements relative to the screen, then you end up with fixed
> sidebars obscuring the main column when you zoom in to read, for example. 
> We had this problem in XUL Fennec nightlies for a while.

After some discussion on IRC, I agree with this behaviour (which is as suggested, with slight alterations):
- Respect user-scalable viewport tag (bug 707571, blocker)
- Fix to the larger of the screen or the CSS viewport

I think with roc's suggestion of having a function to set a rect for fixed position elements to be relative to should make this implementable - we'd also need to alter the scroll/zoom code in browser.js somewhat.
(Assignee)

Comment 168

5 years ago
Created attachment 619666 [details] [diff] [review]
Reconcile async scrolling in CompositorParent

Addresses review comments.
Attachment #619155 - Attachment is obsolete: true
Attachment #619666 - Flags: review?(ajuma)

Comment 169

5 years ago
Comment on attachment 619666 [details] [diff] [review]
Reconcile async scrolling in CompositorParent

Looks good to me.
Attachment #619666 - Flags: review?(ajuma) → review+
Comment on attachment 618899 [details] [diff] [review]
fix

+  /**
+   * CONSTRUCTION PHASE ONLY
+   * A layer is "fixed position" when it draws content from a content
+   * (not chrome) document, the topmost content document's is using a
+   * displayport, but the layer does not move when that displayport scrolls.
+   */

I think "the topmost content document's is using a" is missing some words.

+  /**
+   * Returns true if aActiveScrolledRoot is in a content document,
+   * and its topmost content document ancestor has a root scroll frame with
+   * a displayport set, and that
+   */

This comment just stops mid-sentence.
Attachment #618899 - Flags: review?(tnikkel) → review+
(Assignee)

Comment 171

5 years ago
Bug 750535 is also blocking on layers having some idea of their dimensions.

Comment 172

5 years ago
We may not take this if the risk is too high.
It's not as simple as just giving layers a position and a size. For example in  http://bradfrostweb.com/blog/mobile/fixed-position/ there is just a single fixed-position layer containing both the header and footer. I don't know how you're planning to use a position and size, but it's unlikely to work for a layer like that.
Created attachment 620019 [details] [diff] [review]
improve definition of the "fixed layer" flag

Patch with comments fixed.
Attachment #618899 - Attachment is obsolete: true
Attachment #620019 - Flags: review+
These should land once the tree is open.
(Assignee)

Comment 176

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #173)
> It's not as simple as just giving layers a position and a size. For example
> in  http://bradfrostweb.com/blog/mobile/fixed-position/ there is just a
> single fixed-position layer containing both the header and footer. I don't
> know how you're planning to use a position and size, but it's unlikely to
> work for a layer like that.

Ah, ok - then I guess we need for fixed position elements (perhaps only those that are children to a content node with a displayport?) to always get their own layers - is this feasible? Would then adding a position/size make sense?
Yes, we could do that.

What you want is not exactly a position and size. I think the simplest thing that would handle the cases you care about would be to specify a point in the layer's coordinate space, and horizontal and vertical values for "fraction across the CSS viewport where this point should be anchored".
(Assignee)

Comment 178

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #177)
> Yes, we could do that.
> 
> What you want is not exactly a position and size. I think the simplest thing
> that would handle the cases you care about would be to specify a point in
> the layer's coordinate space, and horizontal and vertical values for
> "fraction across the CSS viewport where this point should be anchored".

What would this point be? Could you elaborate a little?
Examples:
-- element with top edge pinned to the top of the viewport: anchorPoint.y = 0, fraction-across-viewport.y = 0
-- element with bottom edge pinned to the bottom of the viewport: anchorPoint.y = 50 (assuming element is 50 layer pixels tall), fraction-across-viewport.y = 1
-- element with top edge at 70px from the top of the CSS viewport: anchorPoint.y = 0, fraction-across-viewport = 70/current-viewport-height
Does this have patches that should land now that the tree is open?
(Assignee)

Comment 181

5 years ago
Yes, these last two patches should land now - roc, are you planning on pushing your patch?
I can push them both I guess

Updated

5 years ago
Blocks: 751685
https://hg.mozilla.org/integration/mozilla-inbound/rev/82b7beb19194
https://hg.mozilla.org/integration/mozilla-inbound/rev/d2824e2868a6
Backed out due to build bustage (mine)
https://hg.mozilla.org/integration/mozilla-inbound/rev/ba39b0822992
https://hg.mozilla.org/integration/mozilla-inbound/rev/5c35cd6f3ea4
Sorry, after speaking with cwiis on #developers, have backed this out for native Android R1 failures:

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=5c35cd6f3ea4
https://tbpl.mozilla.org/php/getParsedLog.php?id=11463747&tree=Mozilla-Inbound

Note:
The first failure (background-size-zoom-repeat.html) was due to bug 750598 that was backed out after this push; so can be ignored.

This leaves:
{
REFTEST TEST-UNEXPECTED-FAIL | http://10.250.48.210:30070/tests/layout/reftests/border-radius/clipping-4-canvas.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | http://10.250.48.210:30070/tests/layout/reftests/border-radius/clipping-5-refc.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | http://10.250.48.210:30070/tests/layout/reftests/border-radius/intersecting-clipping-1-refc.html | image comparison (==) 
}

https://hg.mozilla.org/integration/mozilla-inbound/rev/32a8564667a5
https://hg.mozilla.org/mozilla-central/rev/ba39b0822992
https://hg.mozilla.org/mozilla-central/rev/5c35cd6f3ea4
Status: REOPENED → RESOLVED
Last Resolved: 6 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
Backed out: https://hg.mozilla.org/mozilla-central/rev/32a8564667a5
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Blocks: 751983
(Assignee)

Comment 189

5 years ago
I had a look at the failure, and it seems there's a general failure with border-radius on a canvas, with and without this patch. I'm not sure why the reconcilliation patch makes this glitch more common, but testing in the browser, the error is very visible.

I put together two of the ref-tests here: http://chrislord.net/files/mozilla/clipping.html

This page renders mostly correct at zoom level 1.0, but any other zoom level shows very visible errors.

I wonder if this is a general case of masks on layers (or tiled layers?) not working correctly, and the reference is correct because it doesn't get split out into a separate layer?
Target Milestone: mozilla15 → ---
Yeah, I'm seeing some similar problems with the 3rd iframe (with border-radius) here: http://people.mozilla.org/~mwargers/tests/layout/white_stripes_scrolling_iframe.html
(Assignee)

Comment 191

5 years ago
Seeing how the mask is used in TiledThebesLayerOGL, it seems that the same mask is applied to each tile - Will open a separate bug, blocking this.
(Assignee)

Updated

5 years ago
Depends on: 753784
Blocks: 753972
Blocks: 753974
This is all Nick...
Blocks: 754543
Assignee: romaxa → ncameron
(Assignee)

Comment 193

5 years ago
Even after applying Nick's patches in bug 753784 (which I've tested, and they work), I still get reftest failures after applying the reconcilliation patch (I assume because it also enables MOZ_FIXED_POSITION_LAYERS, I'm just waiting for the try run to finish to confirm).

The failures can be seen here: https://tbpl.mozilla.org/?tree=Try&rev=aacf9d068a5d

Loading the same pages in a browser, I can't get the same glitches to manifest, so I'm not sure what this could be yet. Currently reading through the layout code that'd be affected by enabling fixed position layers.
(Assignee)

Comment 194

5 years ago
https://tbpl.mozilla.org/?tree=Try&rev=512b9e68a5c7 - This is the same build as the one that fails ref-tests, but without MOZ_FIXED_POSITION_LAYERS set. Given that reftests are captured on the content side, this precludes the code in CompositorParent from being the culprit, leaving just layout code.

I'm particularly dubious of this line: http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp#146 which will be triggred on xul-fennec, but not on native fennec, but I've not finished running through the code yet.

I'm currently doing a xul-fennec PC build (to be followed by a firefox build too) to see if I can manufacture a situation where I can reproduce this failure. I'll be trying to poke various things in the meantime, but it's slow going until I can reproduce this locally.
(Assignee)

Comment 195

5 years ago
(In reply to Chris Lord [:cwiiis] from comment #194)
> I'm particularly dubious of this line:
> http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.
> cpp#146 which will be triggred on xul-fennec, but not on native fennec, but
> I've not finished running through the code yet.

A try build just commenting out the IsFixedFrame clause shows the same error, so I guess this isn't what's causing it.

(for reference: https://tbpl.mozilla.org/?tree=Try&rev=96788ffa7c4b)
(Assignee)

Comment 196

5 years ago
Trying the test in xul-fennec on PC with MOZ_ENABLE_FIXED_POSITION_LAYERS=1, I see a brief frame with the incorrect-looking test, but then it redraws almost instantly. This is promising though, at least I can quickly test it now. It definitely only seems to happen when fixed position layers are enabled.
(Assignee)

Comment 197

5 years ago
Altering things so that MOZ_ENABLE_FIXED_POSITION_LAYERS only gets applied in ShadowLayersParent and not in nsDisplayList still shows the problem - this leads me to think that somewhere in layers may be handling fixed position layers incorrectly... (and if this is the case, apologies to layout for pointing the finger so quickly!)

Updated

5 years ago
Duplicate of this bug: 757210
(Assignee)

Comment 199

5 years ago
So I wasn't too quick - the problem appears to be with the reverse-translation in layout/ipc/RenderFrameParent.cpp (http://mxr.mozilla.org/mozilla-central/source/layout/ipc/RenderFrameParent.cpp#279)

To me, that translation of the clip rect looks suspect... Shouldn't it be translated in the same way that shadowTransform is translated in ReverseTranslate? It seems to me that it'll end up having its transform applied twice this way, which going on the reftest results, appears to be what's happening.
(Assignee)

Comment 200

5 years ago
And another quick note, if reftests actually took the same rendering path as the browser, this bug wouldn't manifest in the same way in native fennec (but would still manifest, as I forget to transform the shadow clip rect in my patch...)

Comment 201

5 years ago
(In reply to Chris Lord [:cwiiis] from comment #200)
> And another quick note, if reftests actually took the same rendering path as
> the browser

Note that this is being fixed in Bug 748088.
(Assignee)

Comment 202

5 years ago
Created attachment 626099 [details] [diff] [review]
Fix reverse translation of shadow layer clip rects

This seems to fix the reftest failures for me locally, I've also pushed to try to confirm: https://tbpl.mozilla.org/?tree=Try&rev=0eb43778187f

Being optimistic that it will work and requesting review now.
Attachment #626099 - Flags: review?(ajuma)

Updated

5 years ago
Attachment #626099 - Flags: review?(ajuma) → review+
(Assignee)

Comment 203

5 years ago
Created attachment 626134 [details] [diff] [review]
Reconcile async scrolling on native android fennec

Rebased and revised to include the clip rects (which I'd missed out before). Re-requesting as it's a slightly non-trivial change.
Attachment #619666 - Attachment is obsolete: true
Attachment #626134 - Flags: review?(ajuma)

Updated

5 years ago
Attachment #626134 - Flags: review?(ajuma) → review+
I think I've fixed my bit, un-assigning. Happy to help out if needed though.
Assignee: ncameron → chrislord.net
(Assignee)

Comment 205

5 years ago
Pushed:

http://hg.mozilla.org/integration/mozilla-inbound/rev/4c57c1044420
http://hg.mozilla.org/integration/mozilla-inbound/rev/d1a44d2ec5c3
http://hg.mozilla.org/integration/mozilla-inbound/rev/1fa2ea598652

But forgot to put r=ajuma on the middle commit, so we'll see...
(Assignee)

Comment 206

5 years ago
Backed out middle commit, re-landed as http://hg.mozilla.org/integration/mozilla-inbound/rev/8fd7f0d0c654
Duplicate of this bug: 756713
Whiteboard: [fennec-4.1?] → [fennec-4.1?][layout]
https://hg.mozilla.org/mozilla-central/rev/4c57c1044420
https://hg.mozilla.org/mozilla-central/rev/1fa2ea598652
https://hg.mozilla.org/mozilla-central/rev/8fd7f0d0c654
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15

Updated

5 years ago
Whiteboard: [fennec-4.1?][layout] → [fennec-4.1?][layout][gfx.relnote.15]
I thought this would fix bug 705506 (the lagging position: fixed divs while scrolling bug), but it doesn't seem like it, afaict. Shouldn't it fix that bug?
(Assignee)

Comment 210

5 years ago
(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #209)
> I thought this would fix bug 705506 (the lagging position: fixed divs while
> scrolling bug), but it doesn't seem like it, afaict. Shouldn't it fix that
> bug?

It should, and it does - but perhaps it doesn't work in every case. Verify that you've tested with these patches applied and any sites that don't work should be filed as bugs. For reference, you can see it working on http://chrislord.net/files/mozilla/fixed.html
What do you mean "with these patches applied"? This bug is marked fixed, doesn't it mean that the patches are in current trunk build?
Fwiw, I'm also seeing these lags on http://chrislord.net/files/mozilla/fixed.html
(Assignee)

Comment 212

5 years ago
(In reply to Martijn Wargers [:mw22] (QA - IRC nick: mw22) from comment #211)
> What do you mean "with these patches applied"? This bug is marked fixed,
> doesn't it mean that the patches are in current trunk build?
> Fwiw, I'm also seeing these lags on
> http://chrislord.net/files/mozilla/fixed.html

Argh, so just looked and a part of the patch is missing... It must've got lost in a rebase or something - will push to inbound...
(Assignee)

Comment 213

5 years ago
Pushed the rest to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/24f3dd5529bb
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Build bustage on all platforms:
gfx/layers/ipc/CompositorParent.cpp:402:51: error: use of undeclared identifier 'mContentSize'; did you mean 'mContentRect'?
  int offsetX = NS_MAX(0, NS_MIN(mScrollOffset.x, mContentSize.width - mWidgetSize.width));
                                                  ^~~~~~~~~~~~
                                                  mContentRect

I followed clang's advice and changed mContentSize => mContentRect
https://hg.mozilla.org/integration/mozilla-inbound/rev/d8e64c2312d3

Please confirm it's the correct fix.
My mistake, I clobbered that code accidentally when I rebased the patch to convert the metrics pointer to a reference. The fix mats applied is correct in that the code now reflects what was there originally. However, looking at it again - should that MIN/MAX take into accounts the left/right bounds of the mContentRect rather than assuming it has a min value of 0? I should have paid more attention to that hunk when I rebased the RTL patch...
(Assignee)

Comment 216

5 years ago
(In reply to Kartikaya Gupta (:kats) from comment #215)
> My mistake, I clobbered that code accidentally when I rebased the patch to
> convert the metrics pointer to a reference. The fix mats applied is correct
> in that the code now reflects what was there originally. However, looking at
> it again - should that MIN/MAX take into accounts the left/right bounds of
> the mContentRect rather than assuming it has a min value of 0? I should have
> paid more attention to that hunk when I rebased the RTL patch...

I think you're right, it'd be easy to test if this is the case by trying fixed position elements on an RTL page. Can I leave this fix down to you?
Yup, I'll upload a patch.
(In reply to Chris Lord [:cwiiis] from comment #213)
> Pushed the rest to inbound:
> http://hg.mozilla.org/integration/mozilla-inbound/rev/24f3dd5529bb

https://hg.mozilla.org/mozilla-central/rev/24f3dd5529bb

(In reply to Mats Palmgren [:mats] from comment #214)
> I followed clang's advice and changed mContentSize => mContentRect
> https://hg.mozilla.org/integration/mozilla-inbound/rev/d8e64c2312d3

https://hg.mozilla.org/mozilla-central/rev/d8e64c2312d3
Created attachment 627683 [details] [diff] [review]
Update fixed position code for RTL pages

Tested using http://people.mozilla.org/~kgupta/bug/607417.html as a test page. My local build is hosed so I built it via tryserver at https://tbpl.mozilla.org/?tree=Try&rev=b30a8a579819

There seems to be an issue on the test page though where the background of the fixed position layer will paint either as black or white, but that was before my patch too so I think it's an unrelated bug. I can file a new bug for that.
Attachment #627683 - Flags: review?(chrislord.net)
Comment on attachment 627683 [details] [diff] [review]
Update fixed position code for RTL pages

Cwiiis is on vacation, so requesting the review from ajuma instead
Attachment #627683 - Flags: review?(chrislord.net) → review?(ajuma)

Updated

5 years ago
Attachment #627683 - Flags: review?(ajuma) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e6ad9e6c6eb
Depends on: 759389
Should be in http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/mozilla-inbound-android/1338327891/
There's some bad rerendering issues using martijn's test page : http://bit.ly/L0NSsN
using the inbound build in comment 222.  Zooming, running the test then panning around could cause the background to be off from where it is suppose to be located.

It seems to work with some other simplier tests just fine: 
http://www.w3schools.com/cssref/pr_class_position.asp
https://hg.mozilla.org/mozilla-central/rev/1e6ad9e6c6eb
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
tracking-fennec: + → 15+
blocking-fennec1.0: + → -
letting this ride the trains for fx15
(In reply to Chris Lord [:cwiiis] from comment #167)
> After some discussion on IRC, I agree with this behaviour (which is as
> suggested, with slight alterations):
> - Respect user-scalable viewport tag (bug 707571, blocker)
> - Fix to the larger of the screen or the CSS viewport
> 
> I think with roc's suggestion of having a function to set a rect for fixed
> position elements to be relative to should make this implementable - we'd
> also need to alter the scroll/zoom code in browser.js somewhat.

This hasn't been implemented yet, right? Can somebody summarize what the current expected behaviour is for fixed-position elements? I read through this bug a couple of times but am unsure if (with the code currently in m-c) the fixed-position elements are supposed to be fixed to the screen, CSS viewport, the larger of the two, or something else. This will determine if bug 757362 is actually a bug at all, and/or what the correct fix is.
I believe currently fixed-pos elements are fixed to the CSS viewport.
(Assignee)

Comment 228

5 years ago
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #227)
> I believe currently fixed-pos elements are fixed to the CSS viewport.

This is correct - and the CSS viewport is always fixed to the top-left, so zooming in can make fixed-position elements inaccessible.

This will need to be fixed on both the layout side (for us to be able to set a rect for fixed position elements to layout to) and the compositor side (to handle async zooming correctly), solutions for which have been detailed in previous comments.

It looks like we're tracking this in bug 760805 now, so I'll add some details there.
You need to log in before you can comment on or make changes to this bug.