343585 - tab scrolling: scenario where open link in background doesn't enable the scroll button

Status: RESOLVED FIXED

(Firefox :: Tabbed Browser, defect, P1)

Description

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

tab scrolling:  scenario where open link in background doesn't enable the scroll button

to reproduce:

1)  open up enough tabs where you get the scroll box arrows
2)  click the right scroll box arrow until it disabled
3)  open a new link in the background

the right scroll box arrow continues to appear disabled.  note, it is clickable and will do the correct thing if you click on it.

Comment 1

[Asaf Romano (gone)]

I was trying to fix this by adding a DOMNodeInserted handler (phase=capturing) in the scrollbox binding (which contains the scrollbox-innerbox into which the <xu:tab>s are added). For some reason, the event isn't always dispatched (or doesn't bubble?). The handler is never called when i'm adding the second tab. and is _almost_ never called when i'm adding tabs in the background.

CCing Smaug, Boris and jst.

Comment 2

[Håkan Waara]

I think it would be less hackish if a new (internal?) XUL event was invented, a box-changed event, that would fire when a box changed size.

The scrollbox would watch that, and adjust its arrows accordingly.

Comment 3

[Asaf Romano (gone)]

s/in the scrollbox binding/in the arrowscrollbox binding

Comment 4

[Olli Pettay [:smaug][[e-mail]]]

(In reply to comment #1)
> I was trying to fix this by adding a DOMNodeInserted handler (phase=capturing)
> in the scrollbox binding (which contains the scrollbox-innerbox into which the
> <xu:tab>s are added). For some reason, the event isn't always dispatched (or
> doesn't bubble?). The handler is never called when i'm adding the second tab.
> and is _almost_ never called when i'm adding tabs in the background.
> 

I do get the DOMNodeInserted event every time, but the style of the right arrow button isn't updated.

Comment 5

[Asaf Romano (gone)]

Smaug: This turns out to be a xbl issue, it looks like my handler isn't attached at the same time the binding is applied (i.e. once I do get DOMNodeInserted dispatched, i do get it for each new tab); tested on windows (1.8.1).

Comment 6

[Asaf Romano (gone)]

I'm going to work around this in tabbrowser.xml and file a separate bug on something like comment 2.

Comment 7

[Asaf Romano (gone)]

Attachment: patch

This also fixes bug 338254

Comment 8

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

Comment on attachment 229944 [details] [diff] [review]
patch

r=sspitzer, but a couple nits (and questions)

1)  nit, _delaydUpdate -> _delayedUpdate

2)  can this temporary workaround function be made into a method so we don't have to duplicate the code (and the comment?)  also, please log a spin off bug on a non-temporary fix.
 
3)  we've discussed this in another bug, but the reason you are removing this TabFoo event hander is "they're being dispatched from the tabbrowser binding itself".  Should we log a bug on removing the TabSelect handler?

-      <handler event="TabClose"  action="this.adjustTabstrip(true);"/>

Comment 9

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

related to this code is also bug #342105 (Close button not shown if there is only one tab) which as a patch, but I have not review or tested yet.

I'll wait until after asaf lands this one to re-work that patch.

Comment 10

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

"(assuming scroll indicators stay)"

ben/mconnor/beltzner/pkasting, I'm under the impression that scroll indicators are staying.  is that the wrong assumption?

Comment 11

[Peter Kasting]

(In reply to comment #10)
> "(assuming scroll indicators stay)"
> 
> ben/mconnor/beltzner/pkasting, I'm under the impression that scroll indicators
> are staying.  is that the wrong assumption?

I have asked ben and beltzenr to use the current trunk patch and give their thoughts on this particular issue.  They have both told me they will do so.

Comment 12

[Asaf Romano (gone)]

(In reply to comment #8)
> (From update of attachment 229944 [details] [diff] [review] [edit])
> r=sspitzer, but a couple nits (and questions)
> 
> 1)  nit, _delaydUpdate -> _delayedUpdate

will do on checkin.

> 2)  can this temporary workaround function be made into a method so we don't
> have to duplicate the code (and the comment?)

I don't think it's worth it, because:
 * the setTimeout call, together with the that i'm passing different values to adjustTabstrip.
 * There are only two places which need this sort-of-hacky workaround.
 * I don't want to expose this as part of the tabbrowser API.

> also, please log a spin off bug on a non-temporary fix.

will do.

> 3)  we've discussed this in another bug, but the reason you are removing this
> TabFoo event hander is "they're being dispatched from the tabbrowser binding
> itself".  Should we log a bug on removing the TabSelect handler?
> 
> -      <handler event="TabClose"  action="this.adjustTabstrip(true);"/>

Yes, though TabSelect is still fired from the tabbox binding (see bug 343096); note we could move whatever it does now to the onselect handler.

Comment 13

[Asaf Romano (gone)]

Note to self: also file a bug on the DOMNodeInserted listener issue.

Comment 14

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

>  * the setTimeout call, together with the that i'm passing different values >to adjustTabstrip.

can't you do: 

setTimeout(_delayedUpdate, 0, this.mTabContainer, true);
setTimeout(_delayedUpdate, 0, this.mTabContainer, false);

and make your method take two arguments, aTabContainer and a boolean that you pass to adjustTabstrip?

Comment 15

[Asaf Romano (gone)]

As I noted somewhere else, we shouldn't add <method>s in which "this" doesn't point to the binding (this is always the case for callbacks).

Comment 16

[Asaf Romano (gone)]

With sspitzer agreement (on IRC), landed this on trunk without adding a general method (see comment 8, comment 12 and comment 14).

mozilla/toolkit/content/widgets/tabbrowser.xml 1.169

Comment 17

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

> As I noted somewhere else....

from https://bugzilla.mozilla.org/show_bug.cgi?id=343251#c146

> I don't see how "this" (in my original _handleAllTabsSelect() method) 
> points to something other than the binding.  Can you elaborate?

asaf wrote:

"When you're passing a callback method to methods like addEventListener and
setTimeout, "this" doesn't point to the js object in which the callback
function is scoped in (when the method is called)."

thanks for fixing this, asaf.

Comment 18

[Asaf Romano (gone)]

Filed bug 345399.

Comment 19

[Mike Connor [:mconnor]]

Comment on attachment 229944 [details] [diff] [review]
patch

a=mconnor on behalf of drivers for checkin to the 1.8 branch

Comment 20

[Asaf Romano (gone)]

1.8 branch: mozilla/toolkit/content/widgets/tabbrowser.xml 1.103.2.56