Closed Bug 1031258 Opened 5 years ago Closed 5 years ago

Smart collection view does not use APZ

Categories

(Firefox OS Graveyard :: Gaia::Everything.me, defect)

defect
Not set

Tracking

(tracking-b2g:backlog, b2g-v2.0 fixed, b2g-v2.1 fixed)

RESOLVED FIXED
tracking-b2g backlog
Tracking Status
b2g-v2.0 --- fixed
b2g-v2.1 --- fixed

People

(Reporter: vingtetun, Assigned: vingtetun)

References

Details

(Keywords: perf, Whiteboard: [systemsfe])

Attachments

(1 file)

It seems like the html/css code is formatted in such a way that APZ fails.
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking?]
Component: Gaia::Homescreen → Gaia::Everything.me
Whiteboard: [systemsfe]
blocking-b2g: 2.0? → backlog
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking?] → [VH-FL-blocking-][VH-FC-blocking-]
Comment on attachment 8447117 [details] [diff] [review]
bug1031258.patch

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

Scrolling is sooo much better in collections with this. Thanks!

::: apps/collection/view.html
@@ +74,5 @@
>            </header>
>          </section>
>  
> +        <div id="content-background-image">
> +          <div id="content-background-color">

nit: can we rename this to something like: content-background-overlay?

At first glance I assumed that this was for the background-color property, and it seemed weird being on top of the image. I'd recommend a rename to potentially save people the same confusion I faced, but would not block the review on it.
Attachment #8447117 - Flags: review?(kgrandon) → review+
(In reply to Kevin Grandon :kgrandon from comment #2)
> Comment on attachment 8447117 [details] [diff] [review]
> bug1031258.patch
> 
> Review of attachment 8447117 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Scrolling is sooo much better in collections with this. Thanks!
> 
> ::: apps/collection/view.html
> @@ +74,5 @@
> >            </header>
> >          </section>
> >  
> > +        <div id="content-background-image">
> > +          <div id="content-background-color">
> 
> nit: can we rename this to something like: content-background-overlay?
> 
> At first glance I assumed that this was for the background-color property,
> and it seemed weird being on top of the image. I'd recommend a rename to
> potentially save people the same confusion I faced, but would not block the
> review on it.

I will fix the name before landing (already in my PR).
Keywords: perf
Thanks guys. APZ is looking good.

I am a bit uncomfortable with the nested structure of the DOM. When everything is position absolute, I guess we can have the bg as a separate element to the content. Something like:

> <header ... >
> <section id="edit-header" ... >
> <div id="background"></div> // applies image and pseudo element acts as overlay
> <div id="content" ... >


Lmk if it's relevant in your opinion.
Comment on attachment 8447117 [details] [diff] [review]
bug1031258.patch

This is a small patch that greatly improves scroll performance in collections. It is needed for the vertical homescreen.
Attachment #8447117 - Flags: approval-gaia-v2.0?(bbajaj)
Attachment #8447117 - Flags: approval-gaia-v2.0?(bbajaj) → approval-gaia-v2.0+
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.