1227112 - Add Peek & Pop actions to tabs in tab tray

Status: RESOLVED FIXED

(Firefox for iOS :: Theme & Visual Design, defect)

Description

[Emily Toop (:fluffyemily)]

Add a 3D Touch peek action to tab tray tab cells. 

When 3D Touch applied, show larger version of tab tray snapshot (peek).
Scroll up to display further options
Options to be:

* Add to Reading List (if not already in reading list)
* Add to Bookmarks (if not already bookmarked)
* Send to Device (invokes Send Tab extension for URL)(needs clarification when this should be displayed)
* Copy URL (copies URL to clipboard)(needs clarification when this should be displayed)
* Close Tab (needs clarification when this should be displayed)

Add pop action to go straight to tab when deeper 3D touch detected

Comment 1

[Emily Toop (:fluffyemily)]

:tecgirl: can you please provide some clarification as to when the Sent to Device/Copy URL/Close Tab options should be displayed?

Comment 2

[Robin Andersen [:tecgirl]]

(In reply to Emily Toop (:fluffyemily) from comment #1)
> :tecgirl: can you please provide some clarification as to when the Sent to
> Device/Copy URL/Close Tab options should be displayed?

Send to Device would show if the user is signed in to sync and has at least [1] connected device.
Copy URL would show if the user is signed in yet has [0] connected devices or they are not signed in.
Close Tab will show for any P&P instance on the tabs tray.

Comment 3

[Emily Toop (:fluffyemily)]

Attachment: Pull request

Comment 4

[Robin Andersen [:tecgirl]]

Comment on attachment 8698499 [details] [review]
Pull request

We should be able to distinguish between tabs and tabs of panel views, so as not to offer "Add to Bookmarks" or "Copy URL" for a panel tab. [see http://c.tecgirl.com/e5jj] 

So far I've only been able to find one example, but what would disqualify a P&P view from being able to provide "Add to Bookmarks"? [see http://c.tecgirl.com/e5jo]

Holding + until we can resolve the first nit. The second may require a separate bug. I'll see if I can find another example.

Comment 5

[Richard Newman [:rnewman]]

(In reply to Robin Andersen [:tecgirl] from comment #4)

> So far I've only been able to find one example, but what would disqualify a
> P&P view from being able to provide "Add to Bookmarks"? [see
> http://c.tecgirl.com/e5jo]

You might want to use self.profile.history.isIgnoredURL. That returns true for about:, for localhost pages, and whatever else we don't want to put into history.

Comment 6

[Brian Nicholson (:bnicholson)]

Comment on attachment 8698499 [details] [review]
Pull request

Looking good, but I think this could make better use of delegates/interfaces to avoid coupling BVC directly to the controllers here. Also, note that I'm reviewing by code inspection only since I don't have a 3D Touch device; your call if you want another reviewer who can actually test it out.

Comment 7

[Emily Toop (:fluffyemily)]

Comment on attachment 8698499 [details] [review]
Pull request

Brian: I've addressed all your issues
:tecgirl - I've address all of yours
:st3fan - I've added you as you have a 3D touch device and can add additional testing.

Comment 8

[Robin Andersen [:tecgirl]]

:fluffyemily - did you intend on taking out "Add to Reading List"? I'm not seeing that as an option anymore.

Comment 9

[Brian Nicholson (:bnicholson)]

Comment on attachment 8698499 [details] [review]
Pull request

Nice, I think this looks better. I'd throw on my fluffy hat and encourage you to write a test for this, but I guess you get a free pass since 3D touch doesn't work in the simulator :D

Comment 10

[Brian Nicholson (:bnicholson)]

Comment on attachment 8698499 [details] [review]
Pull request

Err, no idea why my review changed the attachment name to another bug. Sorry for the churn.

Comment 11

[Emily Toop (:fluffyemily)]

:tecgirl - have retrieved missing reading list option!

Comment 12

[Robin Andersen [:tecgirl]]

:fluffyemily — I'm still not seeing Add to Reading List as an option :( http://c.tecgirl.com/e7AZ

Comment 13

[Emily Toop (:fluffyemily)]

Attachment: IMG_5871.jpg

:tecgirl That is odd. This is what I see when I run the branch. I checked that all the code was on github.

Which site were you looking at? Is it already in the reading list, in which case that option would not appear?

Comment 14

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Comment on attachment 8698499 [details] [review]
Pull request

Looks good but I need a little more time for on-device testing for this one.

I don't know how well the l10n scripts handle table names with spaces in them. Because they translate to file names.

Emily I don't know if you have time to rename the string table, but if this turns out to be an issue after string freeze then we can probably handle it in l10n scripts.

Comment 15

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Attachment: Screen Shot 2015-12-18 at 4.25.30 PM.png

See screenshot. I found a small bug. When you lightly press a tab, we provide a rectangle that should not be blurred out. Looks like it is a bit off because half the tab cell is part of the blurrrr.

Comment 16

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

Hmm I got the app in a state where we crash with "Array index out of range".

STR:

On a fresh install with no history/bookmarks:

1) Force press on the app icon and 'Open New Private Tab'
2) Load anything in the tab
3) Open the tab tray
4) Force press on the one tab in there, and then select Close Tab
5) Leave Private Mode
6) Try to close the tab on screen

