The default bug view has changed. See this FAQ.

Temporarily disable fixed layers (bug 607417)

RESOLVED FIXED in Firefox 5

Status

()

Core
Graphics
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: mbrubeck, Assigned: romaxa)

Tracking

Trunk
mozilla5
Points:
---

Firefox Tracking Flags

(firefox5+ fixed, status2.0 unaffected, fennec5+)

Details

(Whiteboard: [aurora-backout])

Attachments

(4 attachments, 1 obsolete attachment)

(Reporter)

Description

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

Comment 1

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

Comment 2

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

Comment 4

6 years ago
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.
Attachment #531525 - Flags: review?
(Assignee)

Comment 5

6 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.
Attachment #531525 - Flags: review? → review?(jones.chris.g)
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
(Assignee)

Comment 7

6 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..
tracking-firefox5: ? → ---
tracking-fennec: ? → 5+
Comment on attachment 531525 [details] [diff] [review]
Disable Fixed pos layers remoting by default

Yuck, but r=me as long as this dies eventually.
Attachment #531525 - Flags: review?(jones.chris.g) → review+
(Reporter)

Comment 9

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

Comment 10

6 years ago
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
Assignee: nobody → romaxa
Status: NEW → ASSIGNED
(Assignee)

Updated

6 years ago
Keywords: checkin-needed
(Reporter)

Comment 11

6 years ago
We can't check this in without disabling the tests too.
Keywords: checkin-needed
(Assignee)

Comment 12

6 years ago
Created attachment 531982 [details] [diff] [review]
mark reftest as fails, until related bugs fixed and feature enabled back
(Reporter)

Comment 13

6 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.
tracking-firefox5: --- → ?
Summary: Temorarily disable or back out fixed layers (bug 607417) → Temporarily disable or back out fixed layers (bug 607417)
(Reporter)

Comment 14

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

Comment 15

6 years ago
hmm, checked try build and see unexpected pass even with reftest fails ==...
Created attachment 532200 [details] [diff] [review]
Disable fixed items visibility calculation

without this part there will be visibility issues on scroll
Attachment #532200 - Flags: review?(roc)
(Assignee)

Comment 17

6 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
(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 on attachment 531525 [details] [diff] [review]
Disable Fixed pos layers remoting by default

backout for aurora
Attachment #531525 - Flags: approval-mozilla-aurora+
(Reporter)

Comment 20

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

Comment 21

6 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.
Comment on attachment 532200 [details] [diff] [review]
Disable fixed items visibility calculation

Review of attachment 532200 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #532200 - Flags: review?(roc) → review+
(Reporter)

Comment 23

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

Comment 24

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

Comment 25

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

Comment 26

6 years ago
Created attachment 532514 [details] [diff] [review]
disable reftest

Pushed to try with reftest disabled: http://tbpl.mozilla.org/?tree=Try&rev=999c11b4c175
Attachment #531982 - Attachment is obsolete: true
(Reporter)

Comment 27

6 years ago
Pushed to mozilla-central:
http://hg.mozilla.org/mozilla-central/rev/e90e60817ea7
http://hg.mozilla.org/mozilla-central/rev/5e83bde0aace
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla6
(Reporter)

Comment 28

6 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).
Attachment #532200 - Flags: approval-mozilla-aurora?
(Reporter)

Comment 29

6 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.
Attachment #532514 - Flags: review?(roc)
Attachment #532514 - Flags: approval-mozilla-aurora?
Comment on attachment 532514 [details] [diff] [review]
disable reftest

Review of attachment 532514 [details] [diff] [review]:
-----------------------------------------------------------------
Attachment #532514 - Flags: review?(roc) → review+
We should verify these changes on mozilla-central before approving for mozilla-aurora. We can do that on Monday, May 16th.

Updated

6 years ago
Summary: Temporarily disable or back out fixed layers (bug 607417) → Temporarily disable fixed layers (bug 607417)
Attachment #532200 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #532514 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(Reporter)

Comment 32

6 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
status-firefox5: affected → fixed
Target Milestone: mozilla6 → mozilla5

Updated

6 years ago
tracking-firefox5: ? → +
You need to log in before you can comment on or make changes to this bug.