Last Comment Bug 656167 - Temporarily disable fixed layers (bug 607417)
: Temporarily disable fixed layers (bug 607417)
Status: RESOLVED FIXED
[aurora-backout]
:
Product: Core
Classification: Components
Component: Graphics (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla5
Assigned To: Oleg Romashin (:romaxa)
:
: Milan Sreckovic [:milan]
Mentors:
Depends on:
Blocks: 607417
  Show dependency treegraph
 
Reported: 2011-05-10 16:54 PDT by Matt Brubeck (:mbrubeck)
Modified: 2011-05-19 09:01 PDT (History)
9 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
+
fixed
unaffected
5+


Attachments
Disable Fixed pos layers remoting by default (1.17 KB, patch)
2011-05-10 19:30 PDT, Oleg Romashin (:romaxa)
cjones.bugs: review+
mark.finkle: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
same patch with headers... ready to import (1.24 KB, patch)
2011-05-12 10:02 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
mark reftest as fails, until related bugs fixed and feature enabled back (856 bytes, patch)
2011-05-12 10:38 PDT, Oleg Romashin (:romaxa)
no flags Details | Diff | Splinter Review
Disable fixed items visibility calculation (2.43 KB, patch)
2011-05-13 06:56 PDT, Tatiana Meshkova (:tatiana)
roc: review+
mark.finkle: approval‑mozilla‑aurora+
Details | Diff | Splinter Review
disable reftest (1.36 KB, patch)
2011-05-15 07:15 PDT, Matt Brubeck (:mbrubeck)
roc: review+
mark.finkle: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Matt Brubeck (:mbrubeck) 2011-05-10 16:54:49 PDT
Bug 607417 (position:fixed layers for async scrolling) introduced several visible regressions, including bug 653133, bug 656036, and bug 656114.  These are all regressions from Firefox 4 that are present in mozilla-aurora.  Rather than try to fix all of them on Aurora this late in the Firefox 5 cycle, it might be better to disable or back out bug 607417 until they are fixed.
Comment 1 Oleg Romashin (:romaxa) 2011-05-10 18:46:53 PDT
Problems with crash, having fix already with r. but zoom problem is actually behave very bad. I found that zooming size and scale is actually correct there, but viewport scrolling allows main layer scrolling independently from Fixed positioned layer.... news.google.com for example, where page main layer is hidden behind fixed layer, but it possible to see whole page content if you scroll it right. something wrong with our remote-viewport position and I'm not 100% that it is fixed-pos layers implementation problem at all...
Comment 2 Oleg Romashin (:romaxa) 2011-05-10 18:52:25 PDT
About quick fixed-layers feature disabling, then we can just stop sending info about fixed layers to ShadowLayers (from child to parent) here 
http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/ShadowLayersParent.cpp#293
or make it by pref, or temp env (until problems with zoom issues are fixed)
Comment 3 Benjamin Stover (:stechz) 2011-05-10 19:03:13 PDT
(In reply to comment #1)
> Problems with crash, having fix already with r. but zoom problem is actually
> behave very bad. I found that zooming size and scale is actually correct
> there, but viewport scrolling allows main layer scrolling independently from
> Fixed positioned layer.... news.google.com for example, where page main
> layer is hidden behind fixed layer, but it possible to see whole page
> content if you scroll it right. something wrong with our remote-viewport
> position and I'm not 100% that it is fixed-pos layers implementation problem
> at all...

It's not a matter of fault; it's that this just isn't ready yet. The main problems being: 1) clicking things in fixed position areas, 2) bad positions on zoom and 3) fixed position divs always take up valuable space when zoomed in. 1 and 3 will take a little more time to solve.

(In reply to comment #2)
> About quick fixed-layers feature disabling, then we can just stop sending
> info about fixed layers to ShadowLayers (from child to parent) here 
> http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/
> ShadowLayersParent.cpp#293
> or make it by pref, or temp env (until problems with zoom issues are fixed)

Either is fine with me.
Comment 4 Oleg Romashin (:romaxa) 2011-05-10 19:30:53 PDT
Created attachment 531525 [details] [diff] [review]
Disable Fixed pos layers remoting by default

Probably naming should be better, but in general this should work, and allow easier fixing/testing of new problems caused by remoting fixed-pos layers.
Comment 5 Oleg Romashin (:romaxa) 2011-05-10 19:53:28 PDT
Comment on attachment 531525 [details] [diff] [review]
Disable Fixed pos layers remoting by default

In additional to this patch we should do something with pos-fixed Ripc test https://bugzilla.mozilla.org/attachment.cgi?id=524288
which will fail without this fix...
stechz suggested to use some pref and enable this feature for Ripc tests suite... but not sure what is the right way to do that, probably roc,dbaron could advice with that somehow.
Comment 6 Tatiana Meshkova (:tatiana) 2011-05-11 02:02:54 PDT
Comment on attachment 531525 [details] [diff] [review]
Disable Fixed pos layers remoting by default

you also need to disable IsFixedFrame and IsFixedItem code in nsDisplayList
Comment 7 Oleg Romashin (:romaxa) 2011-05-11 06:20:11 PDT
> 3) fixed position divs always take up valuable space when zoomed in.
this is not very clear, because the same problem we have on desktop... and the only way to solve it: add possibility to hide fixed divs completely, or add option to not soom them with content, but that is more up to design problem..
Comment 8 Chris Jones [:cjones] inactive; ni?/f?/r? if you need me 2011-05-11 22:11:55 PDT
Comment on attachment 531525 [details] [diff] [review]
Disable Fixed pos layers remoting by default

Yuck, but r=me as long as this dies eventually.
Comment 9 Matt Brubeck (:mbrubeck) 2011-05-12 09:34:07 PDT
(In reply to comment #7)
> > 3) fixed position divs always take up valuable space when zoomed in.
> this is not very clear, because the same problem we have on desktop... and
> the only way to solve it: add possibility to hide fixed divs completely, or
> add option to not soom them with content, but that is more up to design
> problem..

I think this would be helped a lot by fixing these elements to the CSS viewport rather than the screen, and ensuring that panning moves the CSS viewport in a useful way (as discussed in bug 656036 comment 3).
Comment 10 Oleg Romashin (:romaxa) 2011-05-12 10:02:46 PDT
Created attachment 531970 [details] [diff] [review]
same patch with headers... ready to import

tree seems not very happy right now, so will add checkin-needed
Comment 11 Matt Brubeck (:mbrubeck) 2011-05-12 10:31:41 PDT
We can't check this in without disabling the tests too.
Comment 12 Oleg Romashin (:romaxa) 2011-05-12 10:38:10 PDT
Created attachment 531982 [details] [diff] [review]
mark reftest as fails, until related bugs fixed and feature enabled back
Comment 13 Matt Brubeck (:mbrubeck) 2011-05-12 10:44:45 PDT
Nominating for tracking-firefox5 to bring this potential Aurora backout to release drivers' attention.  Note that this should affect Firefox for mobile only.  I'm going to push romaxa's patches to Try.
Comment 14 Matt Brubeck (:mbrubeck) 2011-05-12 10:52:01 PDT
(In reply to comment #13)
> I'm going to push romaxa's patches to Try.

Nevermind, I see that romaxa just did:
http://tbpl.mozilla.org/?tree=Try&rev=3c46bc426fac

After these go through tryserver and land on mozilla-central, we will request mozilla-approval-aurora.
Comment 15 Oleg Romashin (:romaxa) 2011-05-12 16:12:34 PDT
hmm, checked try build and see unexpected pass even with reftest fails ==...
Comment 16 Tatiana Meshkova (:tatiana) 2011-05-13 06:56:59 PDT
Created attachment 532200 [details] [diff] [review]
Disable fixed items visibility calculation

without this part there will be visibility issues on scroll
Comment 17 Oleg Romashin (:romaxa) 2011-05-13 07:30:12 PDT
Pushed env disable fix without reftests, checked on try and that works fine.
http://hg.mozilla.org/mozilla-central/rev/7ba9f4f76e73

also need to check patch in comment 16
Comment 18 Dão Gottwald [:dao] 2011-05-13 09:24:33 PDT
(In reply to comment #17)
> Pushed env disable fix without reftests, checked on try and that works fine.
> http://hg.mozilla.org/mozilla-central/rev/7ba9f4f76e73

Bad commit message. It doesn't say whether it disables or backs out.
Comment 19 Mark Finkle (:mfinkle) (use needinfo?) 2011-05-13 12:09:57 PDT
Comment on attachment 531525 [details] [diff] [review]
Disable Fixed pos layers remoting by default

backout for aurora
Comment 20 Matt Brubeck (:mbrubeck) 2011-05-13 14:30:42 PDT
Comment on attachment 531525 [details] [diff] [review]
Disable Fixed pos layers remoting by default

Pushed to Aurora: http://hg.mozilla.org/mozilla-aurora/rev/44057f266e96

It looks like we will want Tatiana's additional patch (attachment 532200 [details] [diff] [review]) on Aurora too, once it is reviewed and landed on mozilla-central.
Comment 21 Matt Brubeck (:mbrubeck) 2011-05-13 14:57:58 PDT
Comment on attachment 532200 [details] [diff] [review]
Disable fixed items visibility calculation

This patch fixes a problem I am seeing in the login form on gog.com (see steps to reproduce in bug 656114).  We will definitely want this on Aurora too.
Comment 22 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-14 02:55:23 PDT
Comment on attachment 532200 [details] [diff] [review]
Disable fixed items visibility calculation

Review of attachment 532200 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 23 Matt Brubeck (:mbrubeck) 2011-05-14 07:44:21 PDT
Comment on attachment 532200 [details] [diff] [review]
Disable fixed items visibility calculation

Pushed to try: http://tbpl.mozilla.org/?tree=Try&rev=c46dd492e8d7
Comment 24 Matt Brubeck (:mbrubeck) 2011-05-14 08:59:42 PDT
Pushed to try again with the reftest-disable patch, which seems to be needed with the visibility fix: http://tbpl.mozilla.org/?tree=Try&rev=953d24503d06
Comment 25 Matt Brubeck (:mbrubeck) 2011-05-14 10:15:00 PDT
Comment on attachment 531982 [details] [diff] [review]
mark reftest as fails, until related bugs fixed and feature enabled back

This doesn't quite work - with the visibility patch applied, this test fails in IPC reftests but still passes in regular reftests.  So we'll need to somehow mark it failing in IPC only, or just disable it.
Comment 26 Matt Brubeck (:mbrubeck) 2011-05-15 07:15:25 PDT
Created attachment 532514 [details] [diff] [review]
disable reftest

Pushed to try with reftest disabled: http://tbpl.mozilla.org/?tree=Try&rev=999c11b4c175
Comment 28 Matt Brubeck (:mbrubeck) 2011-05-15 09:50:53 PDT
Comment on attachment 532200 [details] [diff] [review]
Disable fixed items visibility calculation

Requesting approval-mozilla-aurora.  This is necessary to finish backing out this new feature for Firefox 5 and fix regressions from Firefox 4 (see comment 0 for details).
Comment 29 Matt Brubeck (:mbrubeck) 2011-05-15 09:56:06 PDT
Comment on attachment 532514 [details] [diff] [review]
disable reftest

Requesting approval-mozilla-aurora.  This patch just comments out a reftest.  The reftest was added along with the feature in bug 607417, and will not pass in the IPC harness after the patches in this bug to disable that feature.
Comment 30 Robert O'Callahan (:roc) (email my personal email if necessary) 2011-05-15 19:11:00 PDT
Comment on attachment 532514 [details] [diff] [review]
disable reftest

Review of attachment 532514 [details] [diff] [review]:
-----------------------------------------------------------------
Comment 31 Mark Finkle (:mfinkle) (use needinfo?) 2011-05-15 21:53:00 PDT
We should verify these changes on mozilla-central before approving for mozilla-aurora. We can do that on Monday, May 16th.
Comment 32 Matt Brubeck (:mbrubeck) 2011-05-16 12:10:42 PDT
Pushed the remaining patches (disable reftest and visibility calculation) to Aurora for Firefox 5:
http://hg.mozilla.org/releases/mozilla-aurora/rev/da8b793ae875
http://hg.mozilla.org/releases/mozilla-aurora/rev/afa91b879e86

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