At this point the app will crash. Also notice that even though we left Private Mode, the UI still looks like private mode.

Comment 17

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

BTW These 3D Touch features rock. Loving it. :-)

Comment 18

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

The peek for an top sites tab is empty:

STR:

1) Create a new tab - Don't load anything just let it sit on the top sites
2) Open tab tray
3) Peek at the tab

Expected: top sites

Actual: empty preview


Is this something we can do? If not, or if it makes no sense, maybe peek and pop should be disabled for tabs that do not have a page loaded? (about:blank?)

Comment 19

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

I have got my browser in a weird state where it shows a grey (or maybe translucent on top of black - which ends up being grey) background when you open and close a tab. The grey background only shows during the tab animation. Once the tab shrinks to its cell, the background of the tab tray changes back to black.

Comment 20

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

STR For the bug in comment #19:

1) Start the app cleanly
2) Open The Mozilla Project tile (dont think it matters how you load anything)
3) Open the tab tray
4) Peek the one tab there is and select Close Tab
5) Since it was the last tab, a new empty tab opens
6) Open the tab tray:

Expected: the empty tab minimizes into its cell on top of a black background

Actual: the empty tab minimizes into its cell on top of a grey background

Comment 21

[Robin Andersen [:tecgirl]]

(In reply to Emily Toop (:fluffyemily) from comment #13)
> Created attachment 8699939 [details]
> IMG_5871.jpg
> 
> :tecgirl That is odd. This is what I see when I run the branch. I checked
> that all the code was on github.
> 
> Which site were you looking at? Is it already in the reading list, in which
> case that option would not appear?

I did a clean install and all is well.

Comment 22

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

The crash from comment #16 is hard to reproduce. But it has happened a couple of times for me now. Trying to find good steps for this.

Comment 23

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

(In reply to Robin Andersen [:tecgirl] from comment #2)

> Copy URL would show if the user is signed in yet has [0] connected devices
> or they are not signed in.

What is the reasoning behind this? Isn't Copy URL something that is always useful? Regardless of whether you are logged or have connected devices?

Comment 24

[Robin Andersen [:tecgirl]]

(In reply to Stefan Arentz [:st3fan] from comment #23)
> (In reply to Robin Andersen [:tecgirl] from comment #2)
> 
> > Copy URL would show if the user is signed in yet has [0] connected devices
> > or they are not signed in.
> 
> What is the reasoning behind this? Isn't Copy URL something that is always
> useful? Regardless of whether you are logged or have connected devices?

:st3fan, this is for when we've maxed out 4 options. We *could* add another, but then the action menu starts to take over the viewport.

We're currently offering:
Add to Reading List
Add to Bookmarks
Copy URL/Send Tab
Close Tab

Send Tab would take the place of Copy URL with the basic assumption that the user intends on seeing the tab elsewhere, and supports Task Continuity when signed in. I agree it's always useful but we are restricted to the amount of options.

Comment 25

[Emily Toop (:fluffyemily)]

> At this point the app will crash. Also notice that even though we left
> Private Mode, the UI still looks like private mode.

This is being resolved as a separate issue https://bugzilla.mozilla.org/show_bug.cgi?id=1232017

Comment 26

[Emily Toop (:fluffyemily)]

(In reply to Stefan Arentz [:st3fan] from comment #18)
> The peek for an top sites tab is empty:
> 
> STR:
> 
> 1) Create a new tab - Don't load anything just let it sit on the top sites
> 2) Open tab tray
> 3) Peek at the tab
> 
> Expected: top sites
> 
> Actual: empty preview
> 
> 
> Is this something we can do? If not, or if it makes no sense, maybe peek and
> pop should be disabled for tabs that do not have a page loaded?
> (about:blank?)

The code loads the URL in the background and then displays it when it's loaded, but for about:home this results in a blank preview. Have updated to only load the URL if it's not one of our ignored URL's. In this case we will show only the screenshot.

Comment 27

[Emily Toop (:fluffyemily)]

(In reply to Stefan Arentz [:st3fan] from comment #15)
> Created attachment 8700172 [details]
> Screen Shot 2015-12-18 at 4.25.30 PM.png
> 
> See screenshot. I found a small bug. When you lightly press a tab, we
> provide a rectangle that should not be blurred out. Looks like it is a bit
> off because half the tab cell is part of the blurrrr.

We do not provide any of these transitions - this is all the API's. At this point we have provided the ViewController in `previewingContext(previewingContext: UIViewControllerPreviewing, viewControllerForLocation location: CGPoint) -> UIViewController?` and it's before the VC has been fully presented. We have no route into the animation code at this point.

Comment 28

[Emily Toop (:fluffyemily)]

(In reply to Stefan Arentz [:st3fan] from comment #16)
> Hmm I got the app in a state where we crash with "Array index out of range".
> 
> STR:
> 
> On a fresh install with no history/bookmarks:
> 
> 1) Force press on the app icon and 'Open New Private Tab'
> 2) Load anything in the tab
> 3) Open the tab tray
> 4) Force press on the one tab in there, and then select Close Tab
> 5) Leave Private Mode
> 6) Try to close the tab on screen
> 
> At this point the app will crash. Also notice that even though we left
> Private Mode, the UI still looks like private mode.

