587503 - Improve reordering tabs in groups

Status: VERIFIED FIXED

(Firefox Graveyard :: Panorama, defect, P3)

Description

[Michael Monreal [:micmon]]

Right now it is seemingly impossible to reorder tabs inside a group the way you want. For example, try to make two tabs switch postion or move the first trab to the third position etc.

Slightly moving the not-tragged tab would help here, see the new tab screen on Google Chrome for a nicely working implementation.

Comment 2

[Ethan Sisson]

As far as the visual feedback for this, I think Chrome's new tab page isn't effective. In Chrome, if you drag one page preview over another, the latter moves ~15px in the direction of the former. This is pretty vague.

I would propose either:

1) Use a position marker, like when reordering tabs in Firefox 3.
2) While dragging one page preview in Panorama, other page previews actively shift around to communicate literally what the order will be if the user drops the page where their cursor is. This is similar to Safari's tab reordering, but obviously would involve multiple rows.

Option 2 is preferable.

Comment 4

[Jeff Rodriguez]

I was thinking visual feedback like jQuery UI's "sortable":

http://jqueryui.com/demos/sortable/#display-grid

Comment 6

[Boris Zbarsky [:bzbarsky]]

This makes using tabcandy to actually manage tabs a huge pain...

Comment 7

[The 8472]

As mentioned in bug 604766, it's not just some usability issues (e.g. lack of visual feedback). The implementation as it is is completely broken.

If you have 6+ tabs in 2+ rows it'll only reinsert the tab at the 1st or the last position in the tab group when you drag&drop it, regardless where you actually dropped it (occasionally also into the 2nd position for some reason).

Comment 8

[Dietrich Ayala (:dietrich)]

If we're shipping Panorama, it should not be annoying to manage tabs with. Blocking+.

Comment 9

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v1

Note: I personally really like what came together. In particular, by computing the index where it will be inserted during the dragging, I could completely remove the findInsertionPoint code.

Comment 10

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Video of patch v1

Comment 11

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Video of patch v1 (theora)

For some reason the webm encoding I tried to do didn't actually encode the whole thing. Here's the whole thing in Theora.

Comment 12

[Ian Gilman [:iangilman]]

Comment on attachment 488792 [details] [diff] [review]
Patch v1

Nice! I think there's a bug somewhere for this sort of dynamic preview; make sure to resolve that once this one's resolved. 

I suppose one thing we need to think about is performance... how does this behave on a netbook when dragging over a group of a hundred tabs?

>-    var rects = Items.arrange(null, bb, options);
>+    var {rects} = Items.arrange(null, bb, options);

I think it would be better to be explicit here, perhaps like so: 

    var rects = Items.arrange(null, bb, options).rects;
    
Also, as long as you're there, switch it over to let.

>+      let skip = false;

Looks like you're not really using this... you set it to false and then later check if it's false, but there's never a chance for it to be set to true, so you might as well get rid of it.

Comment 13

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v1.1

Comment 14

[Dietrich Ayala (:dietrich)]

Comment on attachment 489006 [details] [diff] [review]
Patch v1.1


