Last Comment Bug 607417 - Reconcile position:fixed with async scrolling and displayport+meta-viewport
: Reconcile position:fixed with async scrolling and displayport+meta-viewport
Status: RESOLVED FIXED
[fennec-4.1?][layout][gfx.relnote.15]
: mobile
Product: Core
Classification: Components
Component: Layout (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Chris Lord [:cwiiis]
:
: Jet Villegas (:jet)
Mentors:
: 500700 500732 601832 628648 637967 673724 691074 743447 748641 756713 757210 (view as bug list)
Depends on: 656036 650759 653133 656114 656167 668698 699262 701190 749357 753784 759389
Blocks: 616348 751685 751983 753972 753974 754543 628648 705506
  Show dependency treegraph
 
Reported: 2010-10-26 12:31 PDT by Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
Modified: 2012-06-06 06:45 PDT (History)
35 users (show)
roc: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
15+


Attachments
simple testcase (1.44 KB, text/html)
2010-10-26 23:20 PDT, Oleg Romashin (:romaxa)
no flags Details
API part (5.69 KB, patch)
2010-11-19 02:06 PST, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Mark all available layers as fixed when it's parent viewport (7.27 KB, patch)
2010-11-19 06:15 PST, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Hackish handling Fixed layer (2.01 KB, patch)
2010-11-19 11:17 PST, Oleg Romashin (:romaxa)
roc: feedback-
Details | Diff | Splinter Review
Mark all available layers as fixed when it's parent viewport v2 (colorLayer) (12.22 KB, patch)
2010-11-21 11:59 PST, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Part1: Fixed layers detection part, updated to tip (12.97 KB, patch)
2011-02-23 20:02 PST, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Quick fixed layers suspender (2.21 KB, patch)
2011-02-24 17:45 PST, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Reverse translate in RenderFrameParent, make it mostly work (2.30 KB, patch)
2011-02-26 08:47 PST, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
DisplayList tricks WIP (3.64 KB, patch)
2011-03-18 20:11 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Heater simple testcase (1.30 KB, text/html)
2011-03-18 20:14 PDT, Oleg Romashin (:romaxa)
no flags Details
dp.ru - simple testcase (531 bytes, text/html)
2011-03-19 16:54 PDT, Oleg Romashin (:romaxa)
no flags Details
More simple testcase dp.ru (416 bytes, text/html)
2011-03-19 17:16 PDT, Oleg Romashin (:romaxa)
no flags Details
DisplayList tricks (3.72 KB, patch)
2011-03-22 06:35 PDT, Tatiana Meshkova (:tatiana)
no flags Details | Diff | Splinter Review
Part3: DisplayList tricks (4.06 KB, patch)
2011-03-22 09:08 PDT, Tatiana Meshkova (:tatiana)
no flags Details | Diff | Splinter Review
Part 2: Reverse translate in RenderFrameParent (2.38 KB, patch)
2011-03-24 08:44 PDT, Tatiana Meshkova (:tatiana)
no flags Details | Diff | Splinter Review
Part1: Fixed layers detection part, checks if active scrolled root is equal to builder reference frame (8.29 KB, patch)
2011-03-27 07:17 PDT, Tatiana Meshkova (:tatiana)
roc: review+
Details | Diff | Splinter Review
Part 3: DisplayList tricks (13.26 KB, patch)
2011-03-27 07:25 PDT, Tatiana Meshkova (:tatiana)
no flags Details | Diff | Splinter Review
Screenshot1-fennec-long-scroll_footer-is-missing-without-MarkOutOfFlowFrameForDisplay-tricks.png (25.88 KB, image/png)
2011-03-27 07:48 PDT, Tatiana Meshkova (:tatiana)
no flags Details
Part 3: DisplayList tricks (v4) (13.42 KB, patch)
2011-03-27 08:17 PDT, Tatiana Meshkova (:tatiana)
no flags Details | Diff | Splinter Review
Fixed layers detection part (8.50 KB, patch)
2011-03-27 20:51 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Part 1: Fixed layers detection part, FixedPosition name (v5) (8.59 KB, patch)
2011-03-27 22:06 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Part 2: Reverse translate in RenderFrameParent (v5) (2.32 KB, patch)
2011-03-28 03:20 PDT, Tatiana Meshkova (:tatiana)
no flags Details | Diff | Splinter Review
Part 3: Visibility tricks (v5) (12.09 KB, patch)
2011-03-28 03:22 PDT, Tatiana Meshkova (:tatiana)
no flags Details | Diff | Splinter Review
Part 2: Reverse translate in RenderFrameParent (v6) (2.31 KB, patch)
2011-03-28 19:58 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
One more test to fix (1.64 KB, patch)
2011-03-28 22:25 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Part1, Fixed layers detection, Fixed dp.ru case completely (8.96 KB, patch)
2011-03-29 01:12 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Old version vs new version LayersLOG (6.03 KB, text/plain)
2011-03-29 21:50 PDT, Oleg Romashin (:romaxa)
no flags Details
Part1, Fixed layers detection, v6 (6.85 KB, patch)
2011-03-30 16:23 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Part 2: Reverse translate in RenderFrameParent (v7) (2.36 KB, patch)
2011-03-30 16:24 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Part 3: Visibility tricks (v5.1) (9.25 KB, patch)
2011-03-30 16:25 PDT, Oleg Romashin (:romaxa)
roc: review+
Details | Diff | Splinter Review
Part1, Fixed layers detection, v7 (10.92 KB, patch)
2011-03-30 23:40 PDT, Oleg Romashin (:romaxa)
roc: review+
Details | Diff | Splinter Review
Part1: Fixed layers detection (v8) (17.90 KB, patch)
2011-03-31 05:43 PDT, Tatiana Meshkova (:tatiana)
roc: review+
Details | Diff | Splinter Review
Part 3: Visibility tricks (v6) (3.67 KB, patch)
2011-03-31 05:45 PDT, Tatiana Meshkova (:tatiana)
roc: review-
Details | Diff | Splinter Review
Part 2: Reverse translate in RenderFrameParent (v8) (5.24 KB, patch)
2011-03-31 21:47 PDT, Oleg Romashin (:romaxa)
ben: review+
Details | Diff | Splinter Review
Part 3: Visibility tricks (v7) (10.54 KB, patch)
2011-04-03 05:34 PDT, Tatiana Meshkova (:tatiana)
no flags Details | Diff | Splinter Review
Part 3: Visibility tricks (v8) (10.56 KB, patch)
2011-04-04 01:52 PDT, Tatiana Meshkova (:tatiana)
roc: review+
Details | Diff | Splinter Review
Part1: Fixed layers detection (v8) (latest mc tip rejects - safe) (18.01 KB, patch)
2011-04-05 23:24 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Reftest for this feature (2.73 KB, patch)
2011-04-06 15:34 PDT, Oleg Romashin (:romaxa)
roc: review+
cjones.bugs: review+
Details | Diff | Splinter Review
Enable fixed position content support for async scrolling (2.45 KB, patch)
2011-10-19 17:22 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Enable fixed position content support for async scrolling (3.72 KB, patch)
2011-10-19 21:05 PDT, Oleg Romashin (:romaxa)
roc: review+
Details | Diff | Splinter Review
Reconcile async scrolling on native android fennec (6.70 KB, patch)
2012-04-20 11:19 PDT, Chris Lord [:cwiiis]
romaxa: feedback+
Details | Diff | Splinter Review
OMTC embedding with MOZ_ENABLE_FIXED_POSITION_LAYERS=1 (15.19 KB, text/plain)
2012-04-20 11:37 PDT, Oleg Romashin (:romaxa)
no flags Details
OMTC vs IPC layers log tree (2.08 KB, text/plain)
2012-04-20 16:26 PDT, Oleg Romashin (:romaxa)
no flags Details
fix (10.97 KB, patch)
2012-04-26 19:23 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
tnikkel: review+
Details | Diff | Splinter Review
Reconcile async scrolling on native android fennec (5.33 KB, patch)
2012-04-27 13:15 PDT, Chris Lord [:cwiiis]
ajuma.bugzilla: review-
roc: feedback+
Details | Diff | Splinter Review
Reconcile async scrolling in CompositorParent (6.77 KB, patch)
2012-04-30 13:31 PDT, Chris Lord [:cwiiis]
ajuma.bugzilla: review+
Details | Diff | Splinter Review
improve definition of the "fixed layer" flag (11.10 KB, patch)
2012-05-01 12:27 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
roc: review+
Details | Diff | Splinter Review
Fix reverse translation of shadow layer clip rects (2.68 KB, patch)
2012-05-22 11:29 PDT, Chris Lord [:cwiiis]
ajuma.bugzilla: review+
Details | Diff | Splinter Review
Reconcile async scrolling on native android fennec (6.78 KB, patch)
2012-05-22 12:43 PDT, Chris Lord [:cwiiis]
ajuma.bugzilla: review+
Details | Diff | Splinter Review
Update fixed position code for RTL pages (1.93 KB, patch)
2012-05-28 06:25 PDT, away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com)
ajuma.bugzilla: review+
Details | Diff | Splinter Review

Description Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-10-26 12:31:59 PDT
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.
Comment 1 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2010-10-26 12:32:15 PDT
I think this should block final.
Comment 2 Pekka Vanhoja 2010-10-26 22:14:39 PDT
CC'ing myself.
Comment 3 Benjamin Stover (:stechz) 2010-10-26 22:31:41 PDT
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.
Comment 4 Oleg Romashin (:romaxa) 2010-10-26 23:20:40 PDT
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
Comment 5 Matt Brubeck (:mbrubeck) 2010-10-27 07:08:03 PDT
(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
Comment 6 Benjamin Stover (:stechz) 2010-10-27 08:18:02 PDT
> 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 Pekka Vanhoja 2010-10-29 05:29:43 PDT
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?
Comment 8 Benjamin Stover (:stechz) 2010-10-29 08:33:51 PDT
(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.
Comment 9 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-11-10 14:34:23 PST
(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
Comment 10 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-11-10 14:44:36 PST
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.
Comment 11 Benjamin Stover (:stechz) 2010-11-10 14:57:38 PST
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 12 Oleg Romashin (:romaxa) 2010-11-19 02:06:58 PST
Created attachment 491800 [details] [diff] [review]
API part
Comment 13 Oleg Romashin (:romaxa) 2010-11-19 03:04:39 PST
Comment on attachment 491800 [details] [diff] [review]
API part

I think feedback is better here
Comment 14 Oleg Romashin (:romaxa) 2010-11-19 05:24:07 PST
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.
Comment 15 Oleg Romashin (:romaxa) 2010-11-19 06:15:49 PST
Created attachment 491821 [details] [diff] [review]
Mark all available layers as fixed when it's parent viewport
Comment 16 Oleg Romashin (:romaxa) 2010-11-19 11:17:11 PST
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)
Comment 17 Oleg Romashin (:romaxa) 2010-11-21 11:59:10 PST
Created attachment 492206 [details] [diff] [review]
Mark all available layers as fixed when it's parent viewport v2 (colorLayer)
Comment 18 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-11-24 13:56:05 PST
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 19 Robert O'Callahan (:roc) (email my personal email if necessary) 2010-11-24 13:59:21 PST
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.
Comment 20 Oleg Romashin (:romaxa) 2010-11-24 14:59:53 PST
> 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
Comment 21 Brad Lassey [:blassey] (use needinfo?) 2011-02-18 08:23:51 PST
bug 605618 landed a while ago, any update here?
Comment 22 Brad Lassey [:blassey] (use needinfo?) 2011-02-23 13:59:18 PST
*** Bug 628648 has been marked as a duplicate of this bug. ***
Comment 23 Oleg Romashin (:romaxa) 2011-02-23 20:02:05 PST
Created attachment 514718 [details] [diff] [review]
Part1: Fixed layers detection part, updated to tip
Comment 24 Oleg Romashin (:romaxa) 2011-02-23 21:55:33 PST
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?
Comment 25 Oleg Romashin (:romaxa) 2011-02-24 17:45:29 PST
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.
Comment 26 Oleg Romashin (:romaxa) 2011-02-24 17:49:50 PST
Feels like trick with this transformation is wrong... fixed element still moving on yahoo.com/sign in...
Comment 27 Oleg Romashin (:romaxa) 2011-02-26 08:47:08 PST
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..
Comment 28 Oleg Romashin (:romaxa) 2011-02-26 10:49:51 PST
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
Comment 29 Doug Turner (:dougt) 2011-03-01 14:22:38 PST
Love to have, not blocking.  Renom if you disagree.
Comment 30 Mark Finkle (:mfinkle) (use needinfo?) 2011-03-01 18:22:11 PST
*** Bug 637967 has been marked as a duplicate of this bug. ***
Comment 31 Doug Turner (:dougt) 2011-03-16 12:07:30 PDT
*** Bug 500732 has been marked as a duplicate of this bug. ***
Comment 32 Oleg Romashin (:romaxa) 2011-03-18 20:11:48 PDT
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.
Comment 33 Oleg Romashin (:romaxa) 2011-03-18 20:14:06 PDT
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
Comment 34 Oleg Romashin (:romaxa) 2011-03-18 20:14:29 PDT
Created attachment 520386 [details]
Heater simple testcase
Comment 35 Oleg Romashin (:romaxa) 2011-03-19 16:54:29 PDT
Created attachment 520479 [details]
dp.ru - simple testcase
Comment 36 Oleg Romashin (:romaxa) 2011-03-19 17:16:39 PDT
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
Comment 37 Tatiana Meshkova (:tatiana) 2011-03-22 06:35:44 PDT
Created attachment 520903 [details] [diff] [review]
DisplayList tricks

also works for http://meyerweb.com/eric/css/edge/complexspiral/demo.html now
Comment 38 Tatiana Meshkova (:tatiana) 2011-03-22 09:08:32 PDT
Created attachment 520929 [details] [diff] [review]
Part3: DisplayList tricks
Comment 39 Oleg Romashin (:romaxa) 2011-03-23 12:52:45 PDT
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?
Comment 40 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-23 15:10:52 PDT
What exactly are you trying to do here? Why isn't normal display list construction and visibility computation working?
Comment 41 Oleg Romashin (:romaxa) 2011-03-23 16:24:44 PDT
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.
Comment 42 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-23 19:33:49 PDT
Yeah, I meant the third patch.
Comment 43 Tatiana Meshkova (:tatiana) 2011-03-24 05:50:41 PDT
(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]
Comment 44 Tatiana Meshkova (:tatiana) 2011-03-24 08:44:11 PDT
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.
Comment 45 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-24 16:41:07 PDT
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*)".
Comment 46 Tatiana Meshkova (:tatiana) 2011-03-27 07:17:44 PDT
Created attachment 522207 [details] [diff] [review]
Part1: Fixed layers detection part, checks if active scrolled root is equal to builder reference frame
Comment 47 Tatiana Meshkova (:tatiana) 2011-03-27 07:25:40 PDT
Created attachment 522208 [details] [diff] [review]
Part 3: DisplayList tricks

introduces GetActiveScrolledRootFor(item, builder) in order to share active scrolled root selection code
Comment 48 Tatiana Meshkova (:tatiana) 2011-03-27 07:48:36 PDT
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)
Comment 49 Tatiana Meshkova (:tatiana) 2011-03-27 08:17:16 PDT
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
Comment 50 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-27 15:52:44 PDT
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.
Comment 51 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-27 15:53:53 PDT
(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.
Comment 52 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-27 16:14:45 PDT
+  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...
Comment 53 Oleg Romashin (:romaxa) 2011-03-27 20:51:22 PDT
Created attachment 522280 [details] [diff] [review]
Fixed layers detection part

Rename method to PosFixed.
Fixed copy/paste typo bug return value float->bool
Comment 54 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-27 20:55:07 PDT
Set/GetIsPosFixed should be "Set/GetIsFixedPosition".
Comment 55 Oleg Romashin (:romaxa) 2011-03-27 22:06:58 PDT
Created attachment 522296 [details] [diff] [review]
Part 1: Fixed layers detection part, FixedPosition name (v5)
Comment 56 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-27 23:18:55 PDT
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?
Comment 57 Tatiana Meshkova (:tatiana) 2011-03-28 03:20:06 PDT
Created attachment 522325 [details] [diff] [review]
Part 2: Reverse translate in RenderFrameParent (v5)

updated after Part 1 renamings
Comment 58 Tatiana Meshkova (:tatiana) 2011-03-28 03:22:18 PDT
Created attachment 522326 [details] [diff] [review]
Part 3: Visibility tricks (v5)
Comment 59 Oleg Romashin (:romaxa) 2011-03-28 19:58:55 PDT
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
Comment 60 Oleg Romashin (:romaxa) 2011-03-28 22:25:42 PDT
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.
Comment 61 Oleg Romashin (:romaxa) 2011-03-29 01:12:54 PDT
Created attachment 522616 [details] [diff] [review]
Part1, Fixed layers detection, Fixed dp.ru case completely
Comment 62 Oleg Romashin (:romaxa) 2011-03-29 08:26:33 PDT
> 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
Comment 63 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-29 15:38:01 PDT
(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.
Comment 64 Oleg Romashin (:romaxa) 2011-03-29 17:02:49 PDT
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
Comment 65 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-29 20:59:40 PDT
In that case, shouldn't we be marking BasicShadowContainerLayer(2) as fixed? Then the RenderFrameParent code can do the right thing.
Comment 66 Oleg Romashin (:romaxa) 2011-03-29 21:50:16 PDT
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)
Comment 67 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-30 02:33:38 PDT
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()));
Comment 68 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-30 13:36:20 PDT
Actually I think ScrolledByViewportScrolling should take an aBuilder parameter and be

  nsIFrame* rootScrollFrame = aBuilder->ReferenceFrame()->PresContext()->GetPresShell()->GetRootScrollFrame();
  return nsLayoutUtils::IsAncestorFrame(aActiveScrolledRoot, rootScrollFrame);
Comment 69 Oleg Romashin (:romaxa) 2011-03-30 16:23:04 PDT
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
Comment 70 Oleg Romashin (:romaxa) 2011-03-30 16:24:03 PDT
Created attachment 523151 [details] [diff] [review]
Part 2: Reverse translate in RenderFrameParent (v7)
Comment 71 Oleg Romashin (:romaxa) 2011-03-30 16:25:35 PDT
Created attachment 523152 [details] [diff] [review]
Part 3: Visibility tricks (v5.1)

Fixed rejects
Comment 72 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-30 20:23:31 PDT
+  return nsLayoutUtils::IsProperAncestorFrame(rootScrollFrame, aActiveScrolledRoot);

Not "Proper", as we discussed.

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

GetActiveScrolledRootFor(item)
Comment 73 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-30 20:28:21 PDT
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())
Comment 74 Oleg Romashin (:romaxa) 2011-03-30 23:40:02 PDT
Created attachment 523235 [details] [diff] [review]
Part1, Fixed layers detection, v7

Fixed comments
Comment 75 Tatiana Meshkova (:tatiana) 2011-03-31 05:40:37 PDT
(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
Comment 76 Tatiana Meshkova (:tatiana) 2011-03-31 05:43:31 PDT
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
Comment 77 Tatiana Meshkova (:tatiana) 2011-03-31 05:45:14 PDT
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
Comment 78 Oleg Romashin (:romaxa) 2011-03-31 11:53:59 PDT
Comment on attachment 523286 [details] [diff] [review]
Part1: Fixed layers detection (v8)

Need r, new layout API moved into this patch
Comment 79 Oleg Romashin (:romaxa) 2011-03-31 11:55:21 PDT
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 80 Benjamin Stover (:stechz) 2011-03-31 13:01:41 PDT
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 81 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-31 13:12:52 PDT
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.
Comment 82 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-03-31 13:18:44 PDT
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.
Comment 83 Oleg Romashin (:romaxa) 2011-03-31 15:35:44 PDT
> 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.
Comment 84 Benjamin Stover (:stechz) 2011-03-31 15:59:21 PDT
In that case, could you fix the other layers' clip rects as well?
Comment 85 Oleg Romashin (:romaxa) 2011-03-31 21:47:25 PDT
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
Comment 86 Benjamin Stover (:stechz) 2011-04-01 10:34:15 PDT
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!
Comment 87 Tatiana Meshkova (:tatiana) 2011-04-03 05:34:09 PDT
Created attachment 523863 [details] [diff] [review]
Part 3: Visibility tricks (v7)
Comment 88 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-03 15:25:52 PDT
You need to set HasFixedItems if we construct a background-attachment:fixed nsDisplayBackground item.
Comment 89 Tatiana Meshkova (:tatiana) 2011-04-04 01:52:15 PDT
Created attachment 523958 [details] [diff] [review]
Part 3: Visibility tricks (v8)
Comment 90 Oleg Romashin (:romaxa) 2011-04-05 08:57:16 PDT
Try build with latest patches, tested on android - works great:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/romaxa@gmail.com-cf00d3e22402/
Comment 91 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-04-05 23:16:19 PDT
BTW we definitely *have* to have some tests here. reftests can test displayport.
Comment 92 Oleg Romashin (:romaxa) 2011-04-05 23:24:06 PDT
Created attachment 524136 [details] [diff] [review]
Part1: Fixed layers detection (v8) (latest mc tip rejects - safe)
Comment 93 :Ehsan Akhgari 2011-04-06 11:17:29 PDT
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!
Comment 94 Oleg Romashin (:romaxa) 2011-04-06 13:41:59 PDT
(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?
Comment 95 Doug Turner (:dougt) 2011-04-06 13:46:35 PDT
oleg, check bug 648104.
Comment 96 Oleg Romashin (:romaxa) 2011-04-06 15:34:57 PDT
Created attachment 524288 [details] [diff] [review]
Reftest for this feature
Comment 97 Oleg Romashin (:romaxa) 2011-04-07 09:36:49 PDT
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 98 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-04-08 09:21:33 PDT
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.
Comment 99 Oleg Romashin (:romaxa) 2011-04-08 11:59:53 PDT
http://hg.mozilla.org/mozilla-central/rev/0f077c086750
Comment 100 Naoki Hirata :nhirata (please use needinfo instead of cc) 2011-04-22 15:21:22 PDT
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
Comment 101 Naoki Hirata :nhirata (please use needinfo instead of cc) 2011-04-22 15:22:24 PDT
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
Comment 102 Matt Brubeck (:mbrubeck) 2011-05-04 10:31:36 PDT
*** Bug 500700 has been marked as a duplicate of this bug. ***
Comment 103 Oleg Romashin (:romaxa) 2011-05-19 10:55:24 PDT
This feature disabled in bug 656167, so we need to reopen this bug
Comment 104 Martijn Wargers [:mwargers] (not working for Mozilla) 2011-07-24 04:32:24 PDT
*** Bug 673724 has been marked as a duplicate of this bug. ***
Comment 105 Naoki Hirata :nhirata (please use needinfo instead of cc) 2011-07-25 11:37:24 PDT
*** Bug 601832 has been marked as a duplicate of this bug. ***
Comment 106 Doug Turner (:dougt) 2011-07-28 10:19:36 PDT
also see bug 668698
Comment 107 David Baron :dbaron: ⌚️UTC-10 2011-10-06 10:29:39 PDT
*** Bug 691074 has been marked as a duplicate of this bug. ***
Comment 108 Jet Villegas (:jet) 2011-10-19 16:17:42 PDT
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 Asa Dotzler [:asa] 2011-10-19 16:54:16 PDT
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.
Comment 110 Oleg Romashin (:romaxa) 2011-10-19 17:22:07 PDT
Created attachment 568262 [details] [diff] [review]
Enable fixed position content support for async scrolling

Not sure who should review this
Comment 111 Tatiana Meshkova (:tatiana) 2011-10-19 18:03:55 PDT
(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?
Comment 112 Oleg Romashin (:romaxa) 2011-10-19 21:05:12 PDT
Created attachment 568292 [details] [diff] [review]
Enable fixed position content support for async scrolling

Enabled test back
Comment 113 Matt Brubeck (:mbrubeck) 2011-10-24 21:50:33 PDT
(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.
Comment 114 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-10-24 21:53:52 PDT
(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.
Comment 115 Oleg Romashin (:romaxa) 2011-10-24 23:44:25 PDT
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)
Comment 116 Oleg Romashin (:romaxa) 2011-10-25 08:34:27 PDT
try build with enable patch applied
https://tbpl.mozilla.org/?tree=Try&rev=1a9ee6935ae5
Comment 117 Oleg Romashin (:romaxa) 2011-10-25 08:36:15 PDT
BTw, IIUC problem with obscured content will be reproducible also with Native browser and FF browser running on small screen
Comment 118 Matt Brubeck (:mbrubeck) 2011-10-25 09:58:00 PDT
(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.
Comment 119 Oleg Romashin (:romaxa) 2011-12-30 11:27:51 PST
Ok, last important bugs has been fixed, can we enable it? Matt could you check latest builds?
Comment 120 Oleg Romashin (:romaxa) 2012-01-13 00:39:53 PST
Any updates, can we push fixed frames enabler?
Comment 121 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-01-13 01:26:15 PST
It's up to Matt.
Comment 122 Matt Brubeck (:mbrubeck) 2012-01-13 08:33:27 PST
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.
Comment 123 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2012-01-13 10:44:16 PST
The new fennec frontend will be using this code soon after its first release, and b2g will be using it in the near future.
Comment 124 Joe Drew (not getting mail) 2012-04-10 12:05:12 PDT
*** Bug 743447 has been marked as a duplicate of this bug. ***
Comment 125 Chris Lord [:cwiiis] 2012-04-19 08:24:41 PDT
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.
Comment 126 JP Rosevear [:jpr] 2012-04-19 11:42:06 PDT
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
Comment 127 Oleg Romashin (:romaxa) 2012-04-19 13:20:40 PDT
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.
Comment 128 Chris Lord [:cwiiis] 2012-04-20 04:16:42 PDT
(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.
Comment 129 Chris Lord [:cwiiis] 2012-04-20 08:47:40 PDT
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.
Comment 130 Chris Lord [:cwiiis] 2012-04-20 11:19:42 PDT
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.
Comment 131 Chris Lord [:cwiiis] 2012-04-20 11:20:24 PDT
For reference, I find http://bradfrostweb.com/blog/mobile/fixed-position/ to be a great site to test this.
Comment 132 Oleg Romashin (:romaxa) 2012-04-20 11:37:25 PDT
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.
Comment 133 Oleg Romashin (:romaxa) 2012-04-20 16:26:27 PDT
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
Comment 134 Oleg Romashin (:romaxa) 2012-04-20 16:28:58 PDT
oh, display port for scroll=1 container is 0.
Probably we should set display port for that correctly
Comment 135 Oleg Romashin (:romaxa) 2012-04-20 16:47:07 PDT
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
Comment 136 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-20 16:58:50 PDT
(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.
Comment 137 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-20 17:06:00 PDT
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 138 :Ehsan Akhgari 2012-04-24 20:50:03 PDT
*** Bug 748641 has been marked as a duplicate of this bug. ***
Comment 139 Oleg Romashin (:romaxa) 2012-04-24 21:48:32 PDT
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)
Comment 140 Chris Lord [:cwiiis] 2012-04-25 08:41:52 PDT
(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.
Comment 141 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-25 20:14:07 PDT
(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.
Comment 142 Chris Lord [:cwiiis] 2012-04-26 06:55:21 PDT
(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.
Comment 143 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-26 16:29:40 PDT
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?
Comment 144 Chris Lord [:cwiiis] 2012-04-26 18:49:49 PDT
(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).
Comment 145 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-26 18:54:55 PDT
Stop, I'm working on a patch :-)
Comment 146 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-26 19:23:58 PDT
Created attachment 618899 [details] [diff] [review]
fix

Try this.
Comment 147 Chris Lord [:cwiiis] 2012-04-26 19:48:37 PDT
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? :)
Comment 148 Chris Lord [:cwiiis] 2012-04-27 09:38:52 PDT
(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?
Comment 149 Chris Lord [:cwiiis] 2012-04-27 12:21:16 PDT
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?
Comment 150 Chris Lord [:cwiiis] 2012-04-27 13:15:05 PDT
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.
Comment 151 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-27 13:47:17 PDT
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?
Comment 152 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-27 13:55:57 PDT
(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.
Comment 153 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-04-27 14:52:44 PDT
(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).
Comment 154 Timothy Nikkel (:tnikkel) 2012-04-27 15:02:50 PDT
(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.
Comment 155 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-04-27 20:19:53 PDT
(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.
Comment 156 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-28 02:56:46 PDT
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.
Comment 157 Chris Lord [:cwiiis] 2012-04-28 03:05:27 PDT
(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)
Comment 158 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-28 03:11:47 PDT
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)?
Comment 159 Chris Lord [:cwiiis] 2012-04-28 03:17:57 PDT
(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.
Comment 160 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-28 22:35:20 PDT
(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.
Comment 161 Chris Lord [:cwiiis] 2012-04-29 01:40:13 PDT
(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.
Comment 162 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-04-29 15:19:33 PDT
(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.
Comment 163 Chris Lord [:cwiiis] 2012-04-30 05:16:55 PDT
(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.
Comment 164 Chris Lord [:cwiiis] 2012-04-30 05:23:18 PDT
(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 Ali Juma [:ajuma] 2012-04-30 06:56:50 PDT
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"?
Comment 166 Matt Brubeck (:mbrubeck) 2012-04-30 12:11:17 PDT
(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.
Comment 167 Chris Lord [:cwiiis] 2012-04-30 12:41:46 PDT
(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.
Comment 168 Chris Lord [:cwiiis] 2012-04-30 13:31:33 PDT
Created attachment 619666 [details] [diff] [review]
Reconcile async scrolling in CompositorParent

Addresses review comments.
Comment 169 Ali Juma [:ajuma] 2012-04-30 13:33:31 PDT
Comment on attachment 619666 [details] [diff] [review]
Reconcile async scrolling in CompositorParent

Looks good to me.
Comment 170 Timothy Nikkel (:tnikkel) 2012-05-01 01:10:36 PDT
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.
Comment 171 Chris Lord [:cwiiis] 2012-05-01 08:25:49 PDT
Bug 750535 is also blocking on layers having some idea of their dimensions.
Comment 172 JP Rosevear [:jpr] 2012-05-01 12:13:09 PDT
We may not take this if the risk is too high.
Comment 173 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-01 12:24:29 PDT
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.
Comment 174 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-01 12:27:37 PDT
Created attachment 620019 [details] [diff] [review]
improve definition of the "fixed layer" flag

Patch with comments fixed.
Comment 175 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-01 12:28:11 PDT
These should land once the tree is open.
Comment 176 Chris Lord [:cwiiis] 2012-05-01 14:51:14 PDT
(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?
Comment 177 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-01 15:40:11 PDT
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".
Comment 178 Chris Lord [:cwiiis] 2012-05-01 15:44:15 PDT
(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?
Comment 179 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-01 15:59:31 PDT
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
Comment 180 Matt Brubeck (:mbrubeck) 2012-05-03 14:59:34 PDT
Does this have patches that should land now that the tree is open?
Comment 181 Chris Lord [:cwiiis] 2012-05-03 15:11:24 PDT
Yes, these last two patches should land now - roc, are you planning on pushing your patch?
Comment 182 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-03 16:28:59 PDT
I can push them both I guess
Comment 184 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-03 22:29:32 PDT
Backed out due to build bustage (mine)
Comment 186 Ed Morley [:emorley] 2012-05-04 06:37:41 PDT
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
Comment 188 :Ehsan Akhgari 2012-05-04 13:39:08 PDT
Backed out: https://hg.mozilla.org/mozilla-central/rev/32a8564667a5
Comment 189 Chris Lord [:cwiiis] 2012-05-10 07:40:53 PDT
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?
Comment 190 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-05-10 07:46:04 PDT
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
Comment 191 Chris Lord [:cwiiis] 2012-05-10 07:50:01 PDT
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.
Comment 192 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-05-10 14:39:24 PDT
This is all Nick...
Comment 193 Chris Lord [:cwiiis] 2012-05-22 06:15:10 PDT
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.
Comment 194 Chris Lord [:cwiiis] 2012-05-22 07:36:49 PDT
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.
Comment 195 Chris Lord [:cwiiis] 2012-05-22 09:57:32 PDT
(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)
Comment 196 Chris Lord [:cwiiis] 2012-05-22 10:05:37 PDT
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.
Comment 197 Chris Lord [:cwiiis] 2012-05-22 11:03:25 PDT
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!)
Comment 198 Aaron Train [:aaronmt] 2012-05-22 11:12:33 PDT
*** Bug 757210 has been marked as a duplicate of this bug. ***
Comment 199 Chris Lord [:cwiiis] 2012-05-22 11:13:18 PDT
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.
Comment 200 Chris Lord [:cwiiis] 2012-05-22 11:14:51 PDT
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 Ali Juma [:ajuma] 2012-05-22 11:16:49 PDT
(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.
Comment 202 Chris Lord [:cwiiis] 2012-05-22 11:29:25 PDT
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.
Comment 203 Chris Lord [:cwiiis] 2012-05-22 12:43:55 PDT
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.
Comment 204 Nick Cameron [:nrc] 2012-05-22 22:11:29 PDT
I think I've fixed my bit, un-assigning. Happy to help out if needed though.
Comment 206 Chris Lord [:cwiiis] 2012-05-23 01:47:38 PDT
Backed out middle commit, re-landed as http://hg.mozilla.org/integration/mozilla-inbound/rev/8fd7f0d0c654
Comment 207 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-05-23 07:36:13 PDT
*** Bug 756713 has been marked as a duplicate of this bug. ***
Comment 209 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-05-25 10:43:04 PDT
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?
Comment 210 Chris Lord [:cwiiis] 2012-05-26 01:59:43 PDT
(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
Comment 211 Martijn Wargers [:mwargers] (not working for Mozilla) 2012-05-26 02:17:31 PDT
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
Comment 212 Chris Lord [:cwiiis] 2012-05-26 03:39:16 PDT
(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...
Comment 213 Chris Lord [:cwiiis] 2012-05-26 03:45:41 PDT
Pushed the rest to inbound: http://hg.mozilla.org/integration/mozilla-inbound/rev/24f3dd5529bb
Comment 214 Mats Palmgren (:mats) 2012-05-26 04:34:58 PDT
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.
Comment 215 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-05-26 05:22:43 PDT
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...
Comment 216 Chris Lord [:cwiiis] 2012-05-26 07:07:09 PDT
(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?
Comment 217 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-05-26 07:43:30 PDT
Yup, I'll upload a patch.
Comment 218 Ryan VanderMeulen [:RyanVM] 2012-05-26 15:25:54 PDT
(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
Comment 219 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-05-28 06:25:55 PDT
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.
Comment 220 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-05-29 06:20:29 PDT
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
Comment 221 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-05-29 07:24:13 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e6ad9e6c6eb
Comment 222 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-05-29 15:37:13 PDT
Should be in http://ftp.mozilla.org/pub/mozilla.org/mobile/tinderbox-builds/mozilla-inbound-android/1338327891/
Comment 223 Naoki Hirata :nhirata (please use needinfo instead of cc) 2012-05-29 15:53:17 PDT
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
Comment 224 Ed Morley [:emorley] 2012-05-30 08:16:49 PDT
https://hg.mozilla.org/mozilla-central/rev/1e6ad9e6c6eb
Comment 225 Mark Finkle (:mfinkle) (use needinfo?) 2012-05-31 11:53:31 PDT
letting this ride the trains for fx15
Comment 226 away[Nov24,Dec5) Kartikaya Gupta (email:kats@mozilla.com) 2012-06-05 08:34:30 PDT
(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.
Comment 227 Robert O'Callahan (:roc) (email my personal email if necessary) 2012-06-05 16:23:57 PDT
I believe currently fixed-pos elements are fixed to the CSS viewport.
Comment 228 Chris Lord [:cwiiis] 2012-06-06 06:45:14 PDT
(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.

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