635668 - The zoom in animation doesn't work for new tab by group "+" button or double click on group or empty space

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect)

Description

[Raymond Lee [:raymondlee]]

It used to work but the zoom in animation stops working if you create a new tab in the tabview UI using  group + button or double click on group or empty space

Comment 1

[George Carstoiu]

This could be related to Bug 633190.

Comment 2

[Raymond Lee [:raymondlee]]

(In reply to comment #1)
> This could be related to Bug 633190.

I am pretty sure that it's not related to bug 633190 because I am working on a patch for that.

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v1

Comment 4

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 513992 [details] [diff] [review]
patch v1

Passed try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=92a8f2fed542

Comment 5

[Ian Gilman [:iangilman]]

Comment on attachment 513992 [details] [diff] [review]
patch v1

>+  _init: function TabCanvas__init() {
>+    var w = this.canvas.width;
>+    var h = this.canvas.height;
>+    if (!w || !h)
>+      return;
>+
>+    let ctx = this.canvas.getContext("2d");
>+    this._fillCanvasBackground(ctx, w, h, "#fff");
>+  },

Is the canvas width or height ever non-zero? This is only called from the TabItem constructor, before it's given any geometry, so I would think it should always be 0. In which case, does this patch actually do anything?

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #5)
> Is the canvas width or height ever non-zero? This is only called from the
> TabItem constructor, before it's given any geometry, so I would think it should
> always be 0. In which case, does this patch actually do anything?

The <canvas> element has an explicit size of 300x150 (I just checked) so that's why the patch works. Should we give the canvas an explicit size on _init()?

Comment 7

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #6)
> (In reply to comment #5)
> > Is the canvas width or height ever non-zero? This is only called from the
> > TabItem constructor, before it's given any geometry, so I would think it should
> > always be 0. In which case, does this patch actually do anything?
> 
> The <canvas> element has an explicit size of 300x150 (I just checked) so that's
> why the patch works. Should we give the canvas an explicit size on _init()?

Setting a width/height on the canvas early in its init was one of the things I identified as slow in my work for bug 631747... we previously set the width/height on the canvas twice during TabItem creation... once in TabCanvas_init and one via setBounds.

That said, I did not test the new tab creation via UI aspects when writing that patch. Bottom line: please don't reintroduce this bit unless completely necessary.

Comment 8

[Ian Gilman [:iangilman]]

I agree with mitcho; let's not add to the start-up hit. 

I think what we need for this bug is a force option to override this check in _update:

      if (!this.isComplete(tab)) {

... that gets used for the _update calls for zoom. This is a good general principle anyway; when we're going to zoom, we should get the latest regardless of whether it's finished, because that's what you'll be looking at when you arrive at the page.

Comment 9

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #8)
> I think what we need for this bug is a force option to override this check in
> _update:
> 
>       if (!this.isComplete(tab)) {
> 
> ... that gets used for the _update calls for zoom. This is a good general
> principle anyway; when we're going to zoom, we should get the latest regardless
> of whether it's finished, because that's what you'll be looking at when you
> arrive at the page.

Damn that's exactly how I started to write this patch and this also works, of course. Alright I'm gonna do this :)

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v2

Comment 11

[Ian Gilman [:iangilman]]

Comment on attachment 514061 [details] [diff] [review]
patch v2

Looks good!

Comment 12

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 514061 [details] [diff] [review]
patch v2

Note to approvers:

Very small and low-risk patch that fixes zooming when creating new tabs.

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Passed try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=b5bd0105b5ff

Comment 14

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 514061 [details] [diff] [review]
patch v2

a=beltzner

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

http://hg.mozilla.org/mozilla-central/rev/d6975e3ecb4e

Comment 17

[George Carstoiu]

Mozilla/5.0 (X11; Linux i686; rv:2.0b13pre) Gecko/20110224 Firefox/4.0b13pre

Unable to reproduce issue with latest build.