713450 - (tab-slide) Close tabs by slide

Status: VERIFIED FIXED

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

Description

[Michael Monreal [:micmon]]

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.

Comment 1

[Mark Finkle (:mfinkle) (use needinfo?)]

Madhava - Feature request. Probably won't make Fx11, but could make Fx12.

Comment 3

[Wesley Johnston (:wesj)]

Attachment: Patch

Played with this a bit for fun. This works although the Android Flick detector seems bad at detecting quick flicks like you'd want.

Comment 4

[Wesley Johnston (:wesj)]

Attachment: Patch

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.

Comment 5

[Wesley Johnston (:wesj)]

Attachment: Move close button patch

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.

Comment 6

[Wesley Johnston (:wesj)]

Attachment: Screenshot

Comment 7

[Wesley Johnston (:wesj)]

Comment on attachment 591008 [details] [diff] [review]
Patch

I found a bug!

Comment 8

[Wesley Johnston (:wesj)]

Attachment: WIP

WIP. Pretty rough. Just trying to get the right behavior (still not there).

Comment 9

[Wesley Johnston (:wesj)]

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;

Comment 10

[Wesley Johnston (:wesj)]

Attachment: WIP Patch

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.

Comment 11

[Wesley Johnston (:wesj)]

Attachment: WIP

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

Comment 12

[Wesley Johnston (:wesj)]

Attachment: Patch v1

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.

Comment 13

[Wesley Johnston (:wesj)]

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.

Comment 14

[Sriram Ramasubramanian [:sriram]]

Attachment: WIP

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.

Comment 15

[Wesley Johnston (:wesj)]

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.

Comment 16

[Wesley Johnston (:wesj)]

Attachment: Patch v2

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!

Comment 17

[Wesley Johnston (:wesj)]

http://people.mozilla.com/~wjohnston/slidetabs.apk build with this for anyone playing along

Comment 18

[Wesley Johnston (:wesj)]

Attachment: Patch v2.1

Fixed a tiny problem with the last tab on screen.

Comment 19

[Wesley Johnston (:wesj)]

Attachment: Patch v3 - the unbitrotting

Unbitrotted

Comment 20

[Sriram Ramasubramanian [: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.

Comment 21

[Wesley Johnston (:wesj)]

Attachment: Patch 3.1

> 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.

Comment 22

[Sriram Ramasubramanian [:sriram]]

Comment on attachment 632940 [details] [diff] [review]
Patch 3.1

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

Looks good to me.

Comment 23

[Wesley Johnston (:wesj)]

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.

Comment 24

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/6d05ace9ed4f

Comment 25

[Wesley Johnston (:wesj)]

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.

Comment 26

[Alex Keybl [:akeybl]]

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.

Comment 27

[Alex Keybl [:akeybl]]

(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.

Comment 28

[Wesley Johnston (:wesj)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7fbc80d114d0

Comment 29

[Michael Monreal [:micmon]]

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

Comment 30

[Wesley Johnston (:wesj)]

Should be the same code on Nightly that's on Aurora right now.
File a new bug please. Device info would be helpful too.

Comment 31

[Paul Feher]

Verified/fixed on:
Nightly Fennec 16.0a1 (2012-07-08)
Aurora Fennec 15.0a2 (2012-07-09)
Using:
HTC Desire Z (2.3.3)

Comment 32

[Adrian Tamas (:AdrianT)]

Featured covered by the tests:
https://moztrap.mozilla.org/manage/cases/_detail/5319/
https://moztrap.mozilla.org/manage/cases/_detail/5315/