950250 - Settings App Should Have a Scrollable Opaque Layer behind The Scrollable Content

Status: RESOLVED FIXED

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

Comment 1

[Mason Chang [Inactive] [:mchang]]

Attachment: settings.patch

Here's a patch that doesn't insert an opaque background, but was an interesting thing I found while playing with the CSS. It removes 1 layer of overpainting according to the FPS counters. This is with APZ enabled. The layer tree doesn't look quite right though since the ContainerLayers are smashed together along with the Thebes layer.

Comment 2

[Mason Chang [Inactive] [:mchang]]

Attachment: Original Layer Tree of Settings App

Comment 3

[Mason Chang [Inactive] [:mchang]]

Attachment: Layer Tree w/ Settings Patch

Comment 4

[Mason Chang [Inactive] [:mchang]]

Attachment: insertDiv.patch

Here's a different approach where we insert another div and force that to be opaque. This is just a few of the many elements which we'd have to modify which I don't like. Actually just changing the inner div and trying to make it scroll with the content with the background-attachment property doesn't work. Any ideas why that won't work?

Comment 5

[Mason Chang [Inactive] [:mchang]]

Attachment: Layer Tree w/ Insert Div.patch

Comment 6

[Mike Lee [:mlee]]

1.3+; blocks a 1.3 CS blocker.

Comment 7

[Mason Chang [Inactive] [:mchang]]

Attachment: Profile of Scrolling w/ APZ w/ Removing Background

Doesn't seem like just removing the background color is a good option. We get a fairly consistent 17 ms layout time which is too long.

Comment 8

[Mason Chang [Inactive] [:mchang]]

Attachment: Profile w/ Scrollable Div

We save some amount of time on layout portion. For the first 5 frames, we improve an average of 7ms per layout. Rasterize times are just a little worse, but without a scrollable background, we get larger and more frequent chunks of over 100 ms rasterize times, which is causing the jank. Scrollable div approach looks like the winner here.

Comment 9

[Mason Chang [Inactive] [:mchang]]

Attachment: scrollableDiv.patch

Patch that inserts a surrounding <div> around scrollable content to optimize the layout engine. We remove a single layer and drop our over painting from ~400 to ~300 w/ APZ. If there is a better way to insert extra <divs> around scrollable content please let me know. Thanks!

Comment 10

[Fabien Cazenave [:kaze]]

I’m a bit sad to insert yet another <div> into every panel, especially as I don’t understand the rationale here.

What is the point of this patch? Is that to improve the first paint time, or to have a smoother panning?

Are you saying that all containers with overflow need an additional inner container to scroll smoothly? If that’s true, it sounds like a bad Gecko bug that will hit us for web content too, right?

Comment 11

[Mason Chang [Inactive] [:mchang]]

(In reply to Fabien Cazenave [:kaze] from comment #10)
> I’m a bit sad to insert yet another <div> into every panel, especially as I
> don’t understand the rationale here.

I'm also a bit sad to insert another <div> into the panel. The problem is that currently, the parent <div> isn't scrolling with the content. If you change the background-image to a picture, you can see that it's not scrolling with the content. If we change it to scroll with the content, we should get smoother scrolling performance.

> What is the point of this patch? Is that to improve the first paint time, or
> to have a smoother panning?

Smoother scrolling.
 
> Are you saying that all containers with overflow need an additional inner
> container to scroll smoothly? If that’s true, it sounds like a bad Gecko bug
> that will hit us for web content too, right?

Not in all cases, at least in a basic website I haven't seen Gecko show this behavior. I'm not exactly sure why Settings app demonstrates this. Benwa, can you answer this? Thanks!

Comment 12

[Benoit Girard (:BenWa)]

(In reply to Fabien Cazenave [:kaze] from comment #10)
> Are you saying that all containers with overflow need an additional inner
> container to scroll smoothly? If that’s true, it sounds like a bad Gecko bug
> that will hit us for web content too, right?

It's not a bad bug in gecko. It's how scrolling is spec'ed in CSS it's harder to implement efficiently. If the background doesn't scroll then we need to make the content above is transparent which is more expensive. If the contents of the div have a background itself then we no longer need to make it transparent.

The engine already detects some cases that it can optimize without changes to the page but not all of them. It doesn't detect all of them because doing so is difficult and/or expensive to compute. For short term perf wins it's easier to simplify the page then modify gecko to recognize this usage.

Comment 13

[Mason Chang [Inactive] [:mchang]]

Were we able to answer all your questions? If so, can we land this? Thanks!

Comment 14

[Fabien Cazenave [:kaze]]

Thanks for your explanations, and thanks for making the Settings app smoother.

Sorry for introducing a delay in the resolution of this bug. Let’s land this.

Comment 15

[Arthur Chen [:arthurcc][inactive after 6/30]]

I'm not sure is this patch trying to fix the same problem as bug 938785? In which we added a background for all <ul> elements (the scrollable content), so that they won't be transparent. The background seems to be removed now by this commit (https://github.com/mozilla-b2g/gaia/commit/78b41c3698265308a53edd1eb64d52e264d27ca0). Can we simply add the background color back to fix the issue instead of modifying the DOM tree?

Comment 16

[Benoit Girard (:BenWa)]

Yes, that's probably a better fix. Let's add a comment so it doesn't get removed again.

Comment 17

[Mason Chang [Inactive] [:mchang]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/15209

That worked! Thanks for bringing up the better solution :)

Comment 18

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8358540 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15209

Thanks for the patch! As we stop using document_bg.png as the background, please change it to the current background color.

Comment 19

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8358540 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15209

Implemented Arthur's review.

Comment 20

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8358540 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15209

Please define the background-color in one place, thanks!

Comment 21

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8358540 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15209

Hopefully the last time! Thanks!

Comment 22

[Arthur Chen [:arthurcc][inactive after 6/30]]

So quick! But we still need this line `section[role="region"]`, please check the github comment.

Comment 23

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8358540 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15209

Ok, hopefully last time again. Thanks much!

Comment 24

[Arthur Chen [:arthurcc][inactive after 6/30]]

Comment on attachment 8358540 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15209

r=me. Many thanks!

Comment 25

[Mason Chang [Inactive] [:mchang]]

Landed in master: b138f6884edd8c1cf6b8e675cfc4f9096a0333b3

Comment 26

[Mason Chang [Inactive] [:mchang]]

uplifted to v1.3 - 7fc5eb67ff0aedbeb6874f670891e8e9b4505a16