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.

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.

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. :)

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.

Comment on attachment 491800 [details] [diff] [review] API part I think feedback is better here

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.

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)

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.

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

bug 605618 landed a while ago, any update here?

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?

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.

Feels like trick with this transformation is wrong... fixed element still moving on yahoo.com/sign in...

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..

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.

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

sounds like fixed layer is not create or not marked when we have just position:fixed div without any "text" content

also works for http://meyerweb.com/eric/css/edge/complexspiral/demo.html now

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?

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]

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

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*)".

introduces GetActiveScrolledRootFor(item, builder) in order to share active scrolled root selection code

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

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

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.

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

Rename method to PosFixed. Fixed copy/paste typo bug return value float->bool

Set/GetIsPosFixed should be "Set/GetIsFixedPosition".

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?

updated after Part 1 renamings

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

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.

> 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

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

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

Ok, this seems to work, but required to disable reverse translation for fixed frames which are having parent shadow container with fixed position

Fixed rejects

+ return nsLayoutUtils::IsProperAncestorFrame(rootScrollFrame, aActiveScrolledRoot); Not "Proper", as we discussed. + nsIFrame* f = item->GetUnderlyingFrame(); + nsIFrame* activeScrolledRoot = + nsLayoutUtils::GetActiveScrolledRootFor(f, mBuilder->ReferenceFrame()); GetActiveScrolledRootFor(item)

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

Fixed comments

(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

moved GetActiveScrolledRootFor(item, builder) and ScrolledByViewportScrolling(activeScrolledRoot, builder) to nsLayoutUtils API I guess it's safe to move r+ from v7 here

fixed dp.ru visibility I guess it's safe to move r+ from v5.1 here

Comment on attachment 523286 [details] [diff] [review] Part1: Fixed layers detection (v8) Need r, new layout API moved into this patch

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

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?

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.

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?

> 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

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!

You need to set HasFixedItems if we construct a background-attachment:fixed nsDisplayBackground item.

Try build with latest patches, tested on android - works great: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/romaxa@gmail.com-cf00d3e22402/

BTW we definitely *have* to have some tests here. reftests can test displayport.

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!

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

Comment on attachment 524288 [details] [diff] [review] Reftest for this feature not sure if David around now, maybe Chris can look at this reftest?

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.

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

This feature disabled in bug 656167, so we need to reopen this bug

also see bug 668698

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.

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.

Not sure who should review this

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

Enabled test back

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

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.

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.

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.

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

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.

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.

For reference, I find http://bradfrostweb.com/blog/mobile/fixed-position/ to be a great site to test this.

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.

Ok, OMTC and IPC layers tree looks different and something get confused and setted up Fixed flags not correctly

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.

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)

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

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

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?

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

Try this.

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? :)

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

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?

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.

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?

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

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

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

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

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

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

(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 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"?

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

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

Addresses review comments.

Comment on attachment 619666 [details] [diff] [review] Reconcile async scrolling in CompositorParent Looks good to me.

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.

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

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.

Patch with comments fixed.

These should land once the tree is open.

(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".

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

Yes, these last two patches should land now - roc, are you planning on pushing your patch?

I can push them both I guess

https://hg.mozilla.org/integration/mozilla-inbound/rev/82b7beb19194 https://hg.mozilla.org/integration/mozilla-inbound/rev/d2824e2868a6