Closed Bug 955502 Opened 10 years ago Closed 10 years ago

Double and middle-clicks on the tab bar should open new conversation tab

Categories

(Instantbird Graveyard :: Conversation, defect)

defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: benediktp, Assigned: nhnt11)

References

Details

Attachments

(1 file, 6 obsolete files)

*** Original post on bio 2065 at 2013-07-24 22:06:00 UTC ***

At least on Windows a middle-click on the tab bar should open a new conversation tab. On other OS' it should behave as Firefox does there.
Blocks: 955452
*** Original post on bio 2065 at 2013-07-24 22:07:28 UTC ***

I forgot: double-clicks should do that too!
Summary: Middle-click on the tab bar should open new conversation tab → Double and middle-clicks on the tab bar should open new conversation tab
Attached patch Patch (obsolete) — Splinter Review
*** Original post on bio 2065 as attmnt 2633 at 2013-07-24 22:19:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354402 - Flags: review?(benediktp)
Assignee: nobody → nhnt11
Comment on attachment 8354402 [details] [diff] [review]
Patch

*** Original change on bio 2065 attmnt 2633 at 2013-07-24 22:20:42 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354402 - Attachment is patch: true
Attachment #8354402 - Attachment mime type: application/octet-stream → text/plain
Comment on attachment 8354402 [details] [diff] [review]
Patch

*** Original change on bio 2065 attmnt 2633 at 2013-07-24 22:40:13 UTC ***

>+                  onclick="if (event.target.localName != 'tabs') return;
>+                           if (event.button == 1) goDoCommand('cmd_newtab');"
>+                  ondblclick="if (event.target.localName != 'tabs') return;
>+                              goDoCommand('cmd_newtab')"

You'll need a check for the primary mouse button in the double click handler lest we trigger that on double right clicks.
Attachment #8354402 - Flags: review?(benediktp) → review-
*** Original post on bio 2065 as attmnt 2635 at 2013-07-24 22:42:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354404 - Flags: review?(benediktp)
Comment on attachment 8354402 [details] [diff] [review]
Patch

*** Original change on bio 2065 attmnt 2633 at 2013-07-24 22:42:13 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354402 - Attachment is obsolete: true
*** Original post on bio 2065 at 2013-07-25 11:09:04 UTC ***

Have you tried this patch? It doesn't work for me. goDoCommand doesn't seem to be defined from where you want to use it.
*** Original post on bio 2065 at 2013-07-25 17:39:25 UTC ***