>   // ----------
>   // Function: add
>   // Adds an item to the groupItem.
>   // Parameters:
>   //
>   //   a - The item to add. Can be an <Item>, a DOM element or an iQ object.
>   //       The latter two must refer to the container of an <Item>.
>-  //   dropPos - An object with left and top properties referring to the location dropped at.  Optional.
>-  //   options - An object with optional settings for this call. Currently this includes dontArrange
>-  //       and immediately
>-  add: function GroupItem_add(a, dropPos, options) {
>+  //   options - An object with optional settings for this call. Currently this
>+  //             includes dontArrange, immediately, and an index
>+  add: function GroupItem_add(a, options) {

please document the options object.

also, needs a test. should be good to go with those.

Comment 15

[dmcadmin]

I just wanted to add that in beta7 (Ubuntu), moving the second tab of a group just a few pixels to the right causes it to become the first tab in the group. This is extremely annoying and I hope that it will be taken care of.

Comment 16

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #15)
> I just wanted to add that in beta7 (Ubuntu), moving the second tab of a group
> just a few pixels to the right causes it to become the first tab in the group.
> This is extremely annoying and I hope that it will be taken care of.

Yes, this will be fixed by this patch.

Comment 17

[Aza Raskin [:aza]]

Mitcho: quick question. What happens when you drag a tab across other groups. The thing I'm really worried about is that as you drag a tab from one place to another, all the in-group tabs shuffle around creating a somewhat jarring experience.

A solution is to add a slight delay for which you have to slow down tab motion/pause over a group for which that tab wasn't originally a member before the new behavior kicks in.

Comment 18

[Aza Raskin [:aza]]

Comment on attachment 488795 [details]
Video of patch v1 (theora)

Please see the above comment.

Comment 19

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Another video of patch v1.1

Aza, for reference: this is what the situation looks like right now. Indeed, when you drag things over other groups, it'll shuffle things around.

Comment 20

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Video of patch v1.2

Here's a video of a patch, having incorporated Aza's recent feedback. Changes/features include:

- waiting (currently 1 sec) before rearranging "fly-over" groups. There must be a 1 second period where we keep the same dropTarget and the same (predicted) dropIndex. This state internally is controlled by a _dropSpaceActive flag on the GroupItem. When we activate dropSpace, we also trigger a rearrange *with* animate = true which I think has a nice effect.
- Once we have _dropSpaceActive, as long as you stay in that group and move around, the dropSpace will follow immediately.
- When you leave the group, it will reset _dropSpaceActive, so you have to wait 1s again when you come back.
- You can, of course, just drop immediately without waiting for the dropSpace to open up. In that case, it will recalculate a correct dropSpace at that time.
- If the tab started as a child of a group, that first group (the then-parent group) will start with _dropSpaceActive. Otherwise when we pick up a group it would immediately collapse underneath it for a second.

I tried to demo most of these features in action in the video.

Aside from overall feedback, a specific QUESTION FOR Aza: is 1 second too long?

Comment 21

[Aza Raskin [:aza]]

From watching the video, yes 1 second seems to long. My guess for the right way to do this is that if the tab stops moving over a new group for longer than 150ms it should engage the pretty animations.

Comment 22

[The 8472]

(In reply to comment #17)
> A solution is to add a slight delay for which you have to slow down tab
> motion/pause over a group for which that tab wasn't originally a member before
> the new behavior kicks in.

As mentioned on IRC delays are bad for usability because you get visual feedback later, giving the impression of a sluggish UI. Users are used to that, but they react to it by slowing down their workflow to wait for the UI to show a response. If you're mass-moving tabs this would mean a serious slowdown.

To solve the "fly-over" problem there should simply be an instantanous insertation bar (something like this: ][ ) between the tab previews where it would be inserted.
Some fancy animation could be done later on when the user hovers over the same position long enough.
And by "long enough" I mean something around 200ms. Even someone who moves his mouse slowly will still *move* it, thus not matching the "hovering over the same position" condition.

Comment 23

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Video of patch v1.2 with 250ms delay

Comment 24

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v1.2 with test

Comment 26

[blindskull13]

i posted bug 616729 (https://bugzilla.mozilla.org/show_bug.cgi?id=616729) a short while ago, and i believe that the problem i mentioned there is related to this

Comment 27

[Kevin Hanes]

Beta8 has 1 bug left on it, moving our blockers to b9

Comment 28

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #24)
> Created attachment 495329 [details] [diff] [review]
> Patch v1.2 with test

For reference, this failed try. Investigating.

Comment 29

[Michael Yoshitaka Erlewine [:mitcho]]

I tried sending it to try with a slightly modified test, taking more steps to carry out the dragging, but no dice. It's frustrating, because it passes locally for me (Mac) and even fails the mac builds on try.

Ian et al, could you try this out locally? It clearly is an issue with the finicky-ness of reproducing mouse event sequences.

Alternatively, would it be appropriate to just get a litmus test for this?

Comment 30

[Ian Gilman [:iangilman]]

I tried it locally, ran the tab view tests; no failures. One factor on the test server is that supposedly it runs things a lot slower, like sometimes an order of magnitude. Not sure exactly how that would affect the test, though.

What specifically is failing?

It'd be nice to have this test automated, but if it looks like it's not going to be possible, I'd support doing it with litmus.

Ehsan, any thoughts?

Comment 31

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #30)
> I tried it locally, ran the tab view tests; no failures. One factor on the test
> server is that supposedly it runs things a lot slower, like sometimes an order
> of magnitude. Not sure exactly how that would affect the test, though.
> 
> What specifically is failing?

It seems to fail on the last step: having dragged a tab out of the group, dragging it back into the group. The test is to verify that we actually drag it into the correct position but, for starters, it's not even registering that we dropped it into the group.

Not being able to see actually *where* the tab was moved, visually, is also frustrating. :( But I agree that getting this automated would be ideal.

Comment 32

[Ian Gilman [:iangilman]]

I guess you could wire it up with a whole bunch of logging and send it to try with just one platform + mochitests...

Is it possible you're running into a screen size issue, or something similar to what was happening with the snapping test?

Comment 33

[Ian Gilman [:iangilman]]

By the way, feel free to assign the next version of the patch to me for review if you'd like (rather than Dolske, who is a bit swamped).

Comment 34

[(no longer active)]

(In reply to comment #30)
> Ehsan, any thoughts?

Can you please let me know exactly how I can help here?

Comment 35

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v1.3

Since the last time, I've figured out what the problem with the inconsistent tests were and also touched up one thing in GroupItems.arrange().

Last try push failed, but that was a trivial fix. Just pushed this patch to try and I have a good feeling about it.

Comment 36

[Michael Yoshitaka Erlewine [:mitcho]]

Results are in: mochitest itself (with the new test) passed cleanly, but there were leaks. I'm going to try to figure out where the leak came from, but any help would be appreciated. The try revision is b266d83863de.

I get results like:

TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2452863 bytes during test execution
Analyze the leak.TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2 instances of AsyncStatement with size 48 bytes each (96 bytes total)
Analyze the leak.TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1763 instances of AtomImpl with size 20 bytes each (35260 bytes total)
Analyze the leak.TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of BackstagePass with size 24 bytes
Analyze the leak.TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 8 instances of BodyRule with size 16 bytes each (128 bytes total)
Analyze the leak.TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 6 instances of CSSImportRuleImpl with size 44 bytes each (264 bytes total)
Analyze the leak.

The test I have involves a number of setTimeouts... could that be part of the issue?

Comment 37

[Ian Gilman [:iangilman]]

Sean's been debugging a leak in bug 567029; Sean, perhaps this one is related?

Comment 38

[Ian Gilman [:iangilman]]

Comment on attachment 498531 [details] [diff] [review]
Patch v1.3

>+    if (this.item.parent) {
>+    	Utils.log('calling arrange at the end!');
>+      this.item.parent.arrange();
>+    }
>+
>     if (this.item && !this.item.parent) {

Kill the log call. There are a number more of them, but they're all commented out... still, they should probably all be removed before the patch lands.

Also, it's interesting there's a "if (this.item.parent)" with no preceeding "this.item &&", yet below it is a "if (this.item &&". Which is it? Can we count on this.item existing or not?

>+        let index = 0;
>+        let self = this;
>+        childrenToArrange.forEach(function GroupItem_arrange_children_each(child, i) {
>+          // If dropIndex spacing is active and this is a child after index,
>+          // bump it up one so we actually use the correct rect
>+          // (and skip one for the dropPos)
>+          if (self._dropSpaceActive && index === dropIndex)
>+            index++;
>+          if (!child.locked.bounds) {
>+            child.setBounds(rects[index], !options.animate);
>+            child.setRotation(0);
>+            if (options.z)
>+              child.setZ(options.z);
>+          }
>+          index++;
>+        });

Maybe I'm misreading, but it looks like each arrange moves everything around twice... once in Items.arrange(), and then again here. Seems like we only need one of them.

>   // Returns:
>   //   an object with the width value of the child items and the number of columns, 
>   //   if the return option is set to 'widthAndColumns'; otherwise the list of <Rect>s
>   arrange: function Items_arrange(items, bounds, options) {

This comment needs to be updated. 

>+    if (options.addTab)
>+      count++;
>     if (!count)
>       return rects;

This return statement is inconsistent with the one at the bottom of the routine; please fix.

Comment 39

[Ian Gilman [:iangilman]]

In terms of setTimeouts and leaks, I suppose perhaps the setTimeouts in GroupItem._addHandlers could be causing an issue, since they have DOM elements in their closures.

Comment 40

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #39)
> In terms of setTimeouts and leaks, I suppose perhaps the setTimeouts in
> GroupItem._addHandlers could be causing an issue, since they have DOM elements
> in their closures.

For reference, one try push I made yesterday where I completely removed the dropSpaceTimer setTimeout still triggered the same set of leaks.

Comment 41

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Updated RTL test

I believe I've identified the cause of the leak: it seems to be the Panorama RTL test from bug 587248. Note that it only causes a leak as part of the whole suite, not when run by itself; but removing the RTL test makes all of mochitest-other run without any leaks (locally; verifying on try now).

The RTL test has two "phases": it launches Panorama in the active window and confirms that by default it has the "ltr" dir attribute... then it changes the pref, opens Panorama again and confirms the "rtl" value. It then clears the pref and finishes. Removing the second phase (where it makes it rtl and checks it) was enough to make the leak disappear, but that makes the test pointless, so I instead added a third phase: after clearing the pref, go back into Panorama a third time to confirm the "ltr" default value, then finish.

On my machine, this patch consistently fixes the leak (just sent to try to verify). WARNING: I have no clue why this would cause a leak in conjunction with the new drag/drop/reorder behavior. My new drag/drop/reorder code is not touched during the execution of the rtl test.

Anywho, as such, here's a diff for my updated rtl test.

Comment 42

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v1.4 with updated rtl test

Just pushed to try.

(In reply to comment #38)
> Comment on attachment 498531 [details] [diff] [review]
> Patch v1.3
> 
> >     if (this.item && !this.item.parent) {
> 
> Also, it's interesting there's a "if (this.item.parent)" with no preceeding
> "this.item &&", yet below it is a "if (this.item &&". Which is it? Can we count
> on this.item existing or not?

this.item always exists, so that check's been removed.

> Maybe I'm misreading, but it looks like each arrange moves everything around
> twice... once in Items.arrange(), and then again here. Seems like we only need
> one of them.

So, some of this was legacy code... the arranging in Items.arrange() wasn't actually ever being called. Fixed.

> >   // Returns:
> This comment needs to be updated. 

Done.

> >     if (!count)
> >       return rects;
> 
> This return statement is inconsistent with the one at the bottom of the
> routine; please fix.

Done.

Also, I noticed that there was some code (computing a value called rightMostRight) which wasn't being used at all anymore, so I removed that.

Comment 43

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch v.1.5 with updated RTL test

Whoops... was missing the test file.

Comment 44

[Michael Yoshitaka Erlewine [:mitcho]]

Huzzah! It's passing try with flying colors!

Both the push where I pushed just the updated RTL test and the push with patch v1.5 (which includes the updated RTL test) are passing with no leaks!

Comment 45

[Ian Gilman [:iangilman]]

Comment on attachment 499674 [details] [diff] [review]
Updated RTL test

Unflagging my review on this patch as it's included in patch v1.5. 

Still interested in Ehsan's feedback.

Comment 46

[Ian Gilman [:iangilman]]

Comment on attachment 499689 [details] [diff] [review]
Patch v.1.5 with updated RTL test

Looks good to me, as long as Ehsan's happy with the rtl test patch.

Comment 47

[(no longer active)]

Comment on attachment 499674 [details] [diff] [review]
Updated RTL test

This patch does not fix the leak, it just masks it.  We should investigate to determine the source of the leak, and fix the leak for real.  If you can explain exactly why this leak is happening and why it's a test-only issue and not important for our users, then this patch is safe...

Comment 48

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #47)
> Comment on attachment 499674 [details] [diff] [review]
> Updated RTL test
> 
> This patch does not fix the leak, it just masks it.  We should investigate to
> determine the source of the leak, and fix the leak for real.  If you can
> explain exactly why this leak is happening and why it's a test-only issue and
> not important for our users, then this patch is safe...

True. Alright then, back to figuring out the leak then.

It took me a long time to hunt it down this far to the RTL test... I would very much appreciate anyone's help with this! Here's a starter guide:

http://www.mozilla.org/performance/leak-tutorial.html

Comment 49

[Michael Yoshitaka Erlewine [:mitcho]]

Progress (of sorts): the leak disappears when the rtl test is removed *or* if the search test is removed. Looking into the search test now.

Comment 50

[Aza Raskin [:aza]]

Perhaps Ehsan can help here?

Comment 51

[Michael Yoshitaka Erlewine [:mitcho]]

(In reply to comment #49)
> Progress (of sorts): the leak disappears when the rtl test is removed *or* if
> the search test is removed. Looking into the search test now.

Nevermind... search test doesn't seem to have to do with it. I think that was an intermittent orange of sorts.

However! I have constructed the minimal relevant test set:

browser_tabview_rtl.js (not modified, same as m-c)
browser_tabview_startup_transitions.js
browser_tabview_undo_group.js

These three tests run together yield the leak in question. Same signature; leaked 10 times out of 10.

On the other hand, running any two of the three tests above together results in no leaks of the signature we're looking at.

Comment 52

[Michael Yoshitaka Erlewine [:mitcho]]

The best debug information I've found so far is from the js heap dump:

> 0x11863e1f8 BackstagePass 11c4f4e80 via nsXPCWrappedJS[nsIDOMEventListener,0x11f3dfa90].mJSObj(0x12731c9a0 Function).upvars[0](0x12731c948 Function).parent(0x1237639d8 Call).self(0x1273b2e38 Object).proto(0x11c5c57b8 Object).addSubscriber(0x11c5dd0b0 Function).parent

Looks like we may have a cyclic reference between a DOM event listener and one of our addSubscriber functions?

Comment 53

[Kevin Hanes]

bugspam (moving b9 to b10)

Comment 54

[Kevin Hanes]

bugspam (removing b9)

Comment 55

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: [LEAK] Patch for minimum setup to produce the leak

This patch sets up a minimal-ish context for producing the leak in question. Here's what it is:

- patch v1.5
- revert the updated RTL test
- removed all tests except the three identified above

Now run mochitest-browser-chrome with a debug build on browser/base/content/tests/tabview, which just pulls up these three tests. You'll see that it has leaked with the following signature:

TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 2076923 bytes during test execution
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1412 instances of AtomImpl with size 40 bytes each (56480 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of BackstagePass with size 48 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 1 instance of BodyRule with size 32 bytes
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 6 instances of CSSImportRuleImpl with size 80 bytes each (480 bytes total)
TEST-UNEXPECTED-FAIL | automationutils.processLeakLog() | leaked 44 instances of CSSImportantRule with size 32 bytes each (1408 bytes total)

I'll upload the full mochitest log and the JS heap dump (created by setting XPC_SHUTDOWN_HEAP_DUMP) as well.

Any help appreciated.

Comment 56

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: [LEAK] mochitest-browser-chrome log (just the three tests)

Comment 57

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: [LEAK] JS heap dump

Comment 58

[Michael Yoshitaka Erlewine [:mitcho]]

FYI: either I've totally lost it, or bug 567029 (which has similarly postponed landing because of leaks) seems to be the exact same leak. Some more info in that bug.

Comment 59

[Michael Yoshitaka Erlewine [:mitcho]]

Bug 624722 indeed does seem to fix this! Thank you!

Pushing patch v1.5 without the updated rtl test to try.

Comment 60

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: patch for checkin

Just waiting for try confirmation.

Comment 61

[Michael Yoshitaka Erlewine [:mitcho]]

Try passed!

Note: os x debug leaked the first time, but was run again and didn't leak, though it had a known intermittent orange (bug 618041).

This is ready to land!

Comment 62

[Ian Gilman [:iangilman]]

This burned on trunk; removing "checkin-needed" flag

Comment 63

[Michael Yoshitaka Erlewine [:mitcho]]

Attachment: Patch for checkin

Last patch burned because of 611715. Sigh.

Comment 64

[Ian Gilman [:iangilman]]

http://hg.mozilla.org/mozilla-central/rev/9217634f7b9e

Comment 65

[Tracy Walker [:tracy]]

verified with nightly build of minefield.