345868 - "All Tabs" menu button does not flash during 'Open in Tabs' or any use of gBrowser.addTab when tabs open off-screen

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Oakwine]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060725 BonEcho/2.0b1
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8.1b1) Gecko/20060725 BonEcho/2.0b1

When a new tab is opened offscreen, there is supposed to be an indication that the action has occurred.  Bug 343251 introduced a behavior whereby the "all tabs" menu flashes three times to show this.  However, this flashing does not occur when you open a folder of bookmarks in tabs.

Reproducible: Always

Steps to Reproduce:
1. Open enough tabs so that some overflow and are no longer visible.
2. Middle-click a bookmark so that the new tab opens 
3. Observe that the "all tabs" menu flashes.
4. Middle-click a folder so that all of the bookmarks in that folder open.  (You can also use the "Open All in Tabs" menu item.

Actual Results:  
The "all tabs" menu does not flash, even though the tabs have opened.

Expected Results:  
The "all tabs" menu should flash.

Although it was fixed in a different bug, Bug 342900 "open tab in background gives no indication that tab opened in overflow area" gives background as to why this indicator is important.

I do not know if the indicator should flash if some of the tabs open on-screen and others open off-screen.

Comment 1

[Oakwine]

To reveal this behavior, it may help to have browser.tabs.loadFolderAndReplace set to "false".

Comment 2

[(not reading, please use [e-mail] instead)]

accepting.  thanks for catching this bug.  

(adding [bookmarks] for my own benefit.)

it may not be a ff 2.0 blocker, though.

Comment 3

[Simon Bünzli]

Why not do the flashing from a timeout set in addTab? This would also make it easily consistent for extensions which add tabs. And to reiterate what I already said in bug 343251 comment #147:

Note that this doesn't only affect extensions if you indeed set a timeout
before checking for the tab's visibility. There are other consumers of addTab
(e.g. tabbrowser's loadTabs and SessionStore's restoreWindow and undoCloseTab)
which either add more than one tab at once or move the tab around right after
adding it.

Comment 4

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

I agree with Seth's assertion. We'd take a patch, it's nice polish, but this isn't blocking.

Comment 5

[Simon Bünzli]

Attachment: flash after a timeout set from addTab

Simple fix as you originally had planned it: check for each new tab which doesn't end up as current whether it's completely visible and flash the All Tabs button if it isn't.

Comment 6

[Nickolay_Ponomarev]

It's not just bookmarks, every user of addTab is broken in this way (think extensions, don't know how many in-tree users are there).

Comment 7

[Asaf Romano (gone)]

Comment on attachment 230654 [details] [diff] [review]
flash after a timeout set from addTab

r=mano

Comment 8

[Asaf Romano (gone)]

Checking in toolkit/content/widgets/tabbrowser.xml;
/cvsroot/mozilla/toolkit/content/widgets/tabbrowser.xml,v  <--  tabbrowser.xml
new revision: 1.180; previous revision: 1.179
done

Comment 9

[(not reading, please use [e-mail] instead)]

I haven't tested it on the trunk, but on the branch, on windows xp, this is causing a problem.

for me, the animation never happens anymore.

I'll double check trunk and my branch again, to make sure it wasn't just pilot error on my part.

asaf, did you test this on mac?

Comment 10

[Asaf Romano (gone)]

This does wfm (tested on 1.8 branch/OS X debug build).

Comment 11

[Mike Connor [:mconnor]]

Comment on attachment 230654 [details] [diff] [review]
flash after a timeout set from addTab

Mano, please re-request approval once this is known to work on Windows branch.

Comment 12

[(not reading, please use [e-mail] instead)]

I'll try again on my windows xp branch and trunk, but I'm seeing weirdness

Comment 13

[Simon Bünzli]

(In reply to comment #12)
> I'll try again on my windows xp branch and trunk, but I'm seeing weirdness

It would of course help if you could be a little more specific about the weirdness you're observing. This code happens to work nicely on my 2006072604 branch nightly (WinXP). Have you tried a branch without your work for bug 342900 included?

Comment 14

[(not reading, please use [e-mail] instead)]

simon, my apologies for not being specific.

for me, the animation never happens anymore.

I'll try on a tree without my changes for bug #342900

Comment 15

[Asaf Romano (gone)]

Comment on attachment 230654 [details] [diff] [review]
flash after a timeout set from addTab

Seth says the issue was in the patch for bug 342900

Comment 16

[(not reading, please use [e-mail] instead)]

> Seth says the issue was in the patch for bug 342900

sorry for not reporting back sooner.

the problem is with my patch (but interestingly, this patch helps reveal the issue in my code!)

thanks for fixing this, simon.

Comment 17

[(not reading, please use [e-mail] instead)]

question for simon (and others):

what should happen when I open a folder of <n> bookmarks, and i have <n> tabs already open?

should I get the notification that something was opened off screen, even though a new tab was not created?

Comment 18

[Simon Bünzli]

That depends on what we want to achieve with the flash notification in the first place:
* only note about new tabs being added off-screen
* note about all sort of stuff going on in off-screen tabs

The second (more comprehensive) alternative would probably not only include a notification for automatically loading new URLs in off-screen tabs, but also a notification about tabs being closed off-screen, tabs being refreshed/reloaded off-screen, etc. Not sure where that'd be to stop...

Then there's the issue then that users might think that all tabs are replaced with the bookmark folder while technically it's only their content which is replaced. OTOH I doubt that many will notice the (missing) flashing at all.

Anyway since this is an edge case to an edge case (loading a big bookmark group while having a lot of tabs open) I'd not worry too much about it and probably go with the technically simpler solution we've got now (at least for Firefox 2).

Comment 19

[Mike Schroepfer]

Comment on attachment 230654 [details] [diff] [review]
flash after a timeout set from addTab

approved by schrep for drivers

Comment 20

[(not reading, please use [e-mail] instead)]

fix landed on the branch.

thanks again for fixing this, simon.

Comment 21

[(not reading, please use [e-mail] instead)]

> what should happen when I open a folder of <n> bookmarks, and i have <n> tabs
already open?

I've spun this off to bug #346569 and assigned it to beltzner so that he'll comment.

but I agree simon, either way, this is not something that blocks FF2.