1050727 - Reduce overpaint of the settings app

Status: RESOLVED FIXED

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

Description

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

Attachment: settings.patch

This should shape some overpaint of the app (a bit more than 100) and save some fps while panning. It should also reduce a bit the memory allocated for graphics purpose while panning.

Comment 1

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

Comment on attachment 8469958 [details] [diff] [review]
settings.patch

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

Moving the background color to the body element makes a blank screen upon starting up the app. The patch seem against to the optimization we did in bug 950250.

Mason, would you mind take a look at this patch? Thanks.

::: apps/settings/style/settings.css
@@ +53,5 @@
>    visibility: hidden;
>  }
>  
>  section[role="region"].current {
> +  transform: none;

This breaks the transition. We still need 'translateX(0)'.

Comment 2

[Mason Chang [Inactive] [:mchang]]

Attachment: Layer Tree w/o Patch

Comment 3

[Mason Chang [Inactive] [:mchang]]

Attachment: Layer Tree w/ Patch

Comment 4

[Mason Chang [Inactive] [:mchang]]

Attachment: Layer Tree w/ Notes

I'm actually confused that this works, but it does remove one layer. However, it also makes the scrollable layer (line 38 of this attachment), component alpha instead of opaque which is bad. Is it possible to get a layer tree that both removes the layer that this patch removes as well as main the scrollable layer as opaque?

Comment 5

[Mason Chang [Inactive] [:mchang]]

Comment on attachment 8469958 [details] [diff] [review]
settings.patch

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

See comment below.

::: apps/settings/style/settings.css
@@ -94,5 @@
> -section[role="region"],
> -section[role="region"] > div > ul {
> -  background-color: #f4f4f4;
> -}
> -

If we leave this in, plus the addition of the background-color in line 13, we both remove one layer and maintain the scrollable layer as opaque. Can we do that?

Comment 6

[Mason Chang [Inactive] [:mchang]]

Attachment: Layer Tree w/ modified patch

Layer tree with the suggestion from comment 5.

Comment 7

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

(In reply to Arthur Chen [:arthurcc] from comment #1)
> ::: apps/settings/style/settings.css
> >  
> >  section[role="region"].current {
> > +  transform: none;
> 
> This breaks the transition. We still need 'translateX(0)'.

Really ?

Comment 8

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

The transition plays at the time near the end of the transition. I am not sure about the reason but it might be due to the delay of switching stacking context (no stacking context when transform is set to none).

Comment 9

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

Attachment: settings.less.overpaint.patch

Thanks Arthur for the review. 
I spent some time looking into the reason why the transition is delayed and it seems like it is related to a platform issue where a reflow happens if a non-null |transform| css rule is added to an element with |overflow: hidden|. I filled bug 1059154 for that and remove the overflow: hidden rule in the settings app as it does not seems to be really needed and is a leftover from the past.

Comment 10

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

Comment on attachment 8479711 [details] [diff] [review]
settings.less.overpaint.patch

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

Thanks for the patch! Please check my comment above.

::: apps/settings/style/settings.css
@@ +9,5 @@
>    width: 100%;
>    height: 100%;
>    font-size: 10px;
>    overflow: hidden;
> +  background-color: #f4f4f4;

Not sure if this line is critical to the patch but it results in a black background during the rubber band effect takes place. As we already have a background color specified both on headers and list elements, it seems we don't need a background here.

Comment 11

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

(In reply to Arthur Chen [:arthurcc] from comment #10)
> Comment on attachment 8479711 [details] [diff] [review]
> settings.less.overpaint.patch
> 
> Review of attachment 8479711 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for the patch! Please check my comment above.
> 
> ::: apps/settings/style/settings.css
> @@ +9,5 @@
> >    width: 100%;
> >    height: 100%;
> >    font-size: 10px;
> >    overflow: hidden;
> > +  background-color: #f4f4f4;
> 
> Not sure if this line is critical to the patch but it results in a black
> background during the rubber band effect takes place. As we already have a
> background color specified both on headers and list elements, it seems we
> don't need a background here.

This line is necessary. The black effect you see is gecko bug 1058506. It happens for all apps where the overfill is below 200, which often means there is only one layer to paint and no background color layer.

Comment 12

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

Comment on attachment 8479711 [details] [diff] [review]
settings.less.overpaint.patch

I was concerned about the black effect. But the over-scroll animation has been altered and we won't see the black background again, so I am giving the patch r+. Thanks!

Comment 13

[[e-mail]]

vivien, does this issue landed? could we close it?

Comment 14

[vnicolas]

I guess it can be closed now.