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

VERIFIED FIXED

Status

defect
VERIFIED FIXED
8 years ago
3 years ago

People

(Reporter: raymondlee, Assigned: ttaubert)

Tracking

({regression})

Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 2 obsolete attachments)

Reporter

Description

8 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

8 years ago
This could be related to Bug 633190.
Reporter

Comment 2

8 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.
Hardware: x86 → All
Assignee: nobody → tim.taubert
Status: NEW → ASSIGNED
Blocks: 604699
Blocks: 628701
No longer blocks: 604699
Posted patch patch v1 (obsolete) — Splinter Review
Attachment #513992 - Flags: review?(ian)
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-
(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.
(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 :)
Posted patch patch v2 (obsolete) — Splinter Review
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+
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+
Attachment #514061 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/d6975e3ecb4e
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Keywords: checkin-needed
Resolution: --- → FIXED

Comment 17

8 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.