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

VERIFIED FIXED

Status

Firefox Graveyard
Panorama
VERIFIED FIXED
7 years ago
2 years ago

People

(Reporter: raymondlee, Assigned: ttaubert)

Tracking

({regression})

Trunk
regression

Details

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

7 years ago
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

7 years ago
This could be related to Bug 633190.
(Reporter)

Comment 2

7 years ago
(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.
(Assignee)

Updated

7 years ago
Hardware: x86 → All
(Assignee)

Updated

7 years ago
Assignee: nobody → tim.taubert
Status: NEW → ASSIGNED
(Assignee)

Updated

7 years ago
Blocks: 604699
(Assignee)

Updated

7 years ago
Blocks: 628701
No longer blocks: 604699
(Assignee)

Comment 3

7 years ago
Created attachment 513992 [details] [diff] [review]
patch v1
Attachment #513992 - Flags: review?(ian)
(Assignee)

Updated

7 years ago
No longer blocks: 628701
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?
Attachment #513992 - Flags: review?(ian) → review-
(Assignee)

Comment 6

7 years ago
(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()?
(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.
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.
(Assignee)

Comment 9

7 years ago
(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 :)
(Assignee)

Comment 10

7 years ago
Created attachment 514061 [details] [diff] [review]
patch v2
Attachment #513992 - Attachment is obsolete: true
Attachment #514061 - Flags: review?(ian)
Comment on attachment 514061 [details] [diff] [review]
patch v2

Looks good!
Attachment #514061 - Flags: review?(ian) → review+
(Assignee)

Comment 12

7 years ago
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.
Attachment #514061 - Flags: approval2.0?
Comment on attachment 514061 [details] [diff] [review]
patch v2

a=beltzner
Attachment #514061 - Flags: approval2.0? → approval2.0+
(Assignee)

Comment 15

7 years ago
Created attachment 514257 [details] [diff] [review]
patch for checkin
Attachment #514061 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Keywords: checkin-needed
(Assignee)

Comment 16

7 years ago
http://hg.mozilla.org/mozilla-central/rev/d6975e3ecb4e
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Comment 17

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

Unable to reproduce issue with latest build.
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.