Reduce overpaint of the settings app

RESOLVED FIXED

Status

Firefox OS
Gaia::Settings
P2
normal
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: vingtetun, Unassigned)

Tracking

({perf})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [c=graphics p= s= u=])

Attachments

(5 attachments, 1 obsolete attachment)

Created attachment 8469958 [details] [diff] [review]
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.
Attachment #8469958 - Flags: review?(arthur.chen)
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)'.
Attachment #8469958 - Flags: review?(arthur.chen) → feedback?(mchang)
Created attachment 8471003 [details]
Layer Tree w/o Patch
Created attachment 8471044 [details]
Layer Tree w/ Patch
Keywords: perf
Whiteboard: [c=graphics p= s= u=]
Created attachment 8471115 [details]
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?
Attachment #8469958 - Flags: feedback?(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?
Created attachment 8471127 [details]
Layer Tree w/ modified patch

Layer tree with the suggestion from comment 5.
Priority: -- → P2
(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 ?
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).
Created attachment 8479711 [details] [diff] [review]
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.
Attachment #8469958 - Attachment is obsolete: true
Attachment #8479711 - Flags: review?(arthur.chen)
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.
Attachment #8479711 - Flags: review?(arthur.chen)
(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 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!
Attachment #8479711 - Flags: review+

Comment 13

2 years ago
vivien, does this issue landed? could we close it?
Flags: needinfo?(21)

Comment 14

2 years ago
I guess it can be closed now.
Flags: needinfo?(21)

Updated

2 years ago
Status: NEW → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.