610208 - When closing a tab, other tabs should not resize until cursor leaves the group

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, enhancement, P3)

Description

[David Bengoa]

User-Agent:       Mozilla/5.0 (X11; Linux x86_64; rv:2.0b8pre) Gecko/20101106 Firefox/4.0b8pre
Build Identifier: Mozilla/5.0 (X11; Linux x86_64; rv:2.0b8pre) Gecko/20101106 Firefox/4.0b8pre

When i close a tab, other tabs resize immediately, making it difficult to close multiple tabs.

This is especially annoying if i accidentally click a tab, because i have to open Panorama and continue closing tabs.


Reproducible: Always

Steps to Reproduce:
1.Open about 20 of tabs in a tabcandy group
2.Open tabcandy
3.Make sure there isn't enough space for all tabs in the group by resizing the group. Tabs should resize to fit in the group.
4.Close several tabs
Actual Results:  
Tab miniatures resize immediately after there is available space

Expected Results:  
Tab miniatures don't resize until the mouse leave the group

See Bug 465086 and Google chrome tab-close behavior

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

Attachment: v1

Comment 2

[David Bengoa]

Attachment: Move the tabs to fill the relased space

Your patch works great for me, i have introduced some small modifications, now the tabs will move to fill the relased space (and now you can close several tabs wothout moving the mouse). Haven't tested a lot, and i don't know if it will break some tests (don't know how to run them :S)

Comment 3

[Michael Yoshitaka Erlewine [:mitcho]]

Tim, David: thanks for taking this bug on! :D Please don't be offended by my feedback-... I really like the direction of these patches!

Personally, I like the behavior in David's (latter) patch better. I don't think the shifting of the tabs within the group will be an issue, given that the cursor will have already been on top of the last focused tab. Getting that "hole" filled again, in case it wasn't the last patch, actually makes closing multiple tabs *easier*.

Tim, David: a few suggestions as you continue:

1. Please make "private" variables like "beforeCloseCount" or "needsAnimatedArrange" are prefixed with _. We can probably turn arrangeAnimatedIfNeeded into _arrangeAnimatedIfNeeded() too.
2. Why are we keeping track of beforeCloseCount? Why not just keep track of the last _columns value and send that to arrange(), ensuring that we just keep the same number of columns? (Though, in effect, this should be the same.)
3. Should we be calling arrangeAnimatedIfNeeded() on Panorama hide as well? Right now if you close a bunch of tabs and then go into one of them, without the cursor leaving the group, then later go back to Panorama, we suddenly trigger the animated arrange at that point.
4. Please make sure to test in "expanded" mode as well; i.e. when a stacked group is laid out as a gray overlay.
5. Make sure to try your patch out on top of bug 587503's, which most likely will land before this one and may affect the behavior. In particular, I worry about the behavior when we close a bunch of tabs and then drag + rearrange some of the tabs within that group.

Ian: any additional comments?

We'll look forward to a next version! Let us know if you have any questions.

Comment 4

[Michael Yoshitaka Erlewine [:mitcho]]

Let's also ask Faaborg what he thinks about the proposed behavior in general, as well.

Comment 5

[David Bengoa]

(In reply to comment #3)
> 2. Why are we keeping track of beforeCloseCount? Why not just keep track of the
> last _columns value and send that to arrange(), ensuring that we just keep the
> same number of columns? (Though, in effect, this should be the same.)

This was my first approach, apparently GroupItem.arrange rewrites the value of options.columns to this._columns before passing it to Items.arrange, so passing a "columns" option to GroupItem.arrange is useless (I don't know why i pass it in my patch).

> 4. Please make sure to test in "expanded" mode as well; i.e. when a stacked
> group is laid out as a gray overlay.

I tested it, and apparently it works ok.

> 5. Make sure to try your patch out on top of bug 587503's, which most likely
> will land before this one and may affect the behavior. In particular, I worry
> about the behavior when we close a bunch of tabs and then drag + rearrange some
> of the tabs within that group.

I will try to have a look at this. Next year.

Comment 6

[Tim Taubert [:ttaubert] (inactive)]

Attachment: v3

(In reply to comment #3)

I like David's behaviour too - so I merged our changes because they don't need to be that separated. I also fixed the the points you mentioned:

> 1. Please make "private" variables like "beforeCloseCount" or
> "needsAnimatedArrange" are prefixed with _. We can probably turn
> arrangeAnimatedIfNeeded into _arrangeAnimatedIfNeeded() too.

Variables got renamed but the naming scheme does now comply with the JS coding standards.

> 2. Why are we keeping track of beforeCloseCount? Why not just keep track of the
> last _columns value and send that to arrange(), ensuring that we just keep the
> same number of columns? (Though, in effect, this should be the same.)

As David already said we really need to keep track of the item count - so I kept that part and removed the 'columns' option.

> 3. Should we be calling arrangeAnimatedIfNeeded() on Panorama hide as well?
> Right now if you close a bunch of tabs and then go into one of them, without
> the cursor leaving the group, then later go back to Panorama, we suddenly
> trigger the animated arrange at that point.

You're totally right, that function is now called on tabviewhidden, too.

> 4. Please make sure to test in "expanded" mode as well; i.e. when a stacked
> group is laid out as a gray overlay.

Did not work properly but now it does, i.e.: fixed.

> 5. Make sure to try your patch out on top of bug 587503's, which most likely
> will land before this one and may affect the behavior. In particular, I worry
> about the behavior when we close a bunch of tabs and then drag + rearrange some
> of the tabs within that group.

I'll join David with that. Next year, whoever of us manages to do first :) For now I fixed the behaviour when dragging+dropping items.

Comment 7

[Alex Faaborg [:faaborg] (Firefox UX)]

>Let's also ask Faaborg what he thinks about the proposed behavior in general,
>as well.

Overall sounds good, sort of a 2D version of bug 465086.  Any chance someone could post try server builds for me to quickly check out the behavior?

Comment 8

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #7)
> >Let's also ask Faaborg what he thinks about the proposed behavior in general,
> >as well.
> 
> Overall sounds good, sort of a 2D version of bug 465086.  Any chance someone
> could post try server builds for me to quickly check out the behavior?

Faaborg, what platform are you on?

Comment 9

[Alex Faaborg [:faaborg] (Firefox UX)]

I'm one of the few full time Windows users at Mozilla [insert boiler plate speech about where our market share is and all that :)]

Comment 10

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #9)
> I'm one of the few full time Windows users at Mozilla [insert boiler plate
> speech about where our market share is and all that :)]

Here you go. This is Tim's v3 patch:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-add61e42fca7/

Comment 11

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 500524 [details] [diff] [review]
v3

Behavior looks good, though I haven't yet tested with the bug 587503 patch.

But first, some more thoughts on the code and some nits:

>+  this._resetItemCountBeforeRemoval();
>+  window.addEventListener('tabviewhidden', function () self._arrangeAdjustingItemSizeIfNeeded(), false);

We never unload this listener, do we? We should do that when the group closes, perhaps in closeHidden? Raymond or Ian may have a better idea for where to put that. We can also put the loading in _addHandlers.

>   $container
>     .css({zIndex: -100})
>-    .appendTo("body");
>+    .appendTo("body")
>+    .mouseout(function (event) {
>+      let mouseHasLeftContainer =
>+        (self.bounds.left > event.clientX || self.bounds.right < event.clientX ||
>+         self.bounds.top > event.clientY || self.bounds.bottom < event.clientY);
>+      if (mouseHasLeftContainer) {

How about something like:

> let mouse = new Rect(event.clientX, event.clientY, 0, 0);
> if (!self.bounds.contains(mouse))


>+  // ----------
>+  // Function: _arrangePreservingItemSize
>+  // Lays out all of the children while preserving the current item size.
>+  //
>+  // Parameters:
>+  //   count - the current item count
>+  _arrangePreservingItemSize: function GroupItem__arrangePreservingItemSize(count) {
>+    if (-1 === this._lastItemCountBeforeRemoval) {
>+      this._lastItemCountBeforeRemoval = count;
>+    }
>+    
>+    this.arrange({count: this._lastItemCountBeforeRemoval});
>+  },

Is there ever a situation in which we will be calling this and actually will ignore the given count argument because _lastItemCountBeforeRemoval is already set at this point? It's not clear to me that that would ever happen...

feedback+ with those points/questions addressed.

Next steps:
- write an automated test for this. feel free to ask us in IRC for tips. Also look at the other tabview tests in browser/base/content/test/tabview
- flag review?ian on next patch

Comment 12

[Ian Gilman [:iangilman]]

(In reply to comment #11)
> We never unload this listener, do we? We should do that when the group closes,
> perhaps in closeHidden? Raymond or Ian may have a better idea for where to put
> that. 

It should go in GroupItem.close().

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #11)

Thanks for your feedback Michael. I didn't know about the Rect class - looks much better with it.

> >+  // Function: _arrangePreservingItemSize
[...]
> Is there ever a situation in which we will be calling this and actually will
> ignore the given count argument because _lastItemCountBeforeRemoval is already
> set at this point? It's not clear to me that that would ever happen...

The given count argument is ignored when one removes multiple tabs without leaving the group (or similar 'final' actions). So we have to track the tab count right before the first tab was removed.

Comment 14

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #13)
> (In reply to comment #11)
> 
> Thanks for your feedback Michael. I didn't know about the Rect class - looks
> much better with it.
> 
> > >+  // Function: _arrangePreservingItemSize
> [...]
> > Is there ever a situation in which we will be calling this and actually will
> > ignore the given count argument because _lastItemCountBeforeRemoval is already
> > set at this point? It's not clear to me that that would ever happen...
> 
> The given count argument is ignored when one removes multiple tabs without
> leaving the group (or similar 'final' actions). So we have to track the tab
> count right before the first tab was removed.

Okay, cool. Thanks for your explanation.

One last thought: 

>+  window.addEventListener('tabviewhidden', function () self._arrangeAdjustingItemSizeIfNeeded(), false);

Do we need to have this event listener on the window for every group, all the time? I'm not thrilled by that. Maybe we can get away with only having only at most one of these handlers set at a time. For example, what about keeping a flag on the group like _hideHandlerSet and, when you remove a tab, adding that event listener at that point (if _hideHandlerSet is false), etc., and then removing the handler either when we leave the group, or when it runs itself.

What do you think?

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #14)
> (In reply to comment #13)
> Do we need to have this event listener on the window for every group, all the
> time? I'm not thrilled by that. Maybe we can get away with only having only at
> most one of these handlers set at a time. For example, what about keeping a
> flag on the group like _hideHandlerSet and, when you remove a tab, adding that
> event listener at that point (if _hideHandlerSet is false), etc., and then
> removing the handler either when we leave the group, or when it runs itself.
> 
> What do you think?

Sounds like a great idea. I'll implement it this way.

Comment 16

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v4 (issues addressed, no test yet)

Michael, could you please give me another feedback about the current changes before I start to write the test for this patch? Thanks :)

Comment 17

[Alex Faaborg [:faaborg] (Firefox UX)]

>Here you go. This is Tim's v3 patch:
>http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds>/[e-mail]-add61e42fca7/

sorry, didn't get to testing this in time.  Can you post v4?  Alternatively I could actually get around to setting up a builds on my windows machine.

Comment 18

[Tim Taubert [:ttaubert] (inactive)]

I just pushed v4 to try:

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-542040152119/

The build is hopefully available in about 3 hours.

Comment 19

[Tim Taubert [:ttaubert] (inactive)]

Build is ready: http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-542040152119/try-w32/

Comment 20

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #16)
> Created attachment 501767 [details] [diff] [review]
> patch v4 (issues addressed, no test yet)
> 
> Michael, could you please give me another feedback about the current changes
> before I start to write the test for this patch? Thanks :)

Looking good! A couple thoughts:

- I'm not sure that I like the term "conditional". "Conditional"? What's the condition? This at *least* needs to have better comments... in particular, since this new parameter (whatever we decide to call it) is not simply passed to Items.arrange or stackArrange, it should be documented under "options" in the docs for GroupItem_arrange.
- Is it okay that _disableArrangeWhenHidden (which deletes _arrangeWhenHidden) is called from within _arrangeWhenHidden? Not sure... might be fine. Looks fishy, though.

Comment 21

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v5 (refactored, no test yet)

(In reply to comment #20)
> - I'm not sure that I like the term "conditional". "Conditional"? What's the
> condition? This at *least* needs to have better comments... in particular,
> since this new parameter (whatever we decide to call it) is not simply passed
> to Items.arrange or stackArrange, it should be documented under "options" in
> the docs for GroupItem_arrange.
> - Is it okay that _disableArrangeWhenHidden (which deletes _arrangeWhenHidden)
> is called from within _arrangeWhenHidden? Not sure... might be fine. Looks
> fishy, though.

Hey Mitcho, thanks for your feedback. I needed some time to get the wording correct and could refactor the whole thing once again to make it more compact and readable. Looking forward to your feedback before hopefully getting down to write the test!

Comment 22

[Kevin Hanes]

bugspam (moving b9 to b10)

Comment 23

[Kevin Hanes]

bugspam (removing b9)

Comment 24

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 502406 [details] [diff] [review]
patch v5 (refactored, no test yet)

I'm liking this phrasing a lot better. Thanks!

Comment 25

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v6 (with test)

Test for expand/collapse is commented out/unfinished because while writing it I realized that this is broken (see bug 625443).

Comment 26

[Ian Gilman [:iangilman]]

Comment on attachment 503658 [details] [diff] [review]
patch v6 (with test)

This is definitely a good feature, but I have to admit to being a bit concerned about landing it so close to shipping, in case there are subtle problems introduced. I suppose it's okay as long as we get it in for b10, especially considering it looks like we can disable it entirely by removing the one freeze call if need be. At any rate, I would prioritize work on this below blockers.

>+  this._freezedItemSizeData = {};

I cannot stand by and allow us to use the word "freezed"; "frozen" is correct. 

By the way, it seems the item count stored in this object is only used as a flag... maybe it should actually be a flag, or maybe the existance of the object itself could be the flag?

>   close: function GroupItem_close() {
>     this.removeAll({dontClose: true});
>     GroupItems.unregister(this);
>+    this._unfreezeItemSize({dontArrange: true});

At this point there's no need to unfreeze, per se, except to remove the event handler. The code's good, but maybe worth adding a comment as to why you're calling it.

>-      if (!options.dontArrange)
>+      if (!options.dontArrange) {
>         this.arrange({animate: !options.immediately});
>+        this._unfreezeItemSize({dontArrange: true});
>+      }

Here in .add(), seems like you should always unfreeze no matter what; for one thing, if you don't, the tabs may grow outside the group.

>@@ -1382,16 +1437,17 @@ GroupItem.prototype = Utils.extend(new I
>       this.expanded.$shield.remove();
>       this.expanded = null;
> 
>       this._children.forEach(function(child) {
>         child.removeClass("stack-trayed");
>       });
> 
>       this.arrange({z: z + 2});
>+      this._unfreezeItemSize({dontArrange: true});

You're covering collapse here, but it looks like you're not unfreezing on expand... should do that too.

>+    container.mouseout(function (event) {
>+      let mouse = new Rect(event.clientX, event.clientY, 0, 0);
>+      if (!self.bounds.contains(mouse))
>+        self._unfreezeItemSize();
>+    });

Here's where I wish iQ had mouseleave. I believe this will fail to unfreeze if the user has their mouse over a tab and then flicks it off fast enough that the next mouse event happens outside the group. Not sure what to do about that, but seems unfortunate.

Also, seems like Rect.bounds should be able to accept a Point (I know it doesn't, but maybe it should be modified to do so)... I guess this is fine too, since you would have to create the Point... just looks funny.

>   setResizable: function GroupItem_setResizable(value, immediately) {
>+    var self = this;
>+
>     this.resizeOptions.minWidth = GroupItems.minGroupWidth;
>     this.resizeOptions.minHeight = GroupItems.minGroupHeight;
>+    this.resizeOptions.start = function () self._unfreezeItemSize();

Definitely important to unfreeze for resize, but I figure you might as well do it for drag as well.

>+  // arrange items when the group title's input field is focused
>+  let actionFocusTitle = function (callback) {
>+    let target = groupItem.$titleShield[0];
>+    EventUtils.synthesizeMouseAtCenter(target, {}, cw);
>+    callback();
>+  }

I found these comments that start with "arrange items when" to be a little confusing... this code isn't doing anything to make the items arrange themselves under those conditions; instead it's generating those conditions in the hopes that the items will arrange themselves. I guess I'd say something like "focus group title's input field to cause item arrange".

>+  let next = function () {
>+    let action = actions.shift();
>+    
>+    if (!action)
>+      return testRemoveWhileStacked();

testRemoveWhileStacked() doesn't actually return anything, and neither does next() on its other code paths. I realize this is an abbreviation for calling and then returning, but it masks intent; please fix.

>+  // when removing the top-most tab of a stacked group we must not freeze
>+  // the item size
>+  let testRemoveWhileStacked = function () {
>+    groupItem.setSize(150, 200, true);
>+    groupItem.setUserSize();
>+
>+    let originalBounds = groupItem.getChild(0).getBounds();
>+
>+    // add a new tab to let the group stack
>+    gBrowser.loadOneTab('about:blank', {inBackground: true});

Should test before adding the tab that the group isn't stacked and afterwards that it is stacked.

>+  // TODO reactivate/write this when #625443 has landed
>+  // when an item is removed while in expanded mode this should work like
>+  // in normal non-stacked groups

If this lands before 625443, please file a follow up to take care of this; otherwise make sure it happens before landing.

>+  showTabView(function () {
>+    cw = TabView.getContentWindow();
>+    groupItem = createGroupItem();
>+    afterAllTabsLoaded(next);
>+  });
>+
>+  TabView.show();

That TabView.show() seems redundant.

>+// ----------
>+function showTabView(callback) {
>+  if (TabView.isVisible())
>+    return callback();
>+
>+  whenTabViewIsShown(callback);
>+  TabView.show();
>+}
>+
>+// ----------
>+function hideTabView(callback) {
>+  if (!TabView.isVisible())
>+    return callback();
>+
>+  whenTabViewIsHidden(callback);
>+  TabView.hide();
>+}
>+
>+// ----------
>+function whenTabViewIsShown(callback) {
>+  if (TabView.isVisible())
>+    return callback();
>+
>+  window.addEventListener('tabviewshown', function () {
>+    window.removeEventListener('tabviewshown', arguments.callee, false);
>+    callback();
>+  }, false);
>+}
>+
>+// ----------
>+function whenTabViewIsHidden(callback) {
>+  if (!TabView.isVisible())
>+    return callback();
>+
>+  window.addEventListener('tabviewhidden', function () {
>+    window.removeEventListener('tabviewhidden', arguments.callee, false);
>+    callback();
>+  }, false);
>+}

Same comment on these 4 as above on the "return callback()" pattern.

Also, these might be candidates for head.js.

Comment 27

[Michael Yoshitaka Erlewine [:mitcho]]

Tim, I'd be happy to take over if you'd like.

Comment 28

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v7 (WIP)

Thanks for taking over Mitcho! Here is the latest patch version that has almost all issues fixed that Ian mentioned. See below:

(In reply to comment #26)
> I cannot stand by and allow us to use the word "freezed"; "frozen" is correct.

I am very sorry :)

> By the way, it seems the item count stored in this object is only used as a
> flag... maybe it should actually be a flag, or maybe the existance of the
> object itself could be the flag?

No, we really do need the item count when more than one item is removed. That count is passed to arrange() options.

> >   close: function GroupItem_close() {
> >     this.removeAll({dontClose: true});
> >     GroupItems.unregister(this);
> >+    this._unfreezeItemSize({dontArrange: true});
> 
> At this point there's no need to unfreeze, per se, except to remove the event
> handler. The code's good, but maybe worth adding a comment as to why you're
> calling it.

Comment added.

> >-      if (!options.dontArrange)
> >+      if (!options.dontArrange) {
> >         this.arrange({animate: !options.immediately});
> >+        this._unfreezeItemSize({dontArrange: true});
> >+      }
> 
> Here in .add(), seems like you should always unfreeze no matter what; for one
> thing, if you don't, the tabs may grow outside the group.

Always unfreezing now in .add().

> >@@ -1382,16 +1437,17 @@ GroupItem.prototype = Utils.extend(new I
> >       this.expanded.$shield.remove();
> >       this.expanded = null;
> > 
> >       this._children.forEach(function(child) {
> >         child.removeClass("stack-trayed");
> >       });
> > 
> >       this.arrange({z: z + 2});
> >+      this._unfreezeItemSize({dontArrange: true});
> 
> You're covering collapse here, but it looks like you're not unfreezing on
> expand... should do that too.

We could unfreeze on collapse, that shouldn't hurt too much. But as removing items from stacked groups does not lead to a 'frozen item size' we shouldn't really need that, do we?

> >+    container.mouseout(function (event) {
> >+      let mouse = new Rect(event.clientX, event.clientY, 0, 0);
> >+      if (!self.bounds.contains(mouse))
> >+        self._unfreezeItemSize();
> >+    });
> 
> Here's where I wish iQ had mouseleave. I believe this will fail to unfreeze if
> the user has their mouse over a tab and then flicks it off fast enough that the
> next mouse event happens outside the group. Not sure what to do about that, but
> seems unfortunate.

iQ does now provide mouseleave and mouseenter. The only tricky thing is that we still have to check if we're hovering a tabItem because they do not descend from groupItems. This can be removed once we get time to address bug 616576.

> >   setResizable: function GroupItem_setResizable(value, immediately) {
> >+    var self = this;
> >+
> >     this.resizeOptions.minWidth = GroupItems.minGroupWidth;
> >     this.resizeOptions.minHeight = GroupItems.minGroupHeight;
> >+    this.resizeOptions.start = function () self._unfreezeItemSize();
> 
> Definitely important to unfreeze for resize, but I figure you might as well do
> it for drag as well.

TODO

> >+  // arrange items when the group title's input field is focused
> >+  let actionFocusTitle = function (callback) {
> >+    let target = groupItem.$titleShield[0];
> >+    EventUtils.synthesizeMouseAtCenter(target, {}, cw);
> >+    callback();
> >+  }
> 
> I found these comments that start with "arrange items when" to be a little
> confusing... this code isn't doing anything to make the items arrange
> themselves under those conditions; instead it's generating those conditions in
> the hopes that the items will arrange themselves. I guess I'd say something
> like "focus group title's input field to cause item arrange".

Fixed.

> >+  let next = function () {
> >+    let action = actions.shift();
> >+    
> >+    if (!action)
> >+      return testRemoveWhileStacked();
> 
> testRemoveWhileStacked() doesn't actually return anything, and neither does
> next() on its other code paths. I realize this is an abbreviation for calling
> and then returning, but it masks intent; please fix.

Fixed.

> >+  // when removing the top-most tab of a stacked group we must not freeze
> >+  // the item size
> >+  let testRemoveWhileStacked = function () {
> >+    groupItem.setSize(150, 200, true);
> >+    groupItem.setUserSize();
> >+
> >+    let originalBounds = groupItem.getChild(0).getBounds();
> >+
> >+    // add a new tab to let the group stack
> >+    gBrowser.loadOneTab('about:blank', {inBackground: true});
> 
> Should test before adding the tab that the group isn't stacked and afterwards
> that it is stacked.

Done.

> >+  // TODO reactivate/write this when #625443 has landed
> >+  // when an item is removed while in expanded mode this should work like
> >+  // in normal non-stacked groups
> 
> If this lands before 625443, please file a follow up to take care of this;
> otherwise make sure it happens before landing.

TODO

> Also, these might be candidates for head.js.

Removed as they were already added to head.js by some other patch.

Comment 29

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v7b (WIP)

Small fix: Corrected mouseenter/mouseleave condition.

Comment 30

[Michael Yoshitaka Erlewine [:mitcho]]

> > By the way, it seems the item count stored in this object is only used as a
> > flag... maybe it should actually be a flag, or maybe the existance of the
> > object itself could be the flag?
> 
> No, we really do need the item count when more than one item is removed. That
> count is passed to arrange() options.

Agreed.

> > >@@ -1382,16 +1437,17 @@ GroupItem.prototype = Utils.extend(new I
> > You're covering collapse here, but it looks like you're not unfreezing on
> > expand... should do that too.
> 
> We could unfreeze on collapse, that shouldn't hurt too much. But as removing
> items from stacked groups does not lead to a 'frozen item size' we shouldn't
> really need that, do we?

Indeed, we don't need this. I verified that the tab item size doesn't change while removing multiple things from an expanded stack.

> > >   setResizable: function GroupItem_setResizable(value, immediately) {
> > >+    var self = this;
> > >+
> > >     this.resizeOptions.minWidth = GroupItems.minGroupWidth;
> > >     this.resizeOptions.minHeight = GroupItems.minGroupHeight;
> > >+    this.resizeOptions.start = function () self._unfreezeItemSize();
> > 
> > Definitely important to unfreeze for resize, but I figure you might as well do
> > it for drag as well.

Done.

> > >+  // TODO reactivate/write this when #625443 has landed
> > >+  // when an item is removed while in expanded mode this should work like
> > >+  // in normal non-stacked groups
> > 
> > If this lands before 625443, please file a follow up to take care of this;
> > otherwise make sure it happens before landing.

Bug 625443 has landed. Fixed.

Comment 31

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v8

I believe, between Tim and I, we have covered all your comments. Please see the couple comments above where we respond to a number of them.

Comment 32

[Michael Yoshitaka Erlewine [:mitcho]]

Pushed v8 to try.

Comment 33

[Ian Gilman [:iangilman]]

Comment on attachment 505981 [details] [diff] [review]
Patch v8

Looks like everything is good except for the mouseleave situation. As far as I can tell, the new mouseleave in iQ, while ambitious, is just a more elaborate version of the "ignore mouseout events that don't really apply" strategy employed in the previous patch. This doesn't address the issue I raised before: if you set your cursor on a tab in the group and then move your cursor quickly outside the group, there's a good chance the group simply won't get a mouseout event (I've verified this with the current patch). 

Possible fixes: 

* Whenever the mouse moves into a sub-item (including things that are actually DOM children, as well as TabItem children), attach a mouseout event to that sub-item, so you can be assured of getting the event. 

* Use an interval to periodically check if the mouse is outside of the group. 

* Use a mousemove on body or some such to do a similar check?

* Figure it's good enough as is.

I'm not really thrilled about the last option, and the other three don't seem trivial.

Comment 34

[Tim Taubert [:ttaubert] (inactive)]

Bug ping pong. Re-assigning to me :)

Comment 35

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v9

(In reply to comment #33)
> Comment on attachment 505981 [details] [diff] [review]
> Patch v8
> 
> Looks like everything is good except for the mouseleave situation. As far as I
> can tell, the new mouseleave in iQ, while ambitious, is just a more elaborate
> version of the "ignore mouseout events that don't really apply" strategy
> employed in the previous patch. This doesn't address the issue I raised before:
> if you set your cursor on a tab in the group and then move your cursor quickly
> outside the group, there's a good chance the group simply won't get a mouseout
> event (I've verified this with the current patch). 
> 
> Possible fixes: 
> 
> * Whenever the mouse moves into a sub-item (including things that are actually
> DOM children, as well as TabItem children), attach a mouseout event to that
> sub-item, so you can be assured of getting the event. 
> 
> * Use an interval to periodically check if the mouse is outside of the group. 
> 
> * Use a mousemove on body or some such to do a similar check?
> 
> * Figure it's good enough as is.
> 
> I'm not really thrilled about the last option, and the other three don't seem
> trivial.

I completely removed the mouseleave/enter changes. When item size is frozen a mousemove listener for window is added and is removed when unfrozen (just like ontabviewhidden). That was actually quite trivial to implement.

I completely restructured the test for this patch and added some new testcases for mouse movement.

One essential change is the one I added in items.js: The code to determine the best drop target for drag/drop operations was moved into a function so that it can be used onmousedown (when dragging starts). So we determine right on drag start if we hover a droppable that needs to receive an out event. So you can move your cursor out as quickly as you want and that won't fail.

Further, this made the change from bug 600812 obsolete and I reverted that. Bug 623853 should be fixed by that, too (though IMHO we should still add a testcase for it).

Comment 36

[Michael Yoshitaka Erlewine [:mitcho]]

Comment on attachment 506755 [details] [diff] [review]
patch v9

Took just a quick look. My only suggestion is that determineBestDropTarget just be moved out into its own method... it may come in handy for other things down the line.

Comment 37

[Tim Taubert [:ttaubert] (inactive)]

v9 passed try.

Comment 38

[Ian Gilman [:iangilman]]

Comment on attachment 506755 [details] [diff] [review]
patch v9

Looks great! Just one minor detail: 

>+  contains: function Rect_contains(a) {
>+    if (Utils.isPoint(a))
>+      a = new Rect(a.x, a.y, 0, 0);
>+
>+    return (a.left > this.left &&
>+            a.right < this.right &&
>+            a.top > this.top &&
>+            a.bottom < this.bottom);
>   },

It seems wasteful to create a new Rect here just to keep the code path unified. I'd prefer a point-specific version of the test, like so: 

  contains: function Rect_contains(a) {
    if (Utils.isPoint(a))
      return (a.x > this.left &&
              a.x < this.right &&
              a.y > this.top &&
              a.y < this.bottom);

    return (a.left > this.left &&
            a.right < this.right &&
            a.top > this.top &&
            a.bottom < this.bottom);
  },

r+ with that.

Comment 39

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch v10

Comment 40

[Tim Taubert [:ttaubert] (inactive)]

Passed try!

Comment 41

[Shawn Wilsher :sdwilsh]

Comment on attachment 507379 [details] [diff] [review]
patch v10

This is big and scary patch, and comment 26 indicates that you really wanted it for beta 10.  It's looking like beta 11 is going to be our last beta, and if any regressions happen to come out from this, we won't be able to get beta feedback on it.  As a result, I'm going to say we'll have to punt on this for Firefox 4.

Comment 42

[Kevin Hanes]

Tim, would you please un-bitrot this patch and flag for check in? The tree isn't currently approval required, though it is managed by the sheriff.

Comment 43

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin

Pushed to try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=d9dd69c4224d

Comment 44

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 522036 [details] [diff] [review]
patch for checkin

Passed try:

http://tbpl.mozilla.org/?tree=MozillaTry&pusher=[e-mail]&rev=d9dd69c4224d

Comment 45

[Tim Taubert [:ttaubert] (inactive)]

Attachment: patch for checkin

Comment 46

[(no longer active)]

http://hg.mozilla.org/projects/cedar/rev/f05317bf91bd

Comment 47

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/f05317bf91bd

Comment 48

[George Carstoiu]

Verified with Mozilla/5.0 (X11; Linux i686; rv:2.2a1pre) Gecko/20110404 Firefox/4.2a1pre

Comment 49

[Stefan]

Regression? At least since 2011-05-10-03-mozilla-central there are cases when tabs are resized with cursor staying in group. STR:

1. Create three (empty) tabs.
2. Set focus to the first tab.
3. Enter Panorama (ctrl-e).
4. Click at "X" of the first tab.

Actual Results: resize occurs
Expected Result: no resize

Comment 50

[Tim Taubert [:ttaubert] (inactive)]

(In reply to comment #49)
> Regression? At least since 2011-05-10-03-mozilla-central there are cases
> when tabs are resized with cursor staying in group.

Thanks for reporting. Filed bug 665502.