Closed Bug 713450 (tab-slide) Opened 8 years ago Closed 8 years ago

Close tabs by slide

Categories

(Firefox for Android :: General, defect, P4)

12 Branch
ARM
Android
defect

Tracking

()

VERIFIED FIXED
Firefox 16
Tracking Status
firefox15 --- verified
firefox16 --- verified

People

(Reporter: micmon, Assigned: wesj)

References

Details

(Keywords: uiwanted, Whiteboard: tabs-ux)

Attachments

(1 file, 12 obsolete files)

It would be nice to be able to close tabs by sliding them out of the screen, similar to how notifications can be dismissed and apps be closed in Android 4.

Right now the close "x" is a rather small target. Also, it is positioned on the far left side, making it impossible to target with the right thumb when using the phone one-handed.
Madhava - Feature request. Probably won't make Fx11, but could make Fx12.
Assignee: nobody → madhava
Priority: -- → P4
Duplicate of this bug: 715345
Attached patch Patch (obsolete) — Splinter Review
Played with this a bit for fun. This works although the Android Flick detector seems bad at detecting quick flicks like you'd want.
Assignee: madhava → wjohnston
Keywords: uiwanted
Attached patch Patch (obsolete) — Splinter Review
Adds a touch listener and some logic to detect swipes to the right. There's some adjustable parameters here if we want to play with this a bit.
Attachment #587216 - Attachment is obsolete: true
Attachment #591008 - Flags: review?(mark.finkle)
Attached patch Move close button patch (obsolete) — Splinter Review
This moves the close button to the other side of the rows as well. I'll throw up a screenshot in a sec and ping ux about both of these in the morning.
Attached image Screenshot (obsolete) —
Attachment #591010 - Attachment mime type: text/plain → image/png
Comment on attachment 591008 [details] [diff] [review]
Patch

I found a bug!
Attachment #591008 - Flags: review?(mark.finkle)
Whiteboard: tabs-ux
Attached patch WIP (obsolete) — Splinter Review
WIP. Pretty rough. Just trying to get the right behavior (still not there).
Attachment #591008 - Attachment is obsolete: true
Finally dug up where this is in the source (at least for the browser). There is no platform code for this apparently. Just a ScrollerView class in the browser component that does a whole lot of math to determine where you moved your finger to and whether or not its a fling. 

For my own reference, they close the tab if 

dx > view.getWidth() / 2
or
Math.abs(velocity) > mFlingVelocity / 2

where mFlingVelocity = getDisplayMetrics().density * 1500;
Attached patch WIP Patch (obsolete) — Splinter Review
I don't think this even builds, but its more in line with what I think we want this to look like.... putting it up for sriram to have a go.
Attached patch WIP (obsolete) — Splinter Review
Updated to be a bit cleaner (and work!). Still have some flickery jank jumping between states though that I want to look at. Build for UX (or anyone) to try out:

http://people.mozilla.com/~wjohnston/slideClose.apk
Attachment #591009 - Attachment is obsolete: true
Attachment #591010 - Attachment is obsolete: true
Attachment #594014 - Attachment is obsolete: true
Attachment #598023 - Attachment is obsolete: true
Attached patch Patch v1 (obsolete) — Splinter Review
spoke to ibarlow who said this looks pretty good except for two bugs:

1.) I'm sliding the entire row including its "background". I had to do that because Android was causing flickering if I didn't. Maybe there's a fix for that. Hoping sriram knows?

2.) We can't smoothly slide the old rows up after you close the tab. Pre-ICS Android really hates animations that try to change layout. Maybe we can write our own Handler for the animation that does that?

ibarlow said he'd be ok landing this on nightly for some feedback, so I'm putting it up for review and, if no one else has problems, we can look at those two points in follow ups.
Attachment #629685 - Attachment is obsolete: true
Attachment #630284 - Flags: review?(sriram)
Also, this will probably drive you nuts because I had to do some things like custom tap detection. Trying to attach touch listeners to tabs themselves, the touch events are eaten by the list if you drag your finger vertically far enough to start "scrolling". Hence, if you start dragging the tab right but drift up slightly we quit dragging, which is annoying. I wanted to be able to do my own locking.

However, in order for the tab to not eat touch events during that initial touch (pre-scrolling) I also had to remove the click listener on the tab. Basically, there are far to many people trying to eat the touch events here, so I killed them all and just wrote my own.
Attached patch WIP (obsolete) — Splinter Review
This is my old patch, bit-trotted.
This is pretty rough. The fling action works. The swipe works too (but a bit hard).
The problem is, the gesture should be listened at activity level and passed on to the view, in which case, things will be smoother, including fling action.
I didn't have patience to try it then ;)
I can try it now.
Attachment #630304 - Flags: feedback?(wjohnston)
Comment on attachment 630304 [details] [diff] [review]
WIP

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

Yeah, I like the idea of using a scrolling view. I'm happy to get rid of the alpha transition in favor of that. But having the tabs animate off or back in place when you release or click close is nice too. UX can weigh in on how important those things are if they want. We could write a Handler to do animated scrolling...

The rest is just "edge cases". Even if you drag your finger off of the tab you're flinging, I think the drag should continue. So you need to hold a reference to the dragged tab when the user taps on it. That may fix the issues I was seeing with flick events not firing enough of the time. It also means you have to battle for events with the click listeners on the TabRow (although that could very well just be my stupidity).

This seems to close tabs (sometimes?) if you drag to the right and then slightly up, even though your finger is still on the screen. i.e. tab close should only happen when on MOTION_UP events.

Needs to adapt for people who want to drag tabs to the left or right as well.

