Ctrl-Tab previews are oversized with a small number of tabs open

VERIFIED FIXED in Firefox 37

Status

()

Firefox
General
VERIFIED FIXED
3 years ago
2 years ago

People

(Reporter: dao, Assigned: abdelrhman, Mentored)

Tracking

Trunk
Firefox 37
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good next bug][lang=js])

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

3 years ago
Steps to reproduce:

- in about:config, set browser.ctrlTab.previews to true
- with with three tabs open, hold Ctrl and hit Tab to open the Ctrl-Tab panel
- open another tab and re-open the Ctrl-Tab panel repeatedly

-> observe the Ctrl-Tab previews getting smaller until you reach six tabs

I think it would make more sense to always have the previews at the same size (as if there were six tabs open).

All the relevant code is located here:
https://hg.mozilla.org/mozilla-central/file/tip/browser/base/content/browser-ctrlTab.js

To fix this bug, a maxTabPreviews getter should be added to the ctrlTab object and it should return this.previews.length - 1 (minus 1 because the last preview is not a tab preview but the "Show all X tabs" button). Then, maxTabPreviews should be used in tabPreviewCount instead of this.previews.length - 1 and in canvasWidth instead of tabPreviewCount; the last part is what will actually fix this bug.
(Assignee)

Comment 1

3 years ago
Created attachment 8538118 [details] [diff] [review]
rev 1 - Ctrl-Tab previews are oversized
Attachment #8538118 - Flags: review?(dao)
(Reporter)

Comment 2

3 years ago
Comment on attachment 8538118 [details] [diff] [review]
rev 1 - Ctrl-Tab previews are oversized

Since canvasWidth, canvasHeight and maxTabPreviews won't ever change, can you make them "smart" getters such that we won't needlessly do these calculations again and again? See for instance the "previews" getter.
(Reporter)

Updated

3 years ago
Assignee: nobody → codo.abdo
(Assignee)

Comment 3

3 years ago
Created attachment 8538554 [details] [diff] [review]
rev 2 - Ctrl-Tab previews are oversized
Attachment #8538554 - Flags: review?(dao)
(Reporter)

Comment 4

3 years ago
Comment on attachment 8538554 [details] [diff] [review]
rev 2 - Ctrl-Tab previews are oversized

>+  get maxTabPreviews () {
>+    return this.maxTabPreviews = this.previews.length - 1;
>+  },

>+  get canvasWidth () {
>+    return this.canvasWidth = Math.ceil(screen.availWidth * .85 / this.maxTabPreviews);
>+  },
>+  get canvasHeight () {
>+    return this.canvasHeight = Math.round(this.canvasWidth * tabPreviews.aspectRatio);
>+  },

You need to delete the getters (e.g. delete this.maxTabPreviews;) before setting the properties. Otherwise this won't have an effect.

Also, could you please move these three getters up to the other smart/lazy getters (e.g. after the "previews" getter)?
Attachment #8538554 - Flags: review?(dao) → review-
(Reporter)

Updated

3 years ago
Attachment #8538118 - Attachment is obsolete: true
Attachment #8538118 - Flags: review?(dao)
(Assignee)

Comment 5

3 years ago
Created attachment 8538615 [details] [diff] [review]
rev 3 - Ctrl-Tab previews are oversized
Attachment #8538615 - Flags: review?(dao)
(Reporter)

Comment 6

3 years ago
Comment on attachment 8538615 [details] [diff] [review]
rev 3 - Ctrl-Tab previews are oversized

Looks perfect, thanks!
Attachment #8538615 - Flags: review?(dao) → review+
(Reporter)

Updated

3 years ago
Attachment #8538554 - Attachment is obsolete: true
(Assignee)

Comment 7

3 years ago
I need to be vouched for Mozillians community, can you help me for that?!
https://mozillians.org/en-US/u/codo.abdo/
(Reporter)

Comment 8

3 years ago
(In reply to Abdelrhman from comment #7)
> I need to be vouched for Mozillians community, can you help me for that?!
> https://mozillians.org/en-US/u/codo.abdo/

Sure, done!
(Assignee)

Comment 9

3 years ago
Thanks, for any upcoming bugs just tell me to work on.
(Reporter)

Comment 10

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/08d1470a4042

(In reply to Abdelrhman from comment #9)
> Thanks, for any upcoming bugs just tell me to work on.

I'll keep an eye out and let you know if I find something.
Something in this push caused mass bustage. Backed out.
https://hg.mozilla.org/integration/fx-team/rev/e4c72ee3f315
(Reporter)

Comment 12

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/a00723e1a186
https://hg.mozilla.org/mozilla-central/rev/a00723e1a186
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 37
I have reproduced this bug with Firefox Nightly 37.0a1 (Build ID: 20141217030202) on 
windows 8.1 64-bit with the instructions from comment 0 .

Verified as fixed with Firefox Nightly 45.0a1 (Build ID: 20151208030212)

Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0
QA Whiteboard: [bugday-20151209]
I have reproduced this bug on Nightly 37.0a1 (2014-12-17) on ubuntu 14.04 LTS, 32 bit!

The bug's fix is now verified on Latest Nightly 46.0a1!

Build ID: 20151216030229
User Agent: Mozilla/5.0 (X11; Linux i686; rv:46.0) Gecko/20100101 Firefox/46.0

Marked this bug as verified!
Status: RESOLVED → VERIFIED
QA Whiteboard: [bugday-20151209] → [bugday-20151216]
You need to log in before you can comment on or make changes to this bug.