Ctrl-Tab should not cycle through hidden tabs

RESOLVED WONTFIX

Status

()

Firefox
Tabbed Browser
RESOLVED WONTFIX
5 years ago
3 years ago

People

(Reporter: lwz, Assigned: lwz)

Tracking

Trunk
Firefox 17
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 6 obsolete attachments)

(Assignee)

Description

5 years ago
User Agent: Mozilla/5.0 (Windows NT 5.1; rv:8.0) Gecko/20100101 Firefox/8.0
Build ID: 20111104165243

Steps to reproduce:

1. Set preference "browser.ctrlTab.previews" to true
2. Create at least two tab groups
3. Switch tab groups (e.g. press Ctrl-`)
4. Hold down Ctrl and press and release the Tab key to open the Ctrl-Tab panel


Actual results:

Panel shows a tab from the other tab group.


Expected results:

Panel should only show tabs from current tab groups.
(Assignee)

Comment 1

5 years ago
Created attachment 594520 [details]
My Fix (in my own coding style)

Filters away hidden tabs from recentlyUsedTabs before adding them to panel.
(Assignee)

Updated

5 years ago
Component: Untriaged → Tabbed Browser
Version: 8 Branch → 12 Branch
(Assignee)

Comment 2

5 years ago
Created attachment 624296 [details] [diff] [review]
Patched get tabList

It's not exactly a patch in the official format, but similar enough. The main idea is to filter hidden tabs out of recentlyUsedTabs, and to use gBrowser.visibleTabs by modifying the loop. I've tested it out by overriding the getter function from ScratchPad.
Attachment #594520 - Attachment is obsolete: true
Attachment #624296 - Flags: review?(dao)
(Assignee)

Comment 3

5 years ago
Has anyone taken a look at the patch? I just don't want to forget about it or let it be forgotten.
(Assignee)

Comment 4

5 years ago
Comment on attachment 624296 [details] [diff] [review]
Patched get tabList

@Tim: Sorry, as Dao wasn't responding, I was hoping you could review this patch instead. Thanks!
Attachment #624296 - Flags: review?(dao) → review?(ttaubert)
Comment on attachment 624296 [details] [diff] [review]
Patched get tabList

>   get tabList () {
>     if (this._tabList)
>       return this._tabList;
> 
>-    // Using gBrowser.tabs instead of gBrowser.visibleTabs, as the latter
>-    // exlcudes closing tabs, breaking the following loop in case the the
>-    // selected tab is closing.
>-    let list = Array.filter(gBrowser.tabs, function (tab) !tab.hidden);
>+    let a, l, list = gBrowser.visibleTabs.slice(0);
> 
>     // Rotate the list until the selected tab is first
>-    while (!list[0].selected)
>-      list.push(list.shift());
>-
>-    list = list.filter(function (tab) !tab.closing);
>+    for (a = 0, l = list.length; a < l; a++) {
>+      if (list[a].selected)
>+        break;
>+      list.push(list[a]);
>+    }
>+    list.splice(0, a);

It's unclear why you're changing this. Seems unrelated to this bug. File a separate bug on it if you think it's a useful change.

Making your patch easy to understand is the best way to bring forward quick reviews ;)
Attachment #624296 - Flags: review?(ttaubert) → review-
(Assignee)

Comment 6

5 years ago
Created attachment 644636 [details] [diff] [review]
Patched get tabList v2

Ok. I have changed only the relevant part now.

The earlier patch changed the original list of tabs to be based on visibleTabs back again but without the infinite loop problem, and didn't use shift (which I think might be inefficient). Also, I thought that splice and unshift might be inefficient, so in that patch I rewrote the last part also. But I suppose those are irrelevant now. Perhaps you could enlighten me about how the JavaScript engine does shift, unshift and splice for arrays... :)
(Assignee)

Updated

5 years ago
Attachment #644636 - Flags: review?(dao)
Comment on attachment 644636 [details] [diff] [review]
Patched get tabList v2

>diff --git a/browser/base/content/browser-tabPreviews.js b/browser/base/content/browser-tabPreviews.js
>--- a/browser/base/content/browser-tabPreviews.js
>+++ b/browser/base/content/browser-tabPreviews.js
>@@ -185,17 +185,28 @@ var ctrlTab = {
>     // Rotate the list until the selected tab is first
>     while (!list[0].selected)
>       list.push(list.shift());
> 
>     list = list.filter(function (tab) !tab.closing);
> 
>     if (this.recentlyUsedLimit != 0) {
>-      let recentlyUsedTabs = this._recentlyUsedTabs;
>-      if (this.recentlyUsedLimit > 0)
>-        recentlyUsedTabs = this._recentlyUsedTabs.slice(0, this.recentlyUsedLimit);
>+      let rlist = this._recentlyUsedTabs, r = this.recentlyUsedLimit, recentlyUsedTabs;

nit: one variable declaration per line, and please name them more clearly

>+      // Filter rlist for relevant tabs and puts them into recentlyUsedTabs
>+      if (r > 0) {
>+        // Limit the number of recently used tabs
>+        let i = 0;
>+        recentlyUsedTabs = [];
>+        while (i < rlist.length && recentlyUsedTabs.length < r) {
>+          if (!rlist[i].hidden && !rlist[i].closing)
>+            recentlyUsedTabs.push(rlist[i]);
>+          i++;
>+        }
>+      } else {
>+        recentlyUsedTabs = rlist.filter(function (tab) !tab.hidden && !tab.closing);
>+      }
>       for (let i = recentlyUsedTabs.length - 1; i >= 0; i--) {
>         list.splice(list.indexOf(recentlyUsedTabs[i]), 1);
>         list.unshift(recentlyUsedTabs[i]);
>       }
>     }
> 
>     return this._tabList = list;

Would it be simpler if we put "...filter(function (tab) !tab.hidden && !tab.closing)" at the bottom, right before the return?
(Assignee)

Comment 8

5 years ago
(In reply to Dão Gottwald [:dao] from comment #7)
> nit: one variable declaration per line, and please name them more clearly

IMO it's clear enough, but I will change it.

> Would it be simpler if we put "...filter(function (tab) !tab.hidden &&
> !tab.closing)" at the bottom, right before the return?

Because then the recentlyUsedLimit will not be accurate anymore.
(Assignee)

Comment 9

5 years ago
What variable names would you like? allRecentlyUsedTabs and recentlyUsedLimit? I've always been in favour of short variable names especially when they are temporary.
(Assignee)

Comment 10

5 years ago
Created attachment 644718 [details] [diff] [review]
Patched get tabList v3
Attachment #624296 - Attachment is obsolete: true
Attachment #644636 - Attachment is obsolete: true
Attachment #644636 - Flags: review?(dao)
Attachment #644718 - Flags: review?(dao)
Comment on attachment 644718 [details] [diff] [review]
Patched get tabList v3

>+      // Filter rlist for relevant tabs and puts them into recentlyUsedTabs

rlist -> allRecentlyUsedTabs
puts -> put

>+      if (recentlyUsedLimit > 0) {
>+        // Limit the number of recently used tabs
>+        let i = 0;
>+        recentlyUsedTabs = [];
>+        while (i < allRecentlyUsedTabs.length && recentlyUsedTabs.length < recentlyUsedLimit) {
>+          if (!allRecentlyUsedTabs[i].hidden && !allRecentlyUsedTabs[i].closing)
>+            recentlyUsedTabs.push(allRecentlyUsedTabs[i]);
>+          i++;
>+        }
>+      } else {
>+        recentlyUsedTabs = allRecentlyUsedTabs.filter(function (tab) !tab.hidden && !tab.closing);
>+      }

Looks like this can be simplified. How about:

>      let recentlyUsedTabs = [];
>      for (let tab of allRecentlyUsedTabs) {
>        if (!tab.hidden && !tab.closing) {
>          recentlyUsedTabs.push(tab);
>          if (recentlyUsedLimit > 0 && recentlyUsedTabs.length >= recentlyUsedLimit)
>            break;
>        }
>      }
Attachment #644718 - Flags: review?(dao) → review-

Updated

5 years ago
Assignee: nobody → limweizhong
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
OS: Windows XP → All
Hardware: x86 → All
Version: 12 Branch → Trunk
(Assignee)

Comment 12

5 years ago
Created attachment 652795 [details] [diff] [review]
Patched get tabList v4

OK. As usual, I thought it would be more efficient doing it in the original way.
Attachment #644718 - Attachment is obsolete: true
Attachment #652795 - Flags: review?(dao)
Comment on attachment 652795 [details] [diff] [review]
Patched get tabList v4

>     if (this.recentlyUsedLimit != 0) {
>-      let recentlyUsedTabs = this._recentlyUsedTabs;
>-      if (this.recentlyUsedLimit > 0)
>-        recentlyUsedTabs = this._recentlyUsedTabs.slice(0, this.recentlyUsedLimit);
>+      let allRecentlyUsedTabs = this._recentlyUsedTabs;
>+      let recentlyUsedLimit = this.recentlyUsedLimit
>+      let recentlyUsedTabs = [];
>+      // Filter allRecentlyUsedTabs for relevant tabs and put them into recentlyUsedTabs
>+      for (let tab of allRecentlyUsedTabs) {
>+        if (!tab.hidden && !tab.closing) {
>+          recentlyUsedTabs.push(tab);
>+          if (recentlyUsedLimit > 0 && recentlyUsedTabs.length >= recentlyUsedLimit)
>+            break;
>+        }
>+      }

Actually, since this code only runs if this.recentlyUsedLimit != 0, you can remove the recentlyUsedLimit > 0 check.
Secondly, you can use this._recentlyUsedTabs and this.recentlyUsedLimit directly at this point without declaring the allRecentlyUsedTabs and recentlyUsedLimit variables. (You'll also need to replace "allRecentlyUsedTabs" in the comment, or just remove the comment.)

r=me with these changes. Thanks!
Attachment #652795 - Flags: review?(dao) → review+
(Assignee)

Comment 14

5 years ago
Actually I thought that if recentlyUsedLimit is negative, it is supposed to add all the tabs (the original worked that way)? I suppose we could change that behavior. Also, I thought looking up a local variable in the immediate scope is more efficient than accessing it from an object?
(In reply to lwz from comment #14)
> Actually I thought that if recentlyUsedLimit is negative, it is supposed to
> add all the tabs (the original worked that way)?

You're right, I didn't consider this case.

> Also, I thought looking up a local variable in the immediate
> scope is more efficient than accessing it from an object?

The difference, if there's one at all, is going to be negligible.
(Assignee)

Comment 16

5 years ago
Created attachment 654596 [details] [diff] [review]
Patched get tabList v5

I've done some testing, and accessing a member of an object with a few members the performance is ~50% while with a large number of members, the performance can go as low as 10%. However, I have done as you wish.

I don't know how to complete this patching process, and I think I'm not able to. My patch is made with WinMerge with 7 lines of context (I can't change that).
Attachment #652795 - Attachment is obsolete: true
Attachment #654596 - Flags: review+
I'm not sure what your 50% and 10% figures refer to.
(Assignee)

Comment 18

5 years ago
It's the speed of accessing the variable from an object compared to accessing the value from a variable in the current scope.
What absolute numbers are we talking about and how do they relate to the total time the loop takes?
(Assignee)

Comment 20

5 years ago
I'm making a general statement based on my experiments with JSPerf. I am not making any specific claim with this particular implementation. The point is that, at least in relative performance, it is just not negligible. If you have say a thousand tabs across all tab groups, there is going to be a slight performance difference.

Whatever the case, I think I have already done what I can with the patching. I don't think I want to debate about this anymore.
The relative difference between this.recentlyUsedLimit and recentlyUsedLimit is only interesting when putting it in perspective, e.g. by answering the two questions in comment 19. Not every micro-optimization is worth it.
Keywords: checkin-needed
Green on Try (the m-oth orange was due to another patch in the push).
https://tbpl.mozilla.org/?tree=Try&rev=4c6632e2334a

https://hg.mozilla.org/integration/mozilla-inbound/rev/1db0e95acab8

Should this have a test?
Flags: in-testsuite?
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/1db0e95acab8
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 17

Updated

5 years ago
Depends on: 808540

Comment 24

5 years ago
I can't pass up this opportunity to complain. This "fix" drove me nuts on upgrading to 17.0.1. It breaks one of the most useful and powerful UI features - to me, anyway. Am I a minority of one? What criteria determined the (apparently consensus) view that "ctrl-tab should not cycle through hidden tabs"? Is it by decree that no information should span tab groups, or did introducing tab groups uncover too many lower-level bugs to fix?
I totally agree with the comment above. Why there's no hidden pref to revert to the previous behavior ?
Could somebody post a link to the discussion that drove to this change ? Don't tell me that there was no discussion about this !
(Assignee)

Comment 26

5 years ago
Ok. I don't disagree with you both, but the reason for doing so was to make it consistent with the behavior without Ctrl-Tab turned on. I'm completely fine if the patch was reverted based on your reasons. However I don't have the authority to do that.
@Dao/Ryan: Are any of you willing to undo the patch?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
I suggest adding a preference browser.ctrlTab.showHiddenTab or something, that should be simple.
(Assignee)

Comment 28

5 years ago
Created attachment 693361 [details] [diff] [review]
Reverts the behavior by default

With this patch, by default Ctrl-Tab will cycle through hidden tabs. Only if a preference "browser.ctrlTab.showHiddenTabs=false" is added, then Ctrl-Tab will not cycle through hidden tabs.
Attachment #693361 - Flags: review?(dao)
(Assignee)

Comment 29

5 years ago
Created attachment 693368 [details] [diff] [review]
Reverts the behavior by default

With this patch, by default Ctrl-Tab will cycle through hidden tabs. Only if a preference "browser.ctrlTab.showHiddenTabs=false" is added, then Ctrl-Tab will not cycle through hidden tabs.

Edit: Now it would not throw an error if you created the preference with a different type.
Attachment #693361 - Attachment is obsolete: true
Attachment #693361 - Flags: review?(dao)
Attachment #693368 - Flags: review?(dao)
Comment on attachment 693368 [details] [diff] [review]
Reverts the behavior by default

I'm not convinced there's enough demand for a hidden pref here. I'd rather just completely revert the change.
Attachment #693368 - Flags: review?(dao) → review-
Let's not reopen the bug unless we actually do decide to revert the change (in which case we can just move it directly to WONTFIX). Seems like we should have that discussion in bug 808540.
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED
Changing to WONTFIX as bug 808540 is on its way to being fixed.
Resolution: FIXED → WONTFIX
You need to log in before you can comment on or make changes to this bug.