Closed Bug 1140095 Opened 5 years ago Closed 5 years ago

[Customizer] Show an opening/closing transition

Categories

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

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: drs, Assigned: djf)

References

Details

(Whiteboard: [spark])

Attachments

(1 file)

The Customizer takes a second or so to open, even on the Aries device. This breaks the user's flow a bit, especially since there's no indication that the gesture was recognized.

One strategy we could use here is to slide in a fake box with the same dimensions as the Customizer, perhaps with the Customizer icon in the center, and then replace it with the actual Customizer once the transition is done.

P1 because I think that this is bad enough that it impedes regular user flow, and isn't just polish.
David, please take this if you have time.
Flags: needinfo?(dflanagan)
I've been working on this, opening and closing the customizer over and over trying to figure out why it is so sluggish when I finally realized that we are registering a new MutationObserver each time we open and never disconnecting it. So we get more and more observers and it gets slower each time. I'll fix that as part of this bug unless someone beats me to it.
Assignee: nobody → dflanagan
Flags: needinfo?(dflanagan)
I've create a PR for the code that removes the mutation observer and the click handler when we close the customizer. https://github.com/fxos/customizer/pull/9

Maybe it is worth landing that as a separate bug, so I'll keep that initial commit separate for now.

I think this makes a noticeable difference in performance so with this patch we may not need as much work to optimize the opening animation.

Also, this work to undo stuff when we close the customizer would also benefit from a change to gaia-dom-tree. We should change the render() method to:

  render: function() {
    this.els.tree.innerHTML = '';
    if (this.root) {
      var tree = this.createTree(this.root);
      this.els.tree.appendChild(tree);
    }
    debug('rendered');
  },

This allows us to set the root to null and then call render() again to clear out the tree and release all its memory. Seems like a worthwhile optimization.
Actually, I've filed bug 1140281 for the resource deallocation stuff, and will attach my patches there then come back to this one.
Depends on: 1140281
This patch uses CSS transitions to animate the open and close. It delays actually populating the tree until the animation has completed so that the animation can be fast. The result is that it feels much more responsive.

There's a minor refactor here where main view CSS from app.css is moved to js/views/main.js, but I can move it back if that's not wanted.

The pull request has two commits in it. The first is the same as the one attached to bug 1140281, and you kind of need it for testing the second commit which is the main point of this patch.

I'm flagging Doug for review of this one since he filed the bug.

But also setting needinfo for Justin since he might actually prefer to do this review.
Flags: needinfo?(jdarcangelo)
Attachment #8573770 - Flags: review?(drs.bugzilla)
Doug: note that it didn't seem necessary to do a fake box as you suggested in comment #0. It was fast enough to animate an empty fxos-customizer on to the screen as long as I waited to populate it until the animation was done.

I've tested this on Flame, and it seems much, much better than what we had before.

Displaying a customizer icon in the animated pane is also a great idea, but that can be part of a p2 bug for later.
Comment on attachment 8573770 [details] [review]
speed up and animate opening and closing the customizer

(In reply to David Flanagan [:djf] from comment #6)
> Doug: note that it didn't seem necessary to do a fake box as you suggested
> in comment #0. It was fast enough to animate an empty fxos-customizer on to
> the screen as long as I waited to populate it until the animation was done.
> 
> I've tested this on Flame, and it seems much, much better than what we had
> before.
> 
> Displaying a customizer icon in the animated pane is also a great idea, but
> that can be part of a p2 bug for later.

Cool, glad to hear it.

I left some comments on the PR. Please rebase this as I've landed the patches in bug 1140281.
Attachment #8573770 - Flags: review?(drs.bugzilla) → review-
Comment on attachment 8573770 [details] [review]
speed up and animate opening and closing the customizer

Doug,

I've addressed your review comments. Please have another look. This basically turned into a refactor of the main view and the main controller. The animation is nice and fast now.

The next step beyond this (in a separate bug) would probably be to defer creation of the fxos-customizer element and all the other views until the customizer is opened for the first time so that we can further reduce our impact on apps when the user does not open the customizer.
Attachment #8573770 - Flags: review- → review?(drs.bugzilla)
Comment on attachment 8573770 [details] [review]
speed up and animate opening and closing the customizer

Looks good, but I left a few comments on the PR. If you end up making extensive changes based on them, it would be best to request review again.
Attachment #8573770 - Flags: review?(drs.bugzilla) → review+
Review comments addressed, and patch landed: https://github.com/fxos/customizer/commit/71d6d9f227d9104c973c813537661ed8bdf291db
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Whiteboard: [lightsaber] → [ignite]
Whiteboard: [ignite] → [spark]
Clearing NI? since this has been resolved.
Flags: needinfo?(jdarcangelo)
You need to log in before you can comment on or make changes to this bug.