342900 - open tab in background gives no indication that tab opened in overflow area

Status: VERIFIED FIXED

(Firefox :: Tabbed Browser, defect)

Description

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

If you have enough tabs to be scrolling the tab bar and you control-click a link to open it in a new tab in the background, you don't receive any indication that a new tab was actually opened -- all the current tabs stay exactly the same size, etc.  We may need to flash or otherwise highlight the ">" button so that people know that something did in fact happen, and so they can find where their tab went.

Comment 1

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

interesting point.  seeing what mconnor and beltzner thing about visually showing that "something" happened.

Comment 2

[:Gavin Sharp [email: [e-mail]]]

*** Bug 342911 has been marked as a duplicate of this bug. ***

Comment 3

[Dão Gottwald [:dao]]

Rather than highlighting/flashing, inserting new tabs next to the opener sounds like a more decent solution.

Comment 4

[Adam Kowalczyk]

(In reply to comment #3)
> Rather than highlighting/flashing, inserting new tabs next to the opener sounds
> like a more decent solution.

This goes along well with some ideas from bug 327562 which also suggest opening new tabs to the right of the current tab. Perhaps a decision should be made there first as it may fix this bug as well.

Comment 5

[Uri Bernstein]

(In reply to comment #3)
> Rather than highlighting/flashing, inserting new tabs next to the opener sounds
> like a more decent solution.
> 

This won't help (by itself) if the current tab happens to be the rightmost tab currently displayed on the tab bar.

Comment 6

[Dão Gottwald [:dao]]

(In reply to comment #5)
> This won't help (by itself) if the current tab happens to be the rightmost tab
> currently displayed on the tab bar.

Of course auto-scrolling would have to happen in that case.

By the way, it's also annoying that one has no idea of how many tabs are off the visible area. But that's probably another bug?

Comment 7

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(In reply to comment #3)
> Rather than highlighting/flashing, inserting new tabs next to the opener sounds
> like a more decent solution.

Don't make me come over there.

Comment 8

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

mconnor suggests:

a)  do *not* change where the tab opens (continue to open it as the last tab)
b)  do *not* ensure the new tab is visible in the scroll tab strip
c)  do visually indicate (as vlad originally suggested) that the new tab was opened.

as an alternative to flashing, he suggested that we turn the arrow scroll button a different color, and then fade back to the normal color.

Comment 9

[Dão Gottwald [:dao]]

(In reply to comment #8)
Are there some reasons, too?

My thoughts are: If I open tabs (even in the background) I actually plan to switch to them very soon. Having to scroll all the time isn't fun at all. To me it's not only a question of indication, but also workflow. I think that's what bug 342911 was about, too, so it should be tackled here.

Comment 10

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

After a quick conversation with mconnor/belzner/a few others, one idea I had was to scroll the tab bar such that both the current tab and the new tab are both visible, if possible.  So if your current tab is the rightmost tab and any new tabs would be overflow, it would keep scrolling to the left until it got to the first position.

However, if the current tab is already in the first position, then you still have this problem -- at which point I think visually highlighting or otherwise flashing the > would be good.

Comment 11

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

yes to blocking, but not needed for beta1

Comment 12

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

What vlad said.  (Which is freakily like what I proposed to beltzner in an IRC channel a few minutes ago.)

I'm still not sure about the whole scrolling thing, tbh, but I'll let it grow on me.

Comment 13

[Uri Bernstein]

(In reply to comment #12)

> I'm still not sure about the whole scrolling thing, tbh, but I'll let it grow
> on me.
> 

Is there some public forum where a discussion is being held about "the whole scrolling thing", or are we past that point? I have some thoughts of my own on this, and I'm also interested in the discussion leading to the current solution.

Comment 14

[Oakwine]

(In reply to comment #8)
> mconnor suggests:
> as an alternative to flashing, he suggested that we turn the arrow scroll
> button a different color, and then fade back to the normal color.
 
This sounds like a good method to me.  Question: if multiple pages are opened simultaneously (e.g., "Open Folder in Tabs", should the arrow scroll button pulse/flash for each tab that is opened, or only once?  Similarly, if the indicator hasn't faded yet, and the user opens a second tab off-screen, how should this be handled?

Comment 15

[Nickolay_Ponomarev]

Some discussion about this and other problems with the overflow solution is happening in bug 343251.

Comment 16

[Phil Ringnalda (:philor)]

*** Bug 344385 has been marked as a duplicate of this bug. ***

Comment 17

[Ria Klaassen (not reading all bugmail)]

*** Bug 344398 has been marked as a duplicate of this bug. ***

Comment 18

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

> as an alternative to flashing, he suggested that we turn the arrow scroll
> button a different color, and then fade back to the normal color.

the patch in bug #343251 takes care of this by changing the background of the new "all tabs" button to orange 3 times (every 150 ms).

Coming up with a better animation shouldn't be too difficult, once we decide what to do.

mconnor has suggested a "pulse then fade".

Comment 19

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

the fix for bug #343251 has landed (on the trunk) with my "place holder animation".

now it's time to implement mconnor's suggested animation:

mconnorfx: basically a very fast transition to full bright, then fade back to 0
mconnorfx: say 1.5 or 2 second to fade
saspitzer: so fast from 0 to bright, and then slow fade back to 0.
saspitzer: (talking about opacity)
mconnorfx: right

Comment 20

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

in another bug, mconnor writes:

> hardcoding colors is bad bad bad!  what if the text color in the user theme was
> blue? need to find an nsITheme color that'd work in this case.

for my temporary indication, I have hard coded a color.  mconnor suggested I use the native focus color.

Comment 21

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

a question for beltnzer / ben / mconnor:  I'm currently showing the indication by flashing (soon to be pulse / fade) of the all tabs button.  before the all tabs button, the plan to was to pulse / fade the right (for LTR) arrow scroll box button.

which do you prefer?

Comment 22

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

Attachment: patch

Comment 23

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

working on a simpler patch, as there are a couple bugs with the stack widget that prevent me from doing it this way.

additionally, mconnor suggests this exact animation:

"what I think we were envisioning was something like going from 0 to 100 in ~ .5 seconds, then staying at 100 for 2 seconds or so, then fading out over 1-1.5 seconds"

Comment 24

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

Attachment: updated patch, some todo items, the biggest is an issue with pinstripe

Comment 25

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

Attachment: updated patch, some todo items, the biggest is an issue with pinstripe

Comment 26

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

Attachment: per mconnor: "start at 0.5 and ramp up in half the time"

Comment 27

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

Attachment: simpler, cleaner patch using work around for bug #346035 (from bz)

Comment 28

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

Attachment: even simpler patch (and renamed mFlash* to mAnimate*)

Comment 29

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

Attachment: more simplification, I don't need the mAnimating boolean

I need to double check on mac, but I think this is at a stage where it could be reviewed.

Comment 30

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

question:  should mouse over or click (or other events) on the button stop the animation?

Comment 31

[Dão Gottwald [:dao]]

(In reply to comment #30)
> question:  should mouse over or click (or other events) on the button stop the
> animation?

Maybe mouseover or mousedown ...
If it doesn't feel right, speeding up the fade out animation would be an option rather than stopping it.

Comment 32

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

Attachment: stop animation on click

per mconnor:

Q: should events (like mouseover, click, etc) of the button stop any animation?
A: click yes, mouseover no, I think

to do this I've added a _stopAnimation() method that I call from when the animation end naturally and when we show the all tabs menupopup.

Comment 33

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

I haven't figure out how to reproduce this, but at least twice now I am seeing a weird issue.

the issue is that the button appears as if the opacity of the toolbarbutton is 1.0 (instead of the .999 that I've set in browser.css)

domi tells me that it's still .999, but visually, it looks like it looks when it is 1.0

Comment 34

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

Comment on attachment 230916 [details] [diff] [review]
stop animation on click

there is a problem with my patch, updated version coming....

Comment 35

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

Attachment: patch, includes a couple other bug fixes which are due to land on the branch

Comment 36

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

this patch includes simon's fix for bug #345868 (which has landed on the trunk) and my fix for bug #346172 (which has also landed on the trunk.)

here's what caused the mixup over #345868.  I was marking the animation as started  (this.mAnimateStep = 0;) before the check to see if the tab was not visible.

simon's fix for bug #345868 exposed this issue with my patch.  (thanks simon!)

mconnor, can you review?

Comment 37

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

> domi tells me that it's still .999, but visually, it looks like it looks when
it is 1.0

I still have not figured out what causes this, or how to fix it.

Comment 38

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

some feedback on the animation from asaf:

1)  he feels 2.75 seconds is too long.
2   he thinks that mousdown on the scrollbutton-down button should also stop the animation.
3)  he thinks there's also some sense in flashing the scrollbutton instead of the "all tabs" button

I agree with #2.  The others I'm still thinking over.

beltzner / mconnor / ben?

Comment 39

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

>> domi tells me that it's still .999, but visually, 
>>it looks like it looks when it is 1.0
>
>I still have not figured out what causes this, or how to fix it.

I don't have a way to reproduce it, but keep running into it.

I claim it is related to bug #346035, so I have spun out this issue into bug #346307, which I think should block ff2.

Comment 40

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

Attachment: uses dbaron's .99 fix for the animation (to work around bug #346307)

Comment 41

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

Comment on attachment 231161 [details] [diff] [review]
uses dbaron's .99 fix for the animation (to work around bug #346307)

note, this patch includes fixes for this (and bugs #346314 #345868 #346172, which are due to land on the branch.)

Comment 42

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

Attachment: patch, using dbaron's "position: relative" trick

Comment 43

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

Attachment: patch for review (excludes fixes for other bugs.)

Comment 44

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

> 2   he thinks that mousedown on the scrollbutton-down button should also stop
> the animation.

spun off to bug #346581

Comment 45

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

Attachment: oops, forgot the flex on the hbox in the pinstripe globalBinding

Comment 46

[Mike Connor [:mconnor]]

Comment on attachment 231323 [details] [diff] [review]
oops, forgot the flex on the hbox in the pinstripe globalBinding

>? diff.txt
>? themes/winstripe/global/jar.mn.
>Index: content/widgets/tabbrowser.xml
>===================================================================
>RCS file: /cvsroot/mozilla/toolkit/content/widgets/tabbrowser.xml,v
>retrieving revision 1.103.2.70
>diff -p -u -8 -r1.103.2.70 tabbrowser.xml
>--- content/widgets/tabbrowser.xml	30 Jul 2006 17:30:18 -0000	1.103.2.70
>+++ content/widgets/tabbrowser.xml	30 Jul 2006 19:10:39 -0000
>@@ -2481,24 +2481,36 @@
>   </binding>
> 
>   <binding id="tabbrowser-tabs"
>            extends="chrome://global/content/bindings/tabbox.xml#tabs">
>     <content>
>       <xul:arrowscrollbox anonid="arrowscrollbox" orient="horizontal" flex="1" style="min-width: 1px;" clicktoscroll="true">
>         <children includes="tab"/>
>       </xul:arrowscrollbox>
>-      <xul:hbox class="tabs-alltabs-box" align="center" pack="end" 
>-                anonid="alltabs-box">
>-        <xul:toolbarbutton class="tabs-alltabs-button" type="menu">
>-          <xul:menupopup class="tabs-alltabs-popup"
>-                         anonid="alltabs-popup" 
>-                         position="after_end"/>
>+      <xul:stack align="center" pack="end">
>+        /* XXXsspitzer hack
>+         * this extra hbox with position: relative
>+         * is needed to work around two bugs, see bugs #346307 and #346035 
>+         */

We typically use HTML style comments, I'm surprised this works, actually.

>       <xul:hbox class="tabs-closebutton-box" align="center" pack="end" anonid="tabstrip-closebutton">
>         <xul:toolbarbutton class="close-button tabs-closebutton"/>
>       </xul:hbox>
>     </content>
>     <implementation implements="nsITimerCallback">
>       <constructor>
>         <![CDATA[
>           var pb2 =
>@@ -2535,19 +2547,19 @@
>           }
>           window.addEventListener("resize", onResize, false);
>         ]]>
>       </constructor>
> 
>       <destructor>
>         <![CDATA[
>           // Release timer to avoid reference cycles.
>-          if (this.mFlashTimer) {
>-            this.mFlashTimer.cancel();
>-            this.mFlashTimer = null;
>+          if (this.mAnimateTimer) {
>+            this.mAnimateTimer.cancel();
>+            this.mAnimateTimer = null;
>           }
>         ]]>
>       </destructor>
> 
>       <field name="mTabstripWidth">0</field>
> 
>       <field name="mTabstrip">
>         document.getAnonymousElementByAttribute(this, "anonid", "arrowscrollbox");
>@@ -2680,67 +2692,87 @@
>                                                 "anonid", "alltabs-popup");
>       </field>
> 
>       <field name="mAllTabsBox">
>         document.getAnonymousElementByAttribute(this, 
>                                                 "anonid", "alltabs-box");
>       </field>
> 
>-      <field name="mFlashTimer">null</field>
>-      <field name="mFlashStage">0</field>
>-      <field name="mFlashStart">6</field>
>-      <field name="mFlashDelay">150</field>
>+      <field name="mAnimateTimer">null</field>
>+      <field name="mAnimateStep">-1</field>
>+      <field name="mAnimateDelay">50</field>
>+      <field name="mAnimatePercents">
>+	[0.50, 0.60, 0.70, 0.80, 0.90,
>+	 1.00, 1.00, 1.00, 1.00, 1.00, 1.00, 1.00, 1.00, 1.00, 1.00,
>+	 1.00, 1.00, 1.00, 1.00, 1.00, 1.00, 1.00, 1.00, 1.00, 1.00,
>+	 1.00, 1.00, 1.00, 1.00, 1.00, 1.00, 1.00, 1.00, 1.00, 1.00,
>+	 0.95, 0.90, 0.85, 0.80, 0.75, 0.70, 0.65, 0.60, 0.55, 0.50,
>+	 0.45, 0.40, 0.35, 0.30, 0.25, 0.20, 0.15, 0.10, 0.05, 0.00]
>+      </field>

So, there's a cleaner way of doing this, IMO, but this will work for now.

These should be private fields (_ not m), IMO.

Looks good, let's get this on trunk and get more feedback ASAP.

Comment 47

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

thanks for the review.

> We typically use HTML style comments, I'm surprised this works, actually.

whoops!  it works, but using DOMi, I see my comments are showing up as #text nodes.

I'll fix it.

> So, there's a cleaner way of doing this, IMO, but this will work for now.

Let me know what you have in mind, and I can clean it up.  But I have found that this way makes it very easy change the animation.

>  These should be private fields (_ not m), IMO.

I'll fix this, too.

i'll fix those two issues, attach a new patch, and land on the trunk.

Comment 48

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

Attachment: use html style comments and switch from mAnimate* to _animate, per mconnor

Comment 49

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

my last patch works on branch mac, trunk and branch windows, but not trunk mac!

even though the opacity of tabs-alltabs-box is 0.0 (and I've confirmed that with the DOMi) the tells, the background-color (orange, until the 2.0 theme update) always shows through.

I'll come up with a test case, and double check that it is not just a pilot error on my part.  

(the patches in this bug have worked on the trunk mac before, honest!)

Comment 50

[Dão Gottwald [:dao]]

(In reply to comment #49)
> my last patch works on branch mac, trunk and branch windows, but not trunk mac!

bug 325296?
you could also set visibility to 'hidden'.

Comment 51

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

> bug 325296?

looks likely! I'll test out the patch in that bug, and see if that fixes my problem.

thanks!

Comment 52

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

I'm not sure the cause yet, but there is definitely a difference in how opacity is handled between the trunk and branch.  See the minimal test case in bug #346035 (I've attached some screen shots.)

Comment 53

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

Attachment: screen shot of the problem on the mac trunk (see bug #346738 a.k.a. #325286)

Comment 54

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

> (see bug #346738 a.k.a. #325286)

oops, I meant aka #325296

due to that trunk mac opacity bug, mconnor suggests that I land but comment out the background-color in pinstripe/global/browser.css which will disable the animation on mac trunk, but it will still work on windows trunk (and, it will work on mac branch, but we won't be able to test this on the mac trunk.)

I'll log a new bug about "background tab animation doesn't work on mac trunk" and refer to it in pinstripe/global/browser.css

Comment 55

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

landed on trunk, but note, the animation (and the previous "flashing" behavior) is disabled for mac due to bug #325296.

Comment 56

[AKIHIRO Misaki (a.k.a Kuden)]

Attachment: screen shot

The hover state of All Tabs Button seems to shift on Windows 2000 or XP Classic theme.
The stack right under .tabbrowser-tabs used some famous Tabbed Extensions.
The id or class needs for stack right under .tabbrowser-tabs.

.tabbrowser-tabs > stack {margin-top: -1px; margin-bottom: 1px;}

Comment 57

[Johannes Buchner]

Could the tab be scrolled into view as long as that doesn't hide the opening tab?

Like: 
  A < B C D E F G H > I

We have the tabs A-H, where A is not visible (left outside). We open a new tab from H or E, I. At the moment it's hidden, but will show the flashing in orange (color is not used anywhere else?).

It would make sense to scroll a little bit to the right to make the new tab visible. (according to a usability study by Google and Netscape)
!Except! if that would scroll the calling tab out of view.

How would you like that?

Comment 58

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

Comment on attachment 231419 [details] [diff] [review]
use html style comments and switch from mAnimate* to _animate, per mconnor

seeking to land on the branch to get more feedback (especially mac, as mac trunk users can't see the animation.)

(note improved animation coming soon, see bug #346901)

Comment 59

[Mike Schroepfer]

Comment on attachment 231419 [details] [diff] [review]
use html style comments and switch from mAnimate* to _animate, per mconnor

approved by schrep for drivers

Comment 60

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

fix landed on the branch.

Comment 61

[Tracy Walker [:tracy]]

verified with Bon Echo from 20060080304 on Windows

Comment 62

[Johannes Buchner]

Put Comment #57 in new bug 347315.

Comment 63

[Dave Townsend [:mossop]]

*** Bug 348326 has been marked as a duplicate of this bug. ***