Add 3D Touch Static Quick Actions

RESOLVED FIXED in 2.0

Status

()

Firefox for iOS
General
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: fluffyemily, Assigned: fluffyemily)

Tracking

(Depends on: 1 bug)

unspecified
Other
iOS
Dependency tree / graph

Firefox Tracking Flags

(fxios2.0+)

Details

(Whiteboard: [ios9][needstrings])

Attachments

(1 attachment, 1 obsolete attachment)

48 bytes, text/x-github-pull-request
sleroux
: review+
tecgirl
: ui-review+
tecgirl
: feedback+
Details | Review | Splinter Review
(Assignee)

Description

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

Comment 2

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

Comment 3

2 years ago
Created attachment 8690795 [details] [review]
Pull request
Attachment #8690795 - Flags: ui-review?(randersen)
Attachment #8690795 - Flags: review?(sleroux)
Attachment #8690795 - Flags: review?(bnicholson)
(Assignee)

Updated

2 years ago
Blocks: 1227103
(Assignee)

Updated

2 years ago
Blocks: 1227106
(Assignee)

Updated

2 years ago
Blocks: 1227108
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.
Attachment #8690795 - Flags: review?(sleroux) → review+
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. :)
Attachment #8690795 - Flags: ui-review?(randersen) → ui-review-
(Assignee)

Comment 6

2 years ago
Created attachment 8691364 [details] [review]
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.
Attachment #8690795 - Attachment is obsolete: true
Attachment #8690795 - Flags: review?(bnicholson)
Attachment #8691364 - Flags: ui-review?(randersen)
Attachment #8691364 - Flags: review?(sleroux)
(Assignee)

Comment 7

2 years ago
Also, still waiting for correct assets from :tecgirl for QuickActions
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.
Attachment #8691364 - Flags: review?(sleroux) → review+
(Assignee)

Updated

2 years ago
Attachment #8691364 - Flags: ui-review?(randersen) → feedback?(randersen)
(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 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.
Attachment #8691364 - Flags: feedback?(randersen) → feedback+
(Assignee)

Comment 12

2 years ago
Comment on attachment 8691364 [details] [review]
Pull request

Added assets. Just need a quick +1 from you Robin.
Attachment #8691364 - Flags: ui-review?(randersen)
Attachment #8691364 - Flags: ui-review?(randersen) → ui-review+
(Assignee)

Updated

2 years ago
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
Resolution: --- → FIXED
Both added strings are missing localization notes,
https://github.com/splewako/firefox-ios-l10n-en/commit/6573867f993d2cb6c6a36f01d9f8be8ee01eb725
Blocks: 1153333
Target Milestone: --- → 2.0

Updated

2 years ago
Depends on: 1232017
Whiteboard: [ios9][needstrings]
Depends on: 1234266
No longer depends on: 1234266
You need to log in before you can comment on or make changes to this bug.