Bug 713450 (tab-slide)

Close tabs by slide

VERIFIED FIXED in Firefox 15

Status

()

P4
normal
VERIFIED FIXED
7 years ago
2 years ago

People

(Reporter: micmon, Assigned: wesj)

Tracking

({uiwanted})

12 Branch
Firefox 16
ARM
Android
uiwanted
Points:
---
Dependency tree / graph
Bug Flags:
in-moztrap +

Firefox Tracking Flags

(firefox15 verified, firefox16 verified)

Details

(Whiteboard: tabs-ux)

Attachments

(1 attachment, 12 obsolete attachments)

(Reporter)

Description

7 years ago
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
(Assignee)

Comment 3

7 years ago
Created attachment 587216 [details] [diff] [review]
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.
Assignee: madhava → wjohnston
Keywords: uiwanted
(Assignee)

Comment 4

7 years ago
Created attachment 591008 [details] [diff] [review]
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.
Attachment #587216 - Attachment is obsolete: true
Attachment #591008 - Flags: review?(mark.finkle)
(Assignee)

Comment 5

7 years ago
Created attachment 591009 [details] [diff] [review]
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.
(Assignee)

Comment 6

7 years ago
Created attachment 591010 [details]
Screenshot
(Assignee)

Updated

7 years ago
Attachment #591010 - Attachment mime type: text/plain → image/png
(Assignee)

Comment 7

7 years ago
Comment on attachment 591008 [details] [diff] [review]
Patch

I found a bug!
Attachment #591008 - Flags: review?(mark.finkle)
Whiteboard: tabs-ux
(Assignee)

Comment 8

7 years ago
Created attachment 594014 [details] [diff] [review]
WIP

WIP. Pretty rough. Just trying to get the right behavior (still not there).
Attachment #591008 - Attachment is obsolete: true
(Assignee)

Comment 9

7 years ago
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;
(Assignee)

Comment 10

7 years ago
Created attachment 598023 [details] [diff] [review]
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.
(Assignee)

Comment 11

7 years ago
Created attachment 629685 [details] [diff] [review]
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
Attachment #591009 - Attachment is obsolete: true
Attachment #591010 - Attachment is obsolete: true
Attachment #594014 - Attachment is obsolete: true
Attachment #598023 - Attachment is obsolete: true
(Assignee)

Comment 12

7 years ago
Created attachment 630284 [details] [diff] [review]
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.
Attachment #629685 - Attachment is obsolete: true
Attachment #630284 - Flags: review?(sriram)
(Assignee)

Comment 13

7 years ago
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.
Created attachment 630304 [details] [diff] [review]
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.
Attachment #630304 - Flags: feedback?(wjohnston)
(Assignee)

Comment 15

7 years ago
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)
(Assignee)

Comment 16

7 years ago
Created attachment 632485 [details] [diff] [review]
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!
Attachment #630284 - Attachment is obsolete: true
Attachment #630304 - Attachment is obsolete: true
Attachment #630284 - Flags: review?(sriram)
Attachment #632485 - Flags: review?(sriram)
(Assignee)

Comment 17

7 years ago
http://people.mozilla.com/~wjohnston/slidetabs.apk build with this for anyone playing along
(Assignee)

Comment 18

7 years ago
Created attachment 632762 [details] [diff] [review]
Patch v2.1

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)
(Assignee)

Comment 19

7 years ago
Created attachment 632773 [details] [diff] [review]
Patch v3 - the unbitrotting

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+
(Assignee)

Comment 21

7 years ago
Created attachment 632940 [details] [diff] [review]
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.
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+
(Assignee)

Comment 23

7 years ago
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
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 16
Flags: in-moztrap?(fennec)
Depends on: 764812
Alias: tab-slide
Depends on: 765165
(Assignee)

Comment 25

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

Comment 29

7 years ago
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
(Assignee)

Comment 30

7 years ago
Should be the same code on Nightly that's on Aurora right now.
File a new bug please. Device info would be helpful too.
status-firefox15: --- → fixed
Blocks: 769867

Comment 31

7 years ago
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
status-firefox15: fixed → verified
status-firefox16: --- → verified
Depends on: 772493
Depends on: 772940
Depends on: 778625
Depends on: 787335
Featured covered by the tests:
https://moztrap.mozilla.org/manage/cases/_detail/5319/
https://moztrap.mozilla.org/manage/cases/_detail/5315/
Flags: in-moztrap?(fennec) → in-moztrap+
You need to log in before you can comment on or make changes to this bug.