Closed Bug 663612 Opened 13 years ago Closed 13 years ago

clicking a group should zoom into the group's active tab

Categories

(Firefox Graveyard :: Panorama, defect)

Product:
Firefox Graveyard
Component:
Panorama
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED
Milestone:
Firefox 8

People

(Reporter: ttaubert, Assigned: raymondlee)

References

Details

Attachments

(1 file, 7 obsolete files)

v8
9.14 KB, patch
sdwilsh
: review+
limi
: ui-review+
ttaubert
: feedback+
Details | Diff | Splinter Review 
* drag/drop of groups is still supported (on the whole container)
* when clicking an empty group create a new tab and zoom into it

https://wiki.mozilla.org/Simplify_Panorama_UI
Blocks: 663613
Blocks: 663614
(In reply to comment #0)
> * drag/drop of groups is still supported (on the whole container)

Is that user can drag and drop a group using the group titlebar area?

> * when clicking an empty group create a new tab and zoom into it

When clicking on a non-empty group, do we create a new tab and zoom into in as well?

> 
> https://wiki.mozilla.org/Simplify_Panorama_UI
Blocks: 663611
(In reply to comment #1)
> (In reply to comment #0)
> > * drag/drop of groups is still supported (on the whole container)
> 
> Is that user can drag and drop a group using the group titlebar area?

I mean: we want to keep the current drag/drop behavior so that the user can drag groups around by clicking the container or the titlebar. If we detect a single click and dragging hasn't started yet then we'll zoom in.

> > * when clicking an empty group create a new tab and zoom into it
> 
> When clicking on a non-empty group, do we create a new tab and zoom into in
> as well?

No, when clicking a non-empty group we just zoom into the group's activeTab (or just the first if non is active somehow).
Attached patch v1 (obsolete) — — Splinter Review 
Please apply the patch for Bug 663614 before this one.
Assignee: nobody → raymond
Status: NEW → ASSIGNED
Attachment #539152 - Flags: feedback?(tim.taubert)
Comment on attachment 539152 [details] [diff] [review]
v1

Review of attachment 539152 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/tabview/groupitems.js
@@ +1645,5 @@
> +      if (!self.isDragging) {
> +        if (time > 0)
> +          setTimeout(function() { 
> +            shouldHandleAsClick(time - self._CHECK_HANDLE_AS_CLICK_INTERVAL, callback)
> +          }, self._CHECK_HANDLE_AS_CLICK_INTERVAL);

I don't think we should use setTimeout() to determine if this is a single click. We should use the same approach as with the groupItem title shields (just check the last mousedown target and .isDragging).

https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabview/groupitems.js#211

::: browser/base/content/test/tabview/browser_tabview_bug663612.js
@@ +17,5 @@
> +  showTabView(function() {
> +    contentWindow = TabView.getContentWindow();
> +    groupItem = createEmptyGroupItem(contentWindow, 300, 300, 200);
> +
> +    testMouseClickOnEmptyGroupItem();

Please use waitForFocus() before starting with all those tests that use EventUtils.

@@ +62,5 @@
> +      groupItem.removeSubscriber(groupItem, "close");
> +      groupItem = null;
> +      finish();
> +    });
> +    groupItem.destroy({ immediately: true });

"closeGroupItem(groupItem, finish)" is much simpler here.
Attachment #539152 - Flags: feedback?(tim.taubert) → feedback-
> > +  showTabView(function() {
> > +    contentWindow = TabView.getContentWindow();
> > +    groupItem = createEmptyGroupItem(contentWindow, 300, 300, 200);
> > +
> > +    testMouseClickOnEmptyGroupItem();
> 
> Please use waitForFocus() before starting with all those tests that use
> EventUtils.

Why doesn't showTabView call waitForFocus (via whenTabViewIsShown)?
(In reply to comment #5)
> > Please use waitForFocus() before starting with all those tests that use
> > EventUtils.
> 
> Why doesn't showTabView call waitForFocus (via whenTabViewIsShown)?

That would be a useful addition so, Raymond, feel free to add this to head.js :)
Attached patch v2 (obsolete) — — Splinter Review 
(In reply to comment #4)
> Comment on attachment 539152 [details] [diff] [review] [review]
> v1
> 
> Review of attachment 539152 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/tabview/groupitems.js
> @@ +1645,5 @@
> > +      if (!self.isDragging) {
> > +        if (time > 0)
> > +          setTimeout(function() { 
> > +            shouldHandleAsClick(time - self._CHECK_HANDLE_AS_CLICK_INTERVAL, callback)
> > +          }, self._CHECK_HANDLE_AS_CLICK_INTERVAL);
> 
> I don't think we should use setTimeout() to determine if this is a single
> click. We should use the same approach as with the groupItem title shields
> (just check the last mousedown target and .isDragging).
> 
> https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabview/
> groupitems.js#211

Thanks, I missed that part.

> 
> ::: browser/base/content/test/tabview/browser_tabview_bug663612.js
> @@ +17,5 @@
> > +  showTabView(function() {
> > +    contentWindow = TabView.getContentWindow();
> > +    groupItem = createEmptyGroupItem(contentWindow, 300, 300, 200);
> > +
> > +    testMouseClickOnEmptyGroupItem();
> 
> Please use waitForFocus() before starting with all those tests that use
> EventUtils.

Added it to the head.js

> 
> @@ +62,5 @@
> > +      groupItem.removeSubscriber(groupItem, "close");
> > +      groupItem = null;
> > +      finish();
> > +    });
> > +    groupItem.destroy({ immediately: true });
> 
> "closeGroupItem(groupItem, finish)" is much simpler here.

Done
Attachment #539152 - Attachment is obsolete: true
Attachment #539317 - Flags: feedback?(tim.taubert)
Comment on attachment 539317 [details] [diff] [review]
v2

Review of attachment 539317 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/tabview/groupitems.js
@@ +1642,5 @@
> +      let target = e.target;
> +      if (Utils.isLeftClick(e) && self.$closeButton[0] != target &&
> +          self.$ntb[0] != target && self.$titlebar[0] != target &&
> +          !self.$titlebar.contains(target))
> +        lastMouseDownTarget = target;

Please set lastMouseDownTarget to null here if the condition is not met. We could receive two mousedowns here without a mouseup.

::: browser/base/content/test/tabview/browser_tabview_bug663612.js
@@ +56,5 @@
> +  whenTabViewIsHidden(function() {
> +    is(groupItem.getChildren().length, 1, "The group item still contains one tab item");
> +
> +    closeGroupItem(groupItem, function() {
> +      delete groupItem;

We can't delete a variable declared with let - and we don't need to.

::: browser/base/content/test/tabview/head.js
@@ +131,5 @@
>  function showTabView(callback, win) {
>    win = win || window;
>  
>    if (win.TabView.isVisible()) {
>      callback();

Please add waitForFocus() here, too.
Attachment #539317 - Flags: feedback?(tim.taubert) → feedback+
Attached patch v3 (obsolete) — — Splinter Review 
(In reply to comment #8)
> Comment on attachment 539317 [details] [diff] [review] [review]
> v2
> 
> Review of attachment 539317 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/tabview/groupitems.js
> @@ +1642,5 @@
> > +      let target = e.target;
> > +      if (Utils.isLeftClick(e) && self.$closeButton[0] != target &&
> > +          self.$ntb[0] != target && self.$titlebar[0] != target &&
> > +          !self.$titlebar.contains(target))
> > +        lastMouseDownTarget = target;
> 
> Please set lastMouseDownTarget to null here if the condition is not met. We
> could receive two mousedowns here without a mouseup.

Added

> 
> ::: browser/base/content/test/tabview/browser_tabview_bug663612.js
> @@ +56,5 @@
> > +  whenTabViewIsHidden(function() {
> > +    is(groupItem.getChildren().length, 1, "The group item still contains one tab item");
> > +
> > +    closeGroupItem(groupItem, function() {
> > +      delete groupItem;
> 
> We can't delete a variable declared with let - and we don't need to.

Removed and updated registerCleanupFunction since we need to determine whether the groupItem still exists or not at that point.

> 
> ::: browser/base/content/test/tabview/head.js
> @@ +131,5 @@
> >  function showTabView(callback, win) {
> >    win = win || window;
> >  
> >    if (win.TabView.isVisible()) {
> >      callback();
> 
> Please add waitForFocus() here, too.

Added
Attachment #539317 - Attachment is obsolete: true
Attachment #539422 - Flags: review?(dietrich)
Comment on attachment 539422 [details] [diff] [review]
v3

>+    if (createdGroupItem)
>+      closeGroupItem(createdGroupItem, function() {});
>+    hideTabView(function() {});

Apparently closeGroupItem's and hideTabView's callbacks should be optional.
(In reply to comment #10)
> Apparently closeGroupItem's and hideTabView's callbacks should be optional.

Yep, I thought the same when reading this. Filed bug 664379.
Passed Try
http://tbpl.mozilla.org/?tree=Try&rev=8543cfe8ca3a

There is an intermittent orange but not related to this patch.
Sorry I'm late to this party, but I just want to point out that we originally had this feature (you could click anywhere in a group to go into its active tab), and we removed it because it was too easy to hit accidentally, and it's a pretty extreme action to have happen when you're not expecting it.
Comment on attachment 539422 [details] [diff] [review]
v3

Review of attachment 539422 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/base/content/tabview/groupitems.js
@@ +1641,5 @@
> +    container.mousedown(function(e) {
> +      let target = e.target;
> +      if (Utils.isLeftClick(e) && self.$closeButton[0] != target &&
> +          self.$ntb[0] != target && self.$titlebar[0] != target &&
> +          !self.$titlebar.contains(target))

that's a bunch of conditions. i'd like to see a comment here summarizing.
Attachment #539422 - Flags: review?(dietrich) → review+
Attached patch Patch for checkin (obsolete) — — Splinter Review 
(In reply to comment #14)
> Comment on attachment 539422 [details] [diff] [review] [review]
> v3
> 
> Review of attachment 539422 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/base/content/tabview/groupitems.js
> @@ +1641,5 @@
> > +    container.mousedown(function(e) {
> > +      let target = e.target;
> > +      if (Utils.isLeftClick(e) && self.$closeButton[0] != target &&
> > +          self.$ntb[0] != target && self.$titlebar[0] != target &&
> > +          !self.$titlebar.contains(target))
> 
> that's a bunch of conditions. i'd like to see a comment here summarizing.

Added a comment.


However, I am not sure whether we should land this or not based on comment 13
Attachment #539422 - Attachment is obsolete: true
It seems like this makes panorama more complex as opposed to simplifying it.  Two navigation steps before getting to the site also might slow users down.  The one exception is that this might be a more natural way to deal with piles in panorama, instead of the overlay box that we currently have.
(In reply to comment #16)
> It seems like this makes panorama more complex as opposed to simplifying it.
> Two navigation steps before getting to the site also might slow users down. 
> The one exception is that this might be a more natural way to deal with
> piles in panorama, instead of the overlay box that we currently have.

It still supports clicking on the specific page (or at least that's the intention, I haven't tried the patch). This just makes it possible to click anywhere in the group to switch to it.
ok, I slightly misunderstood the change (thought this was zooming the group to fill the panorama area before letting you select one of the pages).

I would want to play around with what this feels like for awhile, it seems like group switching would feel a lot faster, which is good (you don't have the cognitive load of picking a target inside of the group first), but is also bad (if you were trying to move the group around, you select it by mistake).  Perhaps land first on the UX branch?
(In reply to comment #18)
> ok, I slightly misunderstood the change (thought this was zooming the group
> to fill the panorama area before letting you select one of the pages).
> 
> I would want to play around with what this feels like for awhile, it seems
> like group switching would feel a lot faster, which is good (you don't have
> the cognitive load of picking a target inside of the group first), but is
> also bad (if you were trying to move the group around, you select it by
> mistake).  Perhaps land first on the UX branch?

Yes, it would be good to land on the UX branch first and then decide what we want to do with this patch.
I tried to push this patch to ux-branch but seems I don't have the permissions to do so:

remote: abort: could not lock repository /repo/hg/mozilla/projects/ux: Permission denied

Who else could do this for us?
Comment on attachment 539422 [details] [diff] [review]
v3

This patch landed on ux-branch yesterday.
Attachment #539422 - Flags: ui-review?(faaborg)
Thank you! This works great, and feels right.

The one bug I found is if you have an app tab as the active tab, it seems to pick the first tab that is not an app tab instead — i.e. the app tab doesn't seem to count as "active".

Especially once we get global app tabs (which means we don't have to show app tabs in every group), this is going to be confusing if it doesn't keep the app tab active if that was the one you just came from. :)
Also, even if you explicitly click on the app tab icon in that group, it doesn't activate it.
Comment on attachment 539422 [details] [diff] [review]
v3

Approved as long as the app tab bug is addressed.
Attachment #539422 - Flags: ui-review?(faaborg) → ui-review+
(In reply to comment #22)
> Thank you! This works great, and feels right.
> 
> The one bug I found is if you have an app tab as the active tab, it seems to
> pick the first tab that is not an app tab instead — i.e. the app tab doesn't
> seem to count as "active".
> 
> Especially once we get global app tabs (which means we don't have to show
> app tabs in every group), this is going to be confusing if it doesn't keep
> the app tab active if that was the one you just came from. :)

I believe the issue would be addressed by bug 600665
(In reply to comment #25)
> > The one bug I found is if you have an app tab as the active tab, it seems to
> > pick the first tab that is not an app tab instead — i.e. the app tab doesn't
> > seem to count as "active".
> > 
> > Especially once we get global app tabs (which means we don't have to show
> > app tabs in every group), this is going to be confusing if it doesn't keep
> > the app tab active if that was the one you just came from. :)
> 
> I believe the issue would be addressed by bug 600665

No, the one bug left is that clicking an app tab is handled by the container's new click handler. We need to add the app tab tray to the exclusion list here:

>+      if (Utils.isLeftClick(e) && self.$closeButton[0] != target &&
>+          self.$ntb[0] != target && self.$titlebar[0] != target &&
>+          !self.$titlebar.contains(target))
>+        lastMouseDownTarget = target;
Attached patch v5 (obsolete) — — Splinter Review 
(In reply to comment #26)
> No, the one bug left is that clicking an app tab is handled by the
> container's new click handler. We need to add the app tab tray to the
> exclusion list here:
> 
> >+      if (Utils.isLeftClick(e) && self.$closeButton[0] != target &&
> >+          self.$ntb[0] != target && self.$titlebar[0] != target &&
> >+          !self.$titlebar.contains(target))
> >+        lastMouseDownTarget = target;

Done
Attachment #539825 - Attachment is obsolete: true
Attachment #543717 - Flags: feedback?(tim.taubert)
Comment on attachment 543717 [details] [diff] [review]
v5

Review of attachment 543717 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks, that looks and works good! Sorry, you were actually right about bug 600665 because we still need to keep the app tab focused when leaving Panorama with a click on a group (as per comment #22). We can fix this for now pretty easily (see below).

F+ with that additional fix.

::: browser/base/content/tabview/groupitems.js
@@ +1644,5 @@
> +    container.mouseup(function(e) {
> +      let same = (e.target == lastMouseDownTarget);
> +      lastMouseDownTarget = null;
> +
> +      if (same && !self.isDragging) {

if (gBrowser.selectedTab.pinned && UI.getActiveTab() != self.getActiveTab()) {
  UI.goToTab(gBrowser.selectedTab);
} else {
  // do zoom into group's active tab...
}
Attachment #543717 - Flags: feedback?(tim.taubert) → feedback+
Attached patch v6 (obsolete) — — Splinter Review 
(In reply to comment #28)
> Comment on attachment 543717 [details] [diff] [review] [review]
> v5
> 
> Review of attachment 543717 [details] [diff] [review] [review]:
> -----------------------------------------------------------------
> 
> Thanks, that looks and works good! Sorry, you were actually right about bug
> 600665 because we still need to keep the app tab focused when leaving
> Panorama with a click on a group (as per comment #22). We can fix this for
> now pretty easily (see below).
> 
> F+ with that additional fix.
> 
> ::: browser/base/content/tabview/groupitems.js
> @@ +1644,5 @@
> > +    container.mouseup(function(e) {
> > +      let same = (e.target == lastMouseDownTarget);
> > +      lastMouseDownTarget = null;
> > +
> > +      if (same && !self.isDragging) {
> 
> if (gBrowser.selectedTab.pinned && UI.getActiveTab() != self.getActiveTab())
> {
>   UI.goToTab(gBrowser.selectedTab);
> } else {
>   // do zoom into group's active tab...
> }

Added
Attachment #543717 - Attachment is obsolete: true
Attachment #543741 - Flags: review?(dao)
We'd really like to get this into Fx 7 together with bug 663613 and bug 663611. That patch is just a little addition to the already reviewed one... Dão, if you could review this in time we and the UX team would be truly grateful :)
Comment on attachment 543741 [details] [diff] [review]
v6

>+      // only set the last mouse down target if it is a left click, not on the
>+      // close button, not on the new tab button, not on the title bar and its
>+      // element
>+      if (Utils.isLeftClick(e) && self.$closeButton[0] != target &&
>+          self.$ntb[0] != target && self.$titlebar[0] != target &&
>+          !self.$titlebar.contains(target) &&
>+          !self.$appTabTray.contains(target))

break the line after &&

>+        if (gBrowser.selectedTab.pinned && UI.getActiveTab() != self.getActiveTab()) {

ditto

I only reviewed the v3->v6 interdiff.
Attachment #543741 - Flags: review?(dao) → review+
Attached patch Patch for checkin (obsolete) — — Splinter Review 
(In reply to comment #31)
> Comment on attachment 543741 [details] [diff] [review] [review]
> v6
> 
> >+      // only set the last mouse down target if it is a left click, not on the
> >+      // close button, not on the new tab button, not on the title bar and its
> >+      // element
> >+      if (Utils.isLeftClick(e) && self.$closeButton[0] != target &&
> >+          self.$ntb[0] != target && self.$titlebar[0] != target &&
> >+          !self.$titlebar.contains(target) &&
> >+          !self.$appTabTray.contains(target))
> 
> break the line after &&
> 

Updated

> >+        if (gBrowser.selectedTab.pinned && UI.getActiveTab() != self.getActiveTab()) {
> 
> ditto
>

Updated 
> I only reviewed the v3->v6 interdiff.
Attachment #543741 - Attachment is obsolete: true
Thanks for your quick review, Dão! Alas, this doesn't seem ready to land, yet. I noticed two remaining problems:

Have some app tabs and one of them is the selectedTab. Create an empty group. Click this empty group.

1) If you viewed the app tab in another group before switching to Panorama this group will be shown again. So we'd need to additionally switch the group when an app tab is active, like here:

https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabview/groupitems.js#1169

2) What is the expected workflow when there is an active app tab and we click an empty group? Should a new tab be created? Should no new tab be created and we'll just show the app tab? (That's the current behavior.) Seems like a question for the UX team.
Attached patch v8 — — Splinter Review 
(In reply to comment #33)
> Thanks for your quick review, Dão! Alas, this doesn't seem ready to land,
> yet. I noticed two remaining problems:
> 
> Have some app tabs and one of them is the selectedTab. Create an empty
> group. Click this empty group.
> 
> 1) If you viewed the app tab in another group before switching to Panorama
> this group will be shown again. So we'd need to additionally switch the
> group when an app tab is active, like here:
> 
> https://mxr.mozilla.org/mozilla-central/source/browser/base/content/tabview/
> groupitems.js#1169

Fixed

> 
> 2) What is the expected workflow when there is an active app tab and we
> click an empty group? Should a new tab be created? Should no new tab be
> created and we'll just show the app tab? (That's the current behavior.)
> Seems like a question for the UX team.

I've added the code to create new tab if the group is empty.  Need feedback from UX team.
Attachment #543888 - Attachment is obsolete: true
Attachment #544316 - Flags: ui-review?(limi)
Attachment #544316 - Flags: feedback?(tim.taubert)
Comment on attachment 544316 [details] [diff] [review]
v8

Review of attachment 544316 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!
Attachment #544316 - Flags: feedback?(tim.taubert) → feedback+
Comment on attachment 544316 [details] [diff] [review]
v8

Works great!
Attachment #544316 - Flags: ui-review?(limi) → ui-review+
Comment on attachment 544316 [details] [diff] [review]
v8

Review of attachment 544316 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!
Attachment #544316 - Flags: review?(sdwilsh)
Comment on attachment 544316 [details] [diff] [review]
v8

Review of attachment 544316 [details] [diff] [review]:
-----------------------------------------------------------------

r=sdwilsh

::: browser/base/content/tabview/groupitems.js
@@ +1666,5 @@
> +          !self.$titlebar.contains(target) &&
> +          !self.$appTabTray.contains(target))
> +        lastMouseDownTarget = target;
> +      else
> +        lastMouseDownTarget = null;

Please brace your ifs if they are this complicated.

::: browser/base/content/test/tabview/browser_tabview_bug663612.js
@@ +1,2 @@
> +/* Any copyright is dedicated to the Public Domain.
> +   http://creativecommons.org/publicdomain/zero/1.0/ */

Please use a descriptive name for this filename instead of a bug number.
Attachment #544316 - Flags: review?(sdwilsh) → review+
http://hg.mozilla.org/mozilla-central/rev/936e084e74b8
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 8
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 673729
Mozilla/5.0 (X11; Linux x86_64; rv:9.0a1) Gecko/20110907 Firefox/9.0a1

Focusing a group works fine. When selecting any group, the active tab from the last group is selected. 
2 questions, however:

When app tabs are available and one of them is active in any group, the respective app tab will be displayed every time another group is focused in panorama. Shouldn't other groups focus on the previously active tab or app tab in that group?

Regarding the second question in Comment 33, when an empty group with global app tabs is focused, a new tab is created. Is this the approved behavior?
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: