1226571 - Add 3D Touch Static Quick Actions

Status: RESOLVED FIXED

(Firefox for iOS :: General, defect)

Description

[Emily Toop (:fluffyemily)]

Add 2 static quick actions to Firefox for iOS.

The static quick actions are:

New Tab - opens to a new normal tab in Firefox 
New Private Tab - opens to a new private tab in Firefox

Comment 1

[Stephan Leroux [:sleroux]]

FYI there's an open PR from a contributor that adds these - maybe we can work with that?

https://github.com/mozilla/firefox-ios/pull/1104

Comment 2

[Emily Toop (:fluffyemily)]

Ah, thanks. That's certainly something we can base this work off, although :tecgirl's design doesn't call for the show bookmarks/reading list but adds some dynamic items instead.

Comment 3

[Emily Toop (:fluffyemily)]

Attachment: Pull request

Comment 4

[Stephan Leroux [:sleroux]]

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

Code looks good - just some nits. Also left an issue I experienced with testing this in the simulator using the simulator plugin. Not sure if it's an actual bug or not though.

Comment 5

[Robin Andersen [:tecgirl]]

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

I believe I see some commentary on github about this, but when I tap on New Private Tab, the URLbar and tab counter UI is off, http://c.tecgirl.com/dtoJ

In a similar (yet inverted) fashion, this is what I see when tapping on New Tab: http://c.tecgirl.com/dtqO

Additionally, the keyboard should be enabled for New Tab/New Private Tab, ready for the user to enter something. 

I will also add the correct assets for all instances, since I see we're just reusing what we have and they don't match. :)

Comment 6

[Emily Toop (:fluffyemily)]

Attachment: Pull request

Fixed a number of issues. Asking for another review as quite a bit has changed.

1. Fixed crash when opening tab from cold
          This needed to be done asynchronously otherwise BVC didn't actually exist when tab was created causing crash
2. Fixed failure to apply UIAppearanceTheme
          It seems that once you have applied UIAppearance values to a component, any changes made to those UIAppearance values will not be applied on those components until you remove and then re-add them to a view. Therefore I am forcibly applying UIAppearance themes when starting from a QuickAction.
3. Fixed crash when adding new tab with different mode when tab tray open
        Found a crasher whereby if you opened a tab in a different mode to the one currently in effect in the backgrounded app, while the app is in the TabTray then the collection view works out the incorrect number of cells and crashes when it doesn't match. Fixed by applying new theme just like a tap to the PB button when a collectionView already exists in the TabTray, rather than just updating the privateMode.
4. Feedback from @tecgirl and now new tabs open with the URL bar as first responder.

Comment 7

[Emily Toop (:fluffyemily)]

Also, still waiting for correct assets from :tecgirl for QuickActions

Comment 8

[Stephan Leroux [:sleroux]]

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

LGTM - just a nit. I can't seem to test it this morning in the simulator though (this simulator hack isn't working for some reason :( )

As a side note, seems like using the UIAppearance proxy for theming isn't the best choice for what we want. Looks like it's more for setting up a static theme at the beginning of the app whereas we're using it to dynamically update styles. I'll file a bug to investigate a more robust/dynamic way of theming.

Comment 9

[Stephan Leroux [:sleroux]]

Bug filed for themes: https://bugzilla.mozilla.org/show_bug.cgi?id=1227550

Comment 10

[Robin Andersen [:tecgirl]]

(In reply to Emily Toop (:fluffyemily) from comment #7)
> Also, still waiting for correct assets from :tecgirl for QuickActions

I added these to the meta so as not to have to separate them:
https://bugzilla.mozilla.org/attachment.cgi?id=8691486

Comment 11

[Robin Andersen [:tecgirl]]

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

Great work, Emily! Note I dropped all the assets on the meta so they're in one place.

Comment 12

[Emily Toop (:fluffyemily)]

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

Added assets. Just need a quick +1 from you Robin.

Comment 13

[Stefan Plewako [:stef]]

Both added strings are missing localization notes,
https://github.com/splewako/firefox-ios-l10n-en/commit/6573867f993d2cb6c6a36f01d9f8be8ee01eb725