322898 - Add events for watching the addition and removal of tabs

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, defect, P2)

Description

[Asaf Romano (gone)]

The current tabbrowser and tabbox bindings do not provide any supported way of watching the addition/removal of tabs (except DOMNodeInserted/Removed events). We should probably fire some events in the addTab and removeTab methods.

Comment 1

[Brian Ryner (not reading)]

I have a patch for this.

Comment 2

[Brian Ryner (not reading)]

Attachment: patch

This adds events for tab creation, removal, and reordering (newtab, closetab, and movetab, respectively).  The event target is the tab that's affected.  For the movetab event, event.detail gives the old index of the tab.

Comment 3

[Asaf Romano (gone)]

Brian, there's already a "NewTab" event for the new-empty-tab case:
http://lxr.mozilla.org/seamonkey/source/toolkit/content/widgets/tabbrowser.xml#1428

Comment 4

[Nickolay_Ponomarev]

Would it make sense to allow the closetab listener cancel the operation?

Comment 5

[Ben Goodger (use ben at mozilla dot org for email)]

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

Can I get you to rename these to tab-open, tab-close and add a similar event for tab-select? (for completeness, currently to get select events you need to attach a select handler directly to mTabContainer, to avoid getting select events from web content).

Comment 6

[Brian Ryner (not reading)]

Attachment: patch

Now with better event names.  You might notice the reversal of the false and true arguments for initializing the "TabSelect" event vs. the "select" event.  The arguments for the select event don't make a lot of sense to me (doesn't bubble, but can cancel? cancelling it does nothing).  But to not break compatibility, I just left it and made the new event work in a sane way.

Comment 7

[Mike Connor [:mconnor]]

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

I'm pretty sure we want the event on the tabs, not the tabpanels... Aside from that I've bitrotted a bunch of this, so I'm going to roll it into my soon-to-be-attached patch for reopening closed tabs

Comment 8

[Brian Ryner (not reading)]

ok, sounds good.

Comment 9

[Dave Townsend [:mossop]]

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

Comment 10

[Mike Connor [:mconnor]]

load-balancing tabbrowser stuff to sspitzer

Comment 11

[Mike Connor [:mconnor]]

gah.  s/.org/.com/!

Comment 12

[[e-mail]]

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

>Index: tabbox.xml
I would have thought that all these new events are tabbrowser-specific and should therefore not be fired here.

Comment 13

[Chris Thomas (CTho) [formerly [e-mail] [e-mail]]]

(In reply to comment #12)
> (From update of attachment 211633 [details] [diff] [review] [edit])
> >Index: tabbox.xml
> I would have thought that all these new events are tabbrowser-specific and
> should therefore not be fired here.
> 

Seems like the kind of stuff that superreview is intended for.

Comment 14

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

accepting.  increasing swag though, because I'm no mconnor!

this blocks the undoclosetab that dietrich is working on, so I plan to get to this right away.

Comment 15

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

finally getting to this (sorry for the delay, dietrich!)

mconnor wrote (over email):

"there should be events for TabClose/TabOpen/TabSelect/TabMove added, and get the change to the tabbrowser-tabs binding that listens for DOMNodeRemoved/DOMNodeAdded while you're there"

working on that now.

Comment 16

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

Attachment: initial patch based on earlier patches from mconnor

Comment 17

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

"there should be events for TabClose/TabOpen/TabSelect/TabMove added, and get the change to the tabbrowser-tabs binding that listens for DOMNodeRemoved/DOMNodeAdded while you're there"

s/DOMNodeAdded/DOMNodeInserted

some questions:

1)

I'm having a problem with the DOMNodeInserted change from mconnor's previous patch (it breaks open group as tabs).

2)  in one of mconnor's earlier patches, he changed "spacer" to "box", but that broke open in a new tab.

+              // changing this to box breaks double clicking
+              // on the spacer to open a new tab
                 aEvent.originalTarget.localName == "spacer") {

3)

finally, the 2nd and 3rd params to initEvent() are for canBubble and cancelable, and I've done true, false (following mconnor's lead).  this makes sense to me, but I wanted to double check with mconnor. 

Comment 18

[[e-mail]]

(In reply to comment #17)
>finally, the 2nd and 3rd params to initEvent() are for canBubble and
>cancelable, and I've done true, false (following mconnor's lead).  this makes
>sense to me, but I wanted to double check with mconnor. 
Triple-checking with Smaug ;-)

(From update of attachment 224502 [details] [diff] [review])
>Index: tabbox.xml
>+            // Support both the old "select" event and the new, better-named
>+            // "TabSelect" event.
Surely the TabSelect event belongs in tabbrowser.xml with the other events?

Comment 19

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

Comment on attachment 224502 [details] [diff] [review]
initial patch based on earlier patches from mconnor

>+
>+          var evt = document.createEvent("UIEvents");
>+          evt.initUIEvent("TabMove", true, false, window, oldPosition);
>+          aTab.dispatchEvent(evt);

I guess this is ok, but it would be great if someone adds some documentation about these
events.


(In reply to comment #17)
>finally, the 2nd and 3rd params to initEvent() are for canBubble and
>cancelable, and I've done true, false (following mconnor's lead).  this makes
>sense to me, but I wanted to double check with mconnor. 

Makes sense to me, because the return value of dispatchEvent is not used.

Comment 20

[Mike Connor [:mconnor]]

TabSelect is useful to non-tabbrowser consumers of tabbox.  If we move dnd reordering to to tabbox, as has been requested, we'd move TabMove there too, IMO.

Comment 21

[[e-mail]]

(In reply to comment #20)
>TabSelect is useful to non-tabbrowser consumers of tabbox.
One event isn't enough for them?

Comment 22

[[e-mail]]

Actually two, as both tabs and panels like to fire select events.

Comment 23

[Dave Townsend [:mossop]]

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

Comment 24

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

Comment on attachment 224502 [details] [diff] [review]
initial patch based on earlier patches from mconnor

obsolete, but this patch has landed (on the trunk) with the patch for bug #318168

Comment 25

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

the new events have landed with the patch in #318168 (on the trunk at least).

Comment 26

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

marking fixed1.8.1 by bug 318168

Comment 27

[John Mellor (Jomel)]

Probably a bit late now, but might it make sense to have a cancelable TabClose event, followed by a non-cancelable TabUnload event as (AFAIK) happens for windows?