604098 - Double click anywhere to create a new tab

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P2)

Description

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

After discussing my issues with the UX around creating a new tab in an empty group, the idea of double clicking anywhere (in a group, I think) to open a new tab was brought up. I like it, Aza seems to like it, Ian seemed to like to. Let it be done!

Another benefit: it makes the experience more similar to the tab strip, where double clicking opens a new blank tab.

To think about:
* should the new tab be focused?
* double clicking outside of a group... perhaps that should open a new group?

Comment 1

[Aza Raskin [:aza]]

It should work this way:

* Double clicking inside a group should immediately create a new tab as close to current location of the cursor is as allowed by the group arranger and then zoom into that tab. The trick is to keep it as fast as the new-tab button (at least since after we went to the much faster animation)
* Double clicking in non-group space should create an orphaned tab right there, and then zoom into the new tab.

Comment 3

[Benjamin Stover (:stechz)]

Double clicking is nice, but not as discoverable and it can be frustrating trying to figure out how to start a new workspace (I assume this would be a pretty common use case).

I'm not sure this is the proper place to discuss, but it would be nice if the new window button were more noticeable when there are no tabs inside it. It could even be big and centered, and transition down to the bottom left corner if tabs are dragged over or the new tab button is created.

Comment 4

[Kevin Hanes]

(In reply to comment #3)

> I'm not sure this is the proper place to discuss, but it would be nice if the
> new window button were more noticeable when there are no tabs inside it. It
> could even be big and centered, and transition down to the bottom left corner
> if tabs are dragged over or the new tab button is created.

The place to discuss, is in fact bug 597767! Feel free to join the discussion :)

Comment 5

[Luis Villa [Outside Counsel; for non-law use [e-mail]]]