This was caused by the TabManager hanging on to far too many delegates for TabTrayVC. When it called removeTab, it sometimes called the delegate method on a TabTrayVC for a privacy mode other than the one we were currently looking at and that would cause it to try and remove an index of -1. Solved by ensuring that the TabTrayVC removes itself as a delegate whenever it is hidden or dismissed, and re-adds itself when it is shown again to ensure that only the delegates we care about are called.

Comment 29

[Emily Toop (:fluffyemily)]

(In reply to Stefan Arentz [:st3fan] from comment #20)
> STR For the bug in comment #19:
> 
> 1) Start the app cleanly
> 2) Open The Mozilla Project tile (dont think it matters how you load
> anything)
> 3) Open the tab tray
> 4) Peek the one tab there is and select Close Tab
> 5) Since it was the last tab, a new empty tab opens
> 6) Open the tab tray:
> 
> Expected: the empty tab minimizes into its cell on top of a black background
> 
> Actual: the empty tab minimizes into its cell on top of a grey background

This one was caused by holding a strong reference to the tab in the PeekVC which meant that it didn't properly get deleted when closing the tab from a Peek action. This left a zombie webView on the stack that wasn't being controlled by the browser tray animators, hence the evil gray background. Making the property weak resolved the issue.

Comment 30

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

(In reply to Stefan Arentz [:st3fan] from comment #18)
> The peek for an top sites tab is empty:
> 
> STR:
> 
> 1) Create a new tab - Don't load anything just let it sit on the top sites
> 2) Open tab tray
> 3) Peek at the tab
> 
> Expected: top sites
> 
> Actual: empty preview
> 
> 
> Is this something we can do? If not, or if it makes no sense, maybe peek and
> pop should be disabled for tabs that do not have a page loaded?
> (about:blank?)

This works well now.

Comment 31

[Stefan Arentz | :st3fan | ⏰ EST | he/him]

(In reply to Emily Toop (:fluffyemily) from comment #29)

...

> This one was caused by holding a strong reference to the tab in the PeekVC
> which meant that it didn't properly get deleted when closing the tab from a
> Peek action. This left a zombie webView on the stack that wasn't being
> controlled by the browser tray animators, hence the evil gray background.
> Making the property weak resolved the issue.

Looks good!