Closed
Bug 955502
Opened 11 years ago
Closed 11 years ago
Double and middle-clicks on the tab bar should open new conversation tab
Categories
(Instantbird Graveyard :: Conversation, defect)
Instantbird Graveyard
Conversation
Tracking
(Not tracked)
RESOLVED
FIXED
1.5
People
(Reporter: benediktp, Assigned: nhnt11)
References
Details
Attachments
(1 file, 6 obsolete files)
(deleted),
patch
|
aleth
:
review+
|
Details | Diff | Splinter Review |
*** 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.
Reporter | ||
Comment 1•11 years ago
|
||
*** 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
Assignee | ||
Comment 2•11 years ago
|
||
*** 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 | ||
Updated•11 years ago
|
Assignee: nobody → nhnt11
Assignee | ||
Comment 3•11 years ago
|
||
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
Reporter | ||
Comment 4•11 years ago
|
||
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-
Assignee | ||
Comment 5•11 years ago
|
||
*** 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)
Assignee | ||
Comment 6•11 years ago
|
||
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
Reporter | ||
Comment 7•11 years ago
|
||
*** 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.
Assignee | ||
Comment 8•11 years ago
|
||
*** 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.
Reporter | ||
Comment 9•11 years ago
|
||
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-
Assignee | ||
Comment 10•11 years ago
|
||
*** 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)
Assignee | ||
Comment 11•11 years ago
|
||
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
Assignee | ||
Comment 12•11 years ago
|
||
*** 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)
Assignee | ||
Comment 13•11 years ago
|
||
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 14•11 years ago
|
||
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-
Assignee | ||
Comment 15•11 years ago
|
||
*** 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)
Assignee | ||
Comment 16•11 years ago
|
||
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 17•11 years ago
|
||
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-
Assignee | ||
Comment 18•11 years ago
|
||
*** 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)
Assignee | ||
Comment 19•11 years ago
|
||
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 20•11 years ago
|
||
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 21•11 years ago
|
||
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-
Assignee | ||
Comment 22•11 years ago
|
||
*** 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)
Assignee | ||
Comment 23•11 years ago
|
||
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 24•11 years ago
|
||
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+
Updated•11 years ago
|
Whiteboard: [checkin-needed]
Comment 25•11 years ago
|
||
*** Original post on bio 2065 at 2013-08-23 21:46:12 UTC ***
http://hg.instantbird.org/instantbird/rev/c1ced2446f94
Status: NEW → RESOLVED
Closed: 11 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.
Description
•