(In reply to comment #5)
> Have you tried this patch? It doesn't work for me. goDoCommand doesn't seem to
> be defined from where you want to use it.

I just realized it works on Mac because the global overlay is included in menus.xul - which is included in the conversation window on Mac. I'll come up with a different solution for this.
Comment on attachment 8354404 [details] [diff] [review]
Handle only primary button double clicks

*** Original change on bio 2065 attmnt 2635 at 2013-08-02 08:33:33 UTC ***

This doesn't work on Windows as we've found out.
Attachment #8354404 - Flags: review?(benediktp) → review-
Attached patch Don't use goDoCommand (obsolete) — Splinter Review
*** Original post on bio 2065 as attmnt 2714 at 2013-08-14 01:58:00 UTC ***

This directly does Conversations.showNewTab().
Attachment #8354483 - Flags: review?(benediktp)
Comment on attachment 8354404 [details] [diff] [review]
Handle only primary button double clicks

*** Original change on bio 2065 attmnt 2635 at 2013-08-14 01:58:27 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354404 - Attachment is obsolete: true
*** Original post on bio 2065 as attmnt 2715 at 2013-08-14 02:04:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354484 - Flags: review?(benediktp)
Comment on attachment 8354483 [details] [diff] [review]
Don't use goDoCommand

*** Original change on bio 2065 attmnt 2714 at 2013-08-14 02:04:34 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354483 - Attachment is obsolete: true
Attachment #8354483 - Flags: review?(benediktp)
Comment on attachment 8354484 [details] [diff] [review]
Use existing click/dblclick event handlers

*** Original change on bio 2065 attmnt 2715 at 2013-08-23 16:25:12 UTC ***

06:19:24 PM - aleth: nhnt11: what does the existing e.initEvent("NewTab", true, true); do?
06:19:56 PM - nhnt11: When there are enough tabs open that the close button is shown only for the focused one, the labels for the other tabs are centered as if the close button isn't there. this causes the whole label to shift to the left when the tab is focused and the close button appears :/
06:20:05 PM - flo-retina: aleth: it's an even that add-ons can catch to know a new tab's been opened
06:20:28 PM - aleth: Oh, so we currently dispatch that and just don't actually do it?
06:20:36 PM - nhnt11: Yeah
06:20:43 PM - aleth: nhnt11: Shouldn't that be sent from showNewTab() ?
06:20:56 PM - nhnt11: Hmm, that makes sense.
06:21:15 PM - nhnt11: Or even from the constructor of the newtab binding
06:21:24 PM - aleth: onTabClick needs a comment explaining which button is checked for, similarly the other handler
06:21:41 PM - aleth: Or define selfexplanatory constants for "1" and "0"
06:22:09 PM - aleth: nhnt11: My point is that it doesn't seem to be sent consistently
06:22:26 PM - aleth: nhnt11: There's also an xxx there that needs addressing one way or another
06:22:56 PM - nhnt11: aleth: I didn't really understand that xxx
06:23:06 PM - flo-retina: nhnt11: well, the code may be trivial; once you have a great idea
06:23:12 PM - flo-retina: nhnt11: I think Firefox has the same bug though
06:23:41 PM - nhnt11: aleth: From my testing, the code was getting called only when I double clicked the empty area anyway
If that's really the case, remove the xxx ;)
Attachment #8354484 - Flags: review?(benediktp) → review-
Attached patch Address comments (obsolete) — Splinter Review
*** Original post on bio 2065 as attmnt 2765 at 2013-08-23 17:59:00 UTC ***

Removed NewTab event and hack note as per IRC discussion.
Attachment #8354534 - Flags: review?(aleth)
Comment on attachment 8354484 [details] [diff] [review]
Use existing click/dblclick event handlers

*** Original change on bio 2065 attmnt 2715 at 2013-08-23 17:59:28 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354484 - Attachment is obsolete: true
Comment on attachment 8354534 [details] [diff] [review]
Address comments

*** Original change on bio 2065 attmnt 2765 at 2013-08-23 18:04:27 UTC ***

>             if (!this._blockDblClick && aEvent.button == 0 &&
>                 aEvent.originalTarget.localName == "box") {
>-              // xxx this needs to check that we're in the empty area of the tabstrip
>-              var e = document.createEvent("Events");
>-              e.initEvent("NewTab", true, true);
>-              this.dispatchEvent(e);
>+              Conversations.showNewTab();
>             }

Nit: you no longer need the {}.

Have you checked that double-clicking elements (scrollarrows etc) when the tabstrip is in overflow mode doesn't trigger the newtab?
Attachment #8354534 - Flags: review?(aleth) → review-
Attached patch Address review comments (obsolete) — Splinter Review
*** Original post on bio 2065 as attmnt 2766 at 2013-08-23 18:11:00 UTC ***

New tabs were indeed being opened when middle clicking the arrow scrollbox arrows and so on, thanks for catching that.
Attachment #8354535 - Flags: review?(aleth)
Comment on attachment 8354534 [details] [diff] [review]
Address comments

*** Original change on bio 2065 attmnt 2765 at 2013-08-23 18:11:48 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354534 - Attachment is obsolete: true
Comment on attachment 8354535 [details] [diff] [review]
Address review comments

*** Original change on bio 2065 attmnt 2766 at 2013-08-23 18:12:39 UTC ***

Thanks!
Attachment #8354535 - Attachment is patch: true
Attachment #8354535 - Attachment mime type: application/octet-stream → text/plain
Attachment #8354535 - Flags: review?(aleth) → review+
Comment on attachment 8354535 [details] [diff] [review]
Address review comments

*** Original change on bio 2065 attmnt 2766 at 2013-08-23 18:14:22 UTC ***

>-        <parameter name="event"/>
>+        <parameter name="aEvent"/>
>             event.stopPropagation();

needs to change too.
Attachment #8354535 - Flags: review+ → review-
*** Original post on bio 2065 as attmnt 2767 at 2013-08-23 18:17:00 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354536 - Flags: review?(aleth)
Comment on attachment 8354535 [details] [diff] [review]
Address review comments

*** Original change on bio 2065 attmnt 2766 at 2013-08-23 18:17:51 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354535 - Attachment is obsolete: true
Comment on attachment 8354536 [details] [diff] [review]
Change "event" to "aEvent"

*** Original change on bio 2065 attmnt 2767 at 2013-08-23 18:18:43 UTC was without comment, so any subsequent comment numbers will be shifted ***
Attachment #8354536 - Flags: review?(aleth) → review+
Whiteboard: [checkin-needed]
*** Original post on bio 2065 at 2013-08-23 21:46:12 UTC ***

http://hg.instantbird.org/instantbird/rev/c1ced2446f94
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [checkin-needed]
Target Milestone: --- → 1.5
You need to log in before you can comment on or make changes to this bug.