Last Comment Bug 786672 - Position: fixed elements seem to be broken in Nightly
: Position: fixed elements seem to be broken in Nightly
Status: VERIFIED FIXED
: regression
Product: Core
Classification: Components
Component: Layout (show other bugs)
: 17 Branch
: ARM Android
: -- normal (vote)
: mozilla18
Assigned To: Chris Lord [:cwiiis]
:
Mentors:
Depends on:
Blocks: 785333
  Show dependency treegraph
 
Reported: 2012-08-29 07:59 PDT by Lucas Rocha (:lucasr)
Modified: 2012-09-12 11:34 PDT (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed
verified


Attachments
Reinstate removed code (1.32 KB, patch)
2012-08-29 11:32 PDT, Chris Lord [:cwiiis]
no flags Details | Diff | Review
Fix getting the active scrolled root of an nsDisplayScrollLayer (2.43 KB, patch)
2012-08-29 12:05 PDT, Chris Lord [:cwiiis]
roc: review+
lukasblakk+bugs: approval‑mozilla‑aurora+
Details | Diff | Review

Description Lucas Rocha (:lucasr) 2012-08-29 07:59:25 PDT
The Reader toolbar is a position: fixed element with bottom: 0px. In Beta (FF16) and Aurora (FF17) the toolbar stays on the same position while panning content (as expected). On latest Nightly though, the toolbar moves while panning.

Need to find the regression range. Seems like a serious regression.
Comment 1 Lucas Rocha (:lucasr) 2012-08-29 08:11:15 PDT
Seems to work now on latest nightly. Never mind.
Comment 2 Chris Lord [:cwiiis] 2012-08-29 08:36:52 PDT
In case this comes up again, to help bisect, a bad revision is 103641:271ca35d7645.

I had a quick look at the layer tree, and async scrolling of fixed pos elements breaks because basically the entire layer tree is marked as fixed position, including the root scroll layer. This could indicate that either nsLayoutUtils::IsScrolledByRootContentDocumentDisplayportScrolling has been changed, nsLayoutUtils::GetActiveScrolledRootFor has changed, or the frame-tree has changed in some was so as to affect one or both of those two functions.

The generated display-list is fine and correct.
Comment 3 Chris Lord [:cwiiis] 2012-08-29 11:05:50 PDT
Oh dear, so after stating that this wasn't my fault, I find that this is indeed my fault - This is caused by bug 785333, which just landed again. Looking into it.
Comment 4 Chris Lord [:cwiiis] 2012-08-29 11:23:28 PDT
I think this may have been caused by removing the code that swapped the underlying frame when merging in nsDisplayScrollLayer. Testing now.
Comment 5 Chris Lord [:cwiiis] 2012-08-29 11:32:44 PDT
Created attachment 656537 [details] [diff] [review]
Reinstate removed code

This fixes the issue and I'm attaching it in case this needs to be urgently fixed, or developers would like to have this feature working for the time being.

I'm going to look into why this happens and see if there's a more reliable way of working around it.
Comment 6 Chris Lord [:cwiiis] 2012-08-29 12:05:42 PDT
Created attachment 656547 [details] [diff] [review]
Fix getting the active scrolled root of an nsDisplayScrollLayer

And here's a real patch to fix it.

I think there may be a similar issue with other item types - I still see fixed-background breaking when zooming in (but this happened before any of these patches too), so it might be nice to find a more generic way of fixing this in all situations... Beats me what that way would be though, for now.
Comment 7 Chris Lord [:cwiiis] 2012-08-29 12:06:55 PDT
(In reply to Chris Lord [:cwiiis] from comment #6)
> Created attachment 656547 [details] [diff] [review]
> Fix getting the active scrolled root of an nsDisplayScrollLayer
> 
> And here's a real patch to fix it.
> 
> I think there may be a similar issue with other item types - I still see
> fixed-background breaking when zooming in (but this happened before any of
> these patches too), so it might be nice to find a more generic way of fixing
> this in all situations... Beats me what that way would be though, for now.

To illustrate the background issue, you can go to http://moonintern.com/, scroll while fully zoomed out, then zoom in slightly (but make sure the background is still visible) and then scroll. I see the same thing on various other sites too.
Comment 8 Chris Lord [:cwiiis] 2012-08-30 00:31:53 PDT
Pushed to inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/6a95de502ec1
Comment 9 Ryan VanderMeulen [:RyanVM] 2012-08-30 19:04:36 PDT
https://hg.mozilla.org/mozilla-central/rev/6a95de502ec1
Comment 11 Cristian Nicolae (:xti) 2012-08-31 05:17:12 PDT
This issue is not reproducible anymore on the latest Nightly build. Closing as verified fixed on:

Firefox 18.0a1 (2012-08-31)
Device: Galaxy Note
OS: Android 4.0.4
Comment 12 Chris Lord [:cwiiis] 2012-09-11 05:58:00 PDT
Comment on attachment 656547 [details] [diff] [review]
Fix getting the active scrolled root of an nsDisplayScrollLayer

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Merging bug 785333 but not this will break position:fixed async scrolling
User impact if declined: Async scrolling of position:fixed elements will often break
Testing completed (on m-c, etc.): Baked on m-c for a while, no complaints.
Risk to taking this patch (and alternatives if risky): Low risk.
String or UUID changes made by this patch: None.
Comment 13 Lukas Blakk [:lsblakk] use ?needinfo 2012-09-12 08:50:25 PDT
Comment on attachment 656547 [details] [diff] [review]
Fix getting the active scrolled root of an nsDisplayScrollLayer

needed for 785333, approving.
Comment 14 Chris Lord [:cwiiis] 2012-09-12 10:37:12 PDT
Pushed to aurora:

https://hg.mozilla.org/releases/mozilla-aurora/rev/b97cf7f8f87d

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