1022746 - The call log isn't using APZC anymore :/

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Dialer, defect, P1)

Description

[Etienne Segonzac (:etienne)]

This might be a fallout from the visual refresh.

STR:
* Install a reference-workload on the device
* Enable overscrolling in the developer settings
* overscroll in the settings app -> you get a cool effect
* Launch the dialer
* Go in the contacts tab, overscroll -> you get a cool effect
* Go in the call log, overscroll -> you get nothing :/ and the scrolling performance is bad.

Looks like we're doing something that makes fall out of the APZC code path.

Comment 1

[Etienne Segonzac (:etienne)]

APZC + Dialer, this one has your name written all over it Doug :)

Comment 2

[Etienne Segonzac (:etienne)]

Oh and I can confirm that everything works fine on earlier versions (tried with 'master@{2014-05-20 18:30:00}').

Comment 3

[Doug Sherk (:drs) (inactive)]

This is the cause (wat?): https://github.com/mozilla-b2g/gaia/commit/c2e8699c8656ddf21e2d35272c28ecd434a57336

Comment 4

[Chris Lord [:cwiiis]]

Although a work-around would be great, it'd be good to understand why this happens from a gfx/layout perspective too - we shouldn't be getting scrollable layers with no APZC :/ (unless I've wildly misunderstood how things are meant to work)

Comment 5

[Kartikaya Gupta (email:[e-mail])]

If I were debugging this, I would first get a compositor-side layer dump to see if the layer is a scroll info layer or a proper scroll layer (see if the thebes layers are children of the container layer or not). If we're getting a scroll info layer (which is the likely scenario), we'd need client-side display list dumps to see why the scroll layer items fail to merge.

Comment 6

[Doug Sherk (:drs) (inactive)]

I found the issue already and I have a fix for it:
https://github.com/DouglasSherk/gaia/commit/349f723cea09fc2e4228ad47105832bfb1701534

I'm just waiting for the integration tests to run since I think this will cause a regression in the number of reflows.

Though I'm not sure why this is bailing us out of the APZC path, and I haven't looked into that yet.

Comment 7

[Botond Ballo [:botond]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> If I were debugging this, I would first get a compositor-side layer dump to
> see if the layer is a scroll info layer or a proper scroll layer (see if the
> thebes layers are children of the container layer or not). If we're getting
> a scroll info layer (which is the likely scenario), we'd need client-side
> display list dumps to see why the scroll layer items fail to merge.

The APZC tree log (bug 958596) contains the same information in a less noisy environment. More, if you count the content description string (which regressed recently, but I just posted a fix in bug 1023491).

Comment 8

[Doug Sherk (:drs) (inactive)]

Attachment: Remove requestAnimationFrame from FontSizeUtils to fix frames falling back to sync panning and zooming.

PR: https://github.com/mozilla-b2g/gaia/pull/20316

This fixes the issue albeit at the cost of some performance, according to the comment. I'm still not sure why this is bailing us out of the APZC code path, but we can investigate that in a followup.

Comment 9

[Jason Smith [:jsmith]]

Can we get an explanation of how bad the scrolling perf is? A video also would help here.

Comment 10

[Doug Sherk (:drs) (inactive)]

(In reply to Jason Smith [:jsmith] from comment #9)
> Can we get an explanation of how bad the scrolling perf is? A video also
> would help here.

With reference-workload-medium, I get about 20-30 fps (eyeballing) on my Flame, so it's pretty bad. We also can't overscroll without APZC.

Comment 11

[Jason Smith [:jsmith]]

(In reply to Doug Sherk (:drs) from comment #10)
> (In reply to Jason Smith [:jsmith] from comment #9)
> > Can we get an explanation of how bad the scrolling perf is? A video also
> > would help here.
> 
> With reference-workload-medium, I get about 20-30 fps (eyeballing) on my
> Flame, so it's pretty bad. We also can't overscroll without APZC.

Okay, that's a pretty bad perf regression. Noming then.

Comment 12

[Guillaume Marty [:gmarty]]

I'm responsible for the initial code using rAF. The font_size_utils.js script does a reflow, so the idea was to delay this reflow until after the layout gets updated. Not using rAF works too but may in some case causes additional reflows.

I need to understand what's the impact of that on the layout.

Comment 13

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8438010 [details] [diff] [review]
Remove requestAnimationFrame from FontSizeUtils to fix frames falling back to sync panning and zooming.

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

Do we know when this code path is triggered ? It should only be triggered after the app startup and once lazy loaded html elements comes in. And so it should not affects the call log, and especially not once the call log is loaded.

Before reviewing it I would like to check that there is not a bigger bug that is hiding in this code.

Comment 14

[Guillaume Marty [:gmarty]]

I double checked and there are not extra calls to functions in font_size_utils after the app is launched.
If, however, I apply this patch, then the call log is butter smooth again and the overscrolling is back.

It's still unclear what's wrong with the rAF.

Comment 15

[Doug Sherk (:drs) (inactive)]

(In reply to Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please) from comment #13)
> Do we know when this code path is triggered ? It should only be triggered
> after the app startup and once lazy loaded html elements comes in. And so it
> should not affects the call log, and especially not once the call log is
> loaded.
> 
> Before reviewing it I would like to check that there is not a bigger bug
> that is hiding in this code.

I don't have time to investigate this deeply (especially since gmarty did a shallow investigation of what you suggested), so unfortunately I think this will have to wait. I filed bug 1027869 to follow up on this. Is this ok with you?

Comment 16

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Comment on attachment 8438010 [details] [diff] [review]
Remove requestAnimationFrame from FontSizeUtils to fix frames falling back to sync panning and zooming.

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

Let's go with the followup. I'm not even sure APZ is disabled - as I can overscroll by panning very quicky sometimes, but then it jumps back to the top instantly.

Comment 17

[Doug Sherk (:drs) (inactive)]

https://github.com/mozilla-b2g/gaia/commit/fec2d2661606a8f1f6f9eba93d9ee84f7273db9b

Comment 18

[Doug Sherk (:drs) (inactive)]

kats says this might have not fixed it for him.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/b3cd6223770b03cd199ecc08dcae0a741c87df60

Comment 20

[Etienne Segonzac (:etienne)]

(In reply to Doug Sherk (:drs) from comment #18)
> kats says this might have not fixed it for him.

Yep on a central/master from today with a reference workload the issue is still happening :/

Comment 21

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug1022746.patch

If it works for you as well Etienne, I will suggest to backout the requestAnimationFrame change as well, since it was likely unrelated.

Comment 22

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Basically the issue seems to be a z-indexes nightmare (again...). I don't see why we need them in shared/style/list.css and removed all of them.

I can't see any regressions on the device without them.

Comment 23

[Etienne Segonzac (:etienne)]

Comment on attachment 8447126 [details] [diff] [review]
bug1022746.patch

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

Looks like this is breaking edit mode, but should be fixable :)

Comment 24

[Etienne Segonzac (:etienne)]

Arnau, any idea?
Maybe we can pointer-events:none instead of putting elements on top with z-indexes.

Or at least keep the z-index stuff only for the edit-mode, so we get APZC scrolling the rest of the time.

Comment 25

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Etienne Segonzac (:etienne) from comment #24)
> Arnau, any idea?
> Maybe we can pointer-events:none instead of putting elements on top with
> z-indexes.
> 
> Or at least keep the z-index stuff only for the edit-mode, so we get APZC
> scrolling the rest of the time.

I want APZ all the time :)

Comment 26

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: bug1022746.patch

Seems like the edit mode is specific for each app...

Comment 27

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Let's see what our CI says about this one: https://tbpl.mozilla.org/?tree=Gaia-Try&rev=01a600fa29ad9a096e67f6e51f17127f05a0f2db

Comment 28

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Yes, edit mode is a bit messy, and will be redesigned soon to use a toolbar at the bottom.
I installed Attachment #8447126 [details] [diff] and I have seen nothing broken in edit mode, is it anything I'm missing?

Comment 29

[Doug Sherk (:drs) (inactive)]

Reverted original fix.

master: https://github.com/mozilla-b2g/gaia/commit/8ffff1ff9eda2b241051388d82ccdbe366abf9af
v2.0: https://github.com/mozilla-b2g/gaia/commit/6b07897e61fc65cdd072f467fd89adbc4ee50e46

Comment 30

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Stealing.

Comment 31

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

Previous target milestone is obsolete, so I'm deferring it to sprint5.

Comment 32

[Etienne Segonzac (:etienne)]

Comment on attachment 8447172 [details] [diff] [review]
bug1022746.patch

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

So smooth! 
Glad we can do this without z-index hacks.

I'd like this to get it stamped by sms/bb peers though.

Comment 33

[Arnau March [:arnau] ( not working in Firefox OS anymore :( )]

Comment on attachment 8447172 [details] [diff] [review]
bug1022746.patch

LGTM.
Vivien, just one thing, could you please also add in lists.css:
[data-type="list"] [data-type="edit"] li > a {
  pointer-events: none;
}

Same thing you are doing in Messages app, in case someone else is using the proposed structure in BB ([data-type="edit"] instead of .edit)

Comment 34

[Steve Chung [:steveck]]

Comment on attachment 8447172 [details] [diff] [review]
bug1022746.patch

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

Nice catch Etienne! Thanks for avoiding the regression.

Comment 35

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://github.com/mozilla-b2g/gaia/commit/d7f7141d3a2b4238ac29234d08ed1728f0a304b6

Comment 36

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/2e99d66d49d18adab49c29d84f2fb1d135e3e10f