(In reply to comment #0)
> Another benefit: it makes the experience more similar to the tab strip, where
> double clicking opens a new blank tab.

Note that, AFAICT, this behavior doesn't work in Windows 7. (Does work here in Mac and Linux.) Not sure if that is expected or if it is a bug, but does suggest that it might not be similar to the tab strip for many users. 

(I'd also note that I've been using Moz/ffox with tabs since roughly forever, on Linux, and have never discovered this feature before, which I think speaks to Benjamin's point about discoverability. But ctl+t has been burned into my head nearly as long as tabs have been around, so take with a grain of salt.)

Comment 6

[Aza Raskin [:aza]]

Im not worried about discoverability as we already have a visual affordance and when we move to the new new tab experience that will be further increased.

Enough people have asked for the double-click gesture that power users will discover it if they try, but no one is harmed by the not knowing.

Comment 7

[Anant Narayanan [:anant]]

Attachment: Dblclick Patch v1

It seems like we try to discourage users from creating orphaned tabs as much as possible. This patch creates a new group with one tab in it on double-clicking in empty space, and adds a new tab to a group if the double click was inside one. In both cases, the newly created tab is zoomed in on immediately.

If we really do want an orphaned tab, that should be easy to change.

Comment 8

[Ian Gilman [:iangilman]]

For what it's worth, I'm not convinced double clicking in the background should do anything (though I don't have any compelling argument against it). If it does do something, however, it should probably just create an orphan tab. Aza?

Comment 9

[Ian Gilman [:iangilman]]

Comment on attachment 487644 [details] [diff] [review]
Dblclick Patch v1

>-        if (e.originalTarget.id == "content")
>-          self._createGroupItemOnDrag(e)
>+        if (e.originalTarget.id == "content") {
>+          self._createGroupItemOnDrag(e);
>+        
>+          // Create a group with just one tab on double click
>+          if (Date.now() - self._lastClick <= self._DBLCLICK_INTERVAL) {

Maybe this should be in a click handler instead of attached to the mousedown handler? I suppose it doesn't matter exactly, just seems cleaner to me (and more consistent with the GroupItem version).

>+            // 21 * 2 = 42
>+            let box = new Rect();
>+            box.left = e.clientX - 21;
>+            box.right = e.clientX + 21;
>+            box.top = e.clientY - 21;
>+            box.bottom = e.clientY + 21;
>+
>+            let grp = new GroupItem(null, {bounds: box, immediately: true});

Does this do the right thing if you click near the edge of the window, or does the newly created group hang off the edge? I guess it doesn't matter if we're killing the group anyway (pending Aza's feedback), but then a similar thing may apply to the newly created tab.

F+ with the above items addressed.

Comment 10

[Aza Raskin [:aza]]

Re comment 8: Double clicking in empty space should open an orphaned tab.

Comment 11

[Ian Gilman [:iangilman]]

(In reply to comment #10)
> Re comment 8: Double clicking in empty space should open an orphaned tab.

Ok, Anant, please make it an orphan tab rather than a tab in a group. 

Aza, is that your UI review? If so, please flip the flag on the patch from ? to -

Comment 12

[Anant Narayanan [:anant]]

Attachment: Dblclick Patch v2

Updated patch to make orphan tab instead of a group. Ready to land!

Comment 13

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

(In reply to comment #12)
Ready to land!

Not yet! Still needs to be reviewed!

Comment 14

[Anant Narayanan [:anant]]

Attachment: Dblclick patch v3

I sent this patch to the tryserver: http://hg.mozilla.org/try/rev/ae18308c3137

No test breakages (there were a few oranges but they were intermittent/timeout failures).

Comment 15

[Ian Gilman [:iangilman]]

Comment on attachment 489951 [details] [diff] [review]
Dblclick patch v3

>+            let box = new Rect();
>+            box.left = e.clientX;
>+            box.right = e.clientX;
>+            box.top = e.clientY;
>+            box.bottom = e.clientY;
>+
>+            // There's no easy way to create an orphan tab
>+            // We create a dummy group with a tab and remove it immediately
>+            let grp = new GroupItem(null, {bounds: box, immediately: true});
>+            grp.newTab();
>+            grp.removeAll();

Are there any problems caused by creating a zero area group?  Do you see any errors on the console, for instance?  Does the tab look the right size when it comes out?  If it doesn't cause any problems, I'll be delighted (if a little surprised).  

This patch should have a test, though.  Search for EventUtils.sendMouseEvent in:

/browser/base/content/test/tabview/

...to get started.

Comment 16

[Kevin Hanes]

Reassigning to Raymond.

Comment 17

[Raymond Lee [:raymondlee]]

Attachment: v4

Comment 18

[Raymond Lee [:raymondlee]]

(In reply to comment #17)
> Created attachment 496564 [details] [diff] [review]
> v4
Passed try!

Comment 19

[Kevin Hanes]

Moving to b9

Comment 20

[Ian Gilman [:iangilman]]

Comment on attachment 496564 [details] [diff] [review]
v4

Raymond, I realize you inherited this patch, and I feel like some of the comments I have are probably just things I over looked before; if that's so, my apologies. I like your solution to the orphan tab thing (rather than creating a blank group and then deleting it), btw.

>   defaultName: tabviewString('groupItem.defaultName'),
>-
>+  

This line adds trailing spaces; please fix. 

>-
>+  _lastClickPositions: null,
>   // ----------
>   // Function: _addHandlers
>   // Helper routine for the constructor; adds various event handlers to the container.
>   _addHandlers: function GroupItem__addHandlers(container) {

Rather than introducing _lastClickPositions at this point in the file, please initialize it in the constructor with all the other variables.

>+      if (Date.now() - self._lastClick <= UI._DBLCLICK_INTERVAL &&
>+          self._lastClickPositions.x == e.clientX && 
>+          self._lastClickPositions.y == e.clientY) {

We need some slop here on the x and y... it should still count as a double click if the mouse has moved, just not very much. Let's say a combined delta of 5 pixels? Make that another constant like _DBLCLICK_INTERVAL and use it in both GroupItem and UI. 

>+        self._lastClickPositions = { x: e.clientX, y: e.clientY };

Might as well make it a Point?

>+  // Variable: _lastClick
>+  // Keeps tracks of the time of last click event to detect double click.
>+  // Used to create tabs on double-click since we cannot attach 'dblclick'

Nit: "Keeps tracks" should be "Keeps track".

>-        if (e.originalTarget.id == "content")
>-          self._createGroupItemOnDrag(e)
>+        if (e.originalTarget.id == "content") {
>+          self._createGroupItemOnDrag(e);
>+
>+          // Create an orphan tab on double click
>+          if (Date.now() - self._lastClick <= self._DBLCLICK_INTERVAL && 

Should only call _createGroupItemOnDrag if there was no double click. Otherwise it's possible to double click and drag out a group all in the same motion. 

>+            let box = 
>+              new Rect(e.clientX, e.clientY, TabItems.tabWidth,
>+                       TabItems.tabHeight);
>+            newTab.tabItem.setBounds(box, true);

Presumably this creates a tab whose upper left hand corner is on the mouse click location. Instead please make the tab centered on the click.

Comment 21

[Ian Gilman [:iangilman]]

By the way, you can assign this to me for review (with your next patch); it's only patches that extend outside the Panorama iFrame that I want a second opinion on.

Comment 22

[Raymond Lee [:raymondlee]]

Attachment: v4

(In reply to comment #20)
> Comment on attachment 496564 [details] [diff] [review]
> v4
> 
> Raymond, I realize you inherited this patch, and I feel like some of the
> comments I have are probably just things I over looked before; if that's so, my
> apologies. I like your solution to the orphan tab thing (rather than creating a
> blank group and then deleting it), btw.
> 
> >   defaultName: tabviewString('groupItem.defaultName'),
> >-
> >+  
> 
> This line adds trailing spaces; please fix. 

Fixed

> 
> >-
> >+  _lastClickPositions: null,
> >   // ----------
> >   // Function: _addHandlers
> >   // Helper routine for the constructor; adds various event handlers to the container.
> >   _addHandlers: function GroupItem__addHandlers(container) {
> 
> Rather than introducing _lastClickPositions at this point in the file, please
> initialize it in the constructor with all the other variables.

Fixed

> 
> >+      if (Date.now() - self._lastClick <= UI._DBLCLICK_INTERVAL &&
> >+          self._lastClickPositions.x == e.clientX && 
> >+          self._lastClickPositions.y == e.clientY) {
> 
> We need some slop here on the x and y... it should still count as a double
> click if the mouse has moved, just not very much. Let's say a combined delta of
> 5 pixels? Make that another constant like _DBLCLICK_INTERVAL and use it in both
> GroupItem and UI. 

Fixed

> 
> >+        self._lastClickPositions = { x: e.clientX, y: e.clientY };
> 
> Might as well make it a Point?

Use Point instead.

> 
> >+  // Variable: _lastClick
> >+  // Keeps tracks of the time of last click event to detect double click.
> >+  // Used to create tabs on double-click since we cannot attach 'dblclick'
> 
> Nit: "Keeps tracks" should be "Keeps track".
> 

Fixed

> >-        if (e.originalTarget.id == "content")
> >-          self._createGroupItemOnDrag(e)
> >+        if (e.originalTarget.id == "content") {
> >+          self._createGroupItemOnDrag(e);
> >+
> >+          // Create an orphan tab on double click
> >+          if (Date.now() - self._lastClick <= self._DBLCLICK_INTERVAL && 
> 
> Should only call _createGroupItemOnDrag if there was no double click. Otherwise
> it's possible to double click and drag out a group all in the same motion.

Added it instead the else block.
 
> 
> >+            let box = 
> >+              new Rect(e.clientX, e.clientY, TabItems.tabWidth,
> >+                       TabItems.tabHeight);
> >+            newTab.tabItem.setBounds(box, true);
> 
> Presumably this creates a tab whose upper left hand corner is on the mouse
> click location. Instead please make the tab centered on the click.

Done.

Comment 23

[Raymond Lee [:raymondlee]]

(In reply to comment #22)
> Created attachment 497206 [details] [diff] [review]
> v4
> 

Passed Try!

Comment 24

[Ian Gilman [:iangilman]]

Comment on attachment 497206 [details] [diff] [review]
v4

Looks good. Two minor points:

>+  // Constant: DBLCLICK_OFFSET
>+  // Defines the offset (in pixels) between two clicks for it to count as
>+  // a double click.

Nit: Add the word "maximum" ("Defines the maximum offset...").

>+          } else {
>+            self._lastClick = Date.now();
>+            self._lastClickPositions = new Point(e.clientX, e.clientY);
>+	          self._createGroupItemOnDrag(e);

Nit: there's a tab character at the beginning of that bottom line.

Comment 25

[Raymond Lee [:raymondlee]]

Attachment: v4

Fixed the two minor points.

r=ian

Comment 26

[Raymond Lee [:raymondlee]]

It would be great if someone can give approval2.0 to this.

Comment 27

[Paul O'Shannessy [:zpao] (not bugmail, email directly)]

CCing a couple people who could give approval.

Comment 28

[Raymond Lee [:raymondlee]]

Should wait for bug 619937 to be landed before applying this patch

Comment 29

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Comment 30

[Kevin Hanes]

bugspam (moving b9 to b10)

Comment 31

[Kevin Hanes]

bugspam (removing b9)

Comment 32

[Raymond Lee [:raymondlee]]

Passed Try

Comment 33

[Raymond Lee [:raymondlee]]

Attachment: Patch for check-in

Updated tab.tabItem to tab._tabViewTabItem

Comment 34

[Ian Gilman [:iangilman]]

http://hg.mozilla.org/mozilla-central/rev/8b54bbf7d3a0

Comment 35

[Tracy Walker [:tracy]]

verified with nightly build of minefield.