Improve reordering tabs in groups

VERIFIED FIXED in Firefox 4.0b10

Status

defect
P3
normal
VERIFIED FIXED
9 years ago
3 years ago

People

(Reporter: micmon, Assigned: mitcho)

Tracking

Trunk
Firefox 4.0b10
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 final+)

Details

(Whiteboard: [softblocker])

Attachments

(4 attachments, 13 obsolete attachments)

1008.49 KB, video/ogg
Details
695.68 KB, video/ogg
Details
1.29 MB, video/ogg
Details
34.22 KB, patch
Details | Diff | Splinter Review
Reporter

Description

9 years ago
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.
Blocks: 586548
No longer blocks: 586548
Depends on: 586548
Duplicate of this bug: 587555
Blocks: 585689
Depends on: 580412

Comment 2

9 years ago
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.
Hardware: x86 → All
Priority: -- → P3
Duplicate of this bug: 585928

Comment 4

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

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

Updated

9 years ago
Duplicate of this bug: 604766
This makes using tabcandy to actually manage tabs a huge pain...
status2.0: --- → ?

Comment 7

9 years ago
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).
Assignee: nobody → mitcho
Status: NEW → ASSIGNED
blocking2.0: --- → ?
status2.0: ? → ---
If we're shipping Panorama, it should not be annoying to manage tabs with. Blocking+.
blocking2.0: ? → final+

Updated

9 years ago
Blocks: 597043
Posted patch Patch v1 (obsolete) — Splinter Review
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.
Attachment #488792 - Flags: feedback?(ian)
Posted video Video of patch v1 (obsolete) —
Attachment #488794 - Flags: ui-review?(aza)
For some reason the webm encoding I tried to do didn't actually encode the whole thing. Here's the whole thing in Theora.
Attachment #488794 - Attachment is obsolete: true
Attachment #488794 - Flags: ui-review?(aza)
Attachment #488795 - Flags: ui-review?(aza)
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.
Attachment #488792 - Flags: feedback?(ian) → feedback+
Posted patch Patch v1.1 (obsolete) — Splinter Review
Attachment #488792 - Attachment is obsolete: true
Attachment #489006 - Flags: review?(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.
Attachment #489006 - Flags: review?(dietrich) → review-

Comment 15

9 years ago
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.
(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.

Updated

9 years ago
Attachment #488795 - Flags: ui-review?(aza) → ui-review+
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 on attachment 488795 [details]
Video of patch v1 (theora)

Please see the above comment.
Attachment #488795 - Flags: ui-review+ → ui-review?
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.
Posted video Video of patch v1.2 (obsolete) —
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?
Attachment #495308 - Flags: ui-review?(aza)
Attachment #495308 - Attachment is patch: false
Attachment #495308 - Attachment mime type: text/plain → video/ogg
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

9 years ago
(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.
Attachment #495308 - Attachment is obsolete: true
Attachment #495311 - Flags: ui-review?(aza)
Attachment #495308 - Flags: ui-review?(aza)
Attachment #488795 - Flags: ui-review?

Updated

9 years ago
Attachment #495311 - Flags: ui-review?(aza) → ui-review+
Posted patch Patch v1.2 with test (obsolete) — Splinter Review
Attachment #489006 - Attachment is obsolete: true
Attachment #495329 - Flags: review?(dolske)
Duplicate of this bug: 616732

Comment 26

9 years ago
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
No longer blocks: 585689

Updated

9 years ago
Blocks: 598154

Comment 27

9 years ago
Beta8 has 1 bug left on it, moving our blockers to b9
No longer blocks: 597043
(In reply to comment #24)
> Created attachment 495329 [details] [diff] [review]
> Patch v1.2 with test

For reference, this failed try. Investigating.
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?
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?
(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.
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?
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

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

Can you please let me know exactly how I can help here?
Posted patch Patch v1.3 (obsolete) — Splinter Review
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.
Attachment #495329 - Attachment is obsolete: true
Attachment #498531 - Flags: review?(ian)
Attachment #495329 - Flags: review?(dolske)
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?
Sean's been debugging a leak in bug 567029; Sean, perhaps this one is related?
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.
Attachment #498531 - Flags: review?(ian) → review-
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.
(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.
Posted patch Updated RTL test (obsolete) — Splinter Review
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.
Attachment #499674 - Flags: review?(ian)
Attachment #499674 - Flags: feedback?(ehsan)
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.
Whoops... was missing the test file.
Attachment #498531 - Attachment is obsolete: true
Attachment #499684 - Attachment is obsolete: true
Attachment #499689 - Flags: review?(ian)
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 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.
Attachment #499674 - Flags: review?(ian)
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.
Attachment #499689 - Flags: review?(ian) → review+

Comment 47

9 years ago
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...
Attachment #499674 - Flags: feedback?(ehsan) → feedback-
(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
Keywords: helpwanted
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.
Perhaps Ehsan can help here?
(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.
Blocks: 616729
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

9 years ago
bugspam (moving b9 to b10)
Blocks: 608028

Comment 54

9 years ago
bugspam (removing b9)
No longer blocks: 598154
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.
Attachment #502595 - Attachment description: Patch for minimum setup to produce the leak → [LEAK] Patch for minimum setup to produce the leak
Posted file [LEAK] JS heap dump (obsolete) —
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.
Depends on: 624722
Bug 624722 indeed does seem to fix this! Thank you!

Pushing patch v1.5 without the updated rtl test to try.
Posted patch patch for checkin (obsolete) — Splinter Review
Just waiting for try confirmation.
Attachment #499674 - Attachment is obsolete: true
Attachment #499689 - Attachment is obsolete: true
Attachment #502595 - Attachment is obsolete: true
Attachment #502597 - Attachment is obsolete: true
Attachment #502598 - Attachment is obsolete: true
Keywords: helpwanted
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!
Whiteboard: [softblocker]
This burned on trunk; removing "checkin-needed" flag
Keywords: checkin-needed
Last patch burned because of 611715. Sigh.
Attachment #502872 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/9217634f7b9e
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 4.0b10
verified with nightly build of minefield.
Status: RESOLVED → VERIFIED
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.