I can give more feedback when its closer to done.
Attachment #630304 - Flags: feedback?(wjohnston)
Attached patch Patch v2 (obsolete) — Splinter Review
Mashup of both patches. I'm not sure this is really less complex, or just written differently, but I like the scrolling animation a lot better!
Attachment #630284 - Attachment is obsolete: true
Attachment #630304 - Attachment is obsolete: true
Attachment #630284 - Flags: review?(sriram)
Attachment #632485 - Flags: review?(sriram)
http://people.mozilla.com/~wjohnston/slidetabs.apk build with this for anyone playing along
Attached patch Patch v2.1 (obsolete) — Splinter Review
Fixed a tiny problem with the last tab on screen.
Attachment #632485 - Attachment is obsolete: true
Attachment #632485 - Flags: review?(sriram)
Attachment #632762 - Flags: review?(sriram)
Attached patch Patch v3 - the unbitrotting (obsolete) — Splinter Review
Unbitrotted
Attachment #632762 - Attachment is obsolete: true
Attachment #632762 - Flags: review?(sriram)
Attachment #632773 - Flags: review?(sriram)
Comment on attachment 632773 [details] [diff] [review]
Patch v3 - the unbitrotting

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

Overall this patch is beautiful!! :) Less complex than the first one :)

I somehow like SLIDE_LEFT and SLIDE_TOP better, as if we do SLIDE_BOTTOM and SLIDE_RIGHT, we might need not need to reverse the "to" field. SLIDE_Y loses that "reversal" thing we do, in its name.

https://bugzilla.mozilla.org/attachment.cgi?id=632773&action=diff#a/mobile/android/base/TabsTray.java_sec2

This lost the "mWaitingForClose". This was added to avoid some race conditions long back. Please add that check again.

https://bugzilla.mozilla.org/attachment.cgi?id=632773&action=diff#a/mobile/android/base/TabsTray.java_sec3

The if (closeAfter) should be "if (closeAfter && !mWaitingForClose)"

finalPos = -1*mView.getWidth(); <-- Please add spacing.

animateTo(mView, finalPos, MAX_ANIMATION_TIME, finalPos != 0 && !mWaitingForClose);
Do we need the check there? Can't we have it in animateTo() as,

if (x == 0) , then close;
else, dont close;

| } else if (mView != null && dir == DragDirection.UNKNOWN) { |

What happens for vertical direction? And, I don't think this should select the row. Select should be done on click. In a list, if you press down on a view and keep moving away, it shouldn't select the view, just the focus on any view should be lost.

Based on the clarifications, I would review this patch.
Attachment #632773 - Flags: review?(sriram) → feedback+
Attached patch Patch 3.1Splinter Review
> I somehow like SLIDE_LEFT and SLIDE_TOP better.
I don't agree with this, but I'll file a different bug for this renaming. We can debate there I guess.

> This lost the "mWaitingForClose". This was added to avoid some race
> conditions long back. Please add that check again.
I put one in the animateTo stuff.

> Do we need the check there? Can't we have it in animateTo() as,
> if (x == 0) , then close;
At first I thought this was too indirect, but I think I'm fine with it.

> What happens for vertical direction?
If you're moving vertically the list will scroll on its own. We don't need to do anything I think.

> And, I don't think this should select the row. Select should be done on click. In 
That's what this does. If you start scrolling, we remove the highlight. Its sorta its own "click" detector. We could use the GestureDetector's click detection code, but since I did the scroll detection and axis locking, it seemed kinda redundant.
Attachment #632773 - Attachment is obsolete: true
Attachment #632940 - Flags: review?(sriram)
Comment on attachment 632940 [details] [diff] [review]
Patch 3.1

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

Looks good to me.
Attachment #632940 - Flags: review?(sriram) → review+
I forgot to change the author on this:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6d05ace9ed4f

so if it breaks anything, sriram will take the blame.
https://hg.mozilla.org/mozilla-central/rev/6d05ace9ed4f
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Flags: in-moztrap?(fennec)
Depends on: 764812
Alias: tab-slide
Depends on: 765165
Comment on attachment 632940 [details] [diff] [review]
Patch 3.1

[Approval Request Comment]
Bug caused by (feature/regressing bug #): New feature. Parity with competitors
User impact if declined: Little less nice to use
Testing completed (on m-c, etc.): Has been on central for a week
Risk to taking this patch (and alternatives if risky): Low risk for regressions. Mobile only. Mostly new behaviour.
String or UUID changes made by this patch: None.
Attachment #632940 - Flags: approval-mozilla-aurora?
Comment on attachment 632940 [details] [diff] [review]
Patch 3.1

[Triage Comment]
Tablet UI bugs are approved for Aurora 15, as they carry little risk to mobile and none to desktop.
Attachment #632940 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
(In reply to Alex Keybl [:akeybl] from comment #26)

Apologies, errant mass modify there. I see no reason to slow this feature down - we should be able to fully qualify it before FN15's release.
Do we need a new bug for visual issues? While the slide out looks wine when done *really* slow it looks horrible when done with a quick flick... not sure how this looks on Nightly though, I can only test Aurora atm
Should be the same code on Nightly that's on Aurora right now.
File a new bug please. Device info would be helpful too.
Blocks: 769867
Verified/fixed on:
Nightly Fennec 16.0a1 (2012-07-08)
Aurora Fennec 15.0a2 (2012-07-09)
Using:
HTC Desire Z (2.3.3)
Status: RESOLVED → VERIFIED
Depends on: 772493
Depends on: 772940
Depends on: 778625
Depends on: 787335
You need to log in before you can comment on or make changes to this bug.