Matt Brubeck (:mbrubeck) Reporter
	Description • 13 years ago

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.

	Oleg Romashin (:romaxa) Assignee
	Comment 1 • 13 years ago

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

	Oleg Romashin (:romaxa) Assignee
	Comment 2 • 13 years ago

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)

	Benjamin Stover (:stechz)
	Comment 3 • 13 years ago

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

	Oleg Romashin (:romaxa) Assignee
	Comment 4 • 13 years ago

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.

	Oleg Romashin (:romaxa) Assignee
	Comment 5 • 13 years ago

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.

	Tatiana Meshkova (:tatiana)
	Comment 6 • 13 years ago

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

	Oleg Romashin (:romaxa) Assignee
	Comment 7 • 13 years ago

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

	Chris Jones [:cjones] inactive; ni?/f?/r? if you need me
	Comment 8 • 13 years ago

Comment on attachment 531525 [details] [diff] [review]
Disable Fixed pos layers remoting by default

Yuck, but r=me as long as this dies eventually.

	Matt Brubeck (:mbrubeck) Reporter
	Comment 9 • 13 years ago

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

	Oleg Romashin (:romaxa) Assignee
	Comment 10 • 13 years ago

tree seems not very happy right now, so will add checkin-needed

	Matt Brubeck (:mbrubeck) Reporter
	Comment 11 • 13 years ago

We can't check this in without disabling the tests too.

	Matt Brubeck (:mbrubeck) Reporter
	Comment 13 • 13 years ago

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.

	Matt Brubeck (:mbrubeck) Reporter
	Comment 14 • 13 years ago

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

	Oleg Romashin (:romaxa) Assignee
	Comment 15 • 13 years ago

hmm, checked try build and see unexpected pass even with reftest fails ==...

	Tatiana Meshkova (:tatiana)
	Comment 16 • 13 years ago

without this part there will be visibility issues on scroll

	Oleg Romashin (:romaxa) Assignee
	Comment 17 • 13 years ago

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

	Dão Gottwald [:dao]
	Comment 18 • 13 years ago

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

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 19 • 13 years ago

Comment on attachment 531525 [details] [diff] [review]
Disable Fixed pos layers remoting by default

backout for aurora

	Matt Brubeck (:mbrubeck) Reporter
	Comment 20 • 13 years ago

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.

	Matt Brubeck (:mbrubeck) Reporter
	Comment 21 • 13 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 22 • 13 years ago

Comment on attachment 532200 [details] [diff] [review]
Disable fixed items visibility calculation

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

	Matt Brubeck (:mbrubeck) Reporter
	Comment 23 • 13 years ago

Comment on attachment 532200 [details] [diff] [review]
Disable fixed items visibility calculation

Pushed to try: http://tbpl.mozilla.org/?tree=Try&rev=c46dd492e8d7

	Matt Brubeck (:mbrubeck) Reporter
	Comment 24 • 13 years ago

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

	Matt Brubeck (:mbrubeck) Reporter
	Comment 25 • 13 years ago

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.

	Matt Brubeck (:mbrubeck) Reporter
	Comment 26 • 13 years ago

Pushed to try with reftest disabled: http://tbpl.mozilla.org/?tree=Try&rev=999c11b4c175

	Matt Brubeck (:mbrubeck) Reporter
	Comment 27 • 13 years ago

Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/e90e60817ea7
http://hg.mozilla.org/mozilla-central/rev/5e83bde0aace

	Matt Brubeck (:mbrubeck) Reporter
	Comment 28 • 13 years ago

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

	Matt Brubeck (:mbrubeck) Reporter
	Comment 29 • 13 years ago

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.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 30 • 13 years ago

Comment on attachment 532514 [details] [diff] [review]
disable reftest

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

	Mark Finkle (:mfinkle) (use needinfo?)
	Comment 31 • 13 years ago

We should verify these changes on mozilla-central before approving for mozilla-aurora. We can do that on Monday, May 16th.

	Matt Brubeck (:mbrubeck) Reporter
	Comment 32 • 13 years ago

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