Repository navigation
Conversation
…of their own tab Every activity shared one browsing mode: AppState.mode and the last known mode hold the mode of the activity created or restarted last. A custom tab shown next to the regular tabs activity in split screen could then get that activity's privacy, or give it its own. These now use the privacy of the activity they are shown in: - the Compose theme (toolbars, menus, dialogs, bottom sheets), read from a new isPrivateTheme attribute of the activity's theme; - the private tabs lock, and the unlock screen, which recognizes a custom tab from its navigation origin; - hiding the contextual menu while private tabs are locked; - the "Remember decision for this site" checkbox of site permission prompts, and hiding an open prompt while private tabs are locked; - the black screen shown behind Android permission dialogs. ActivityPrivacy tells whether an activity is private: the privacy of its custom tab, or for the regular tabs activity, for now, the last known mode. Custom tabs keep writing the last known mode until Part 4, so in split screen the regular tabs activity's checks other than its theme follow the custom tab opened or restarted last. Those checks also read normal right after "Open in private tab" on a top site, until the browser screen switches to private; Part 7 fixes this. Testing, with "Open links in a private tab" on to get private custom tabs, and a private custom tab next to the regular tabs activity in normal mode in split screen: - The custom tab's menu uses the private theme and the regular tabs activity keeps the normal theme. Switching the regular tabs activity to private and back doesn't change the custom tab's menu. - On a page asking for location, the custom tab's prompt has no "Remember decision for this site" checkbox. With "Open links in a private tab" off, a normal custom tab next to a private regular tabs activity shows it. - With "Use screen lock to hide tabs in private browsing" on and Firefox's Android camera permission revoked, allowing a camera request in the custom tab shows the Android dialog over a black screen. - With "Use screen lock to hide tabs in private browsing" on and a private regular tab open, the custom tab opens on the unlock screen, without "Leave private tabs". Unlock, long press a link, send Firefox to the background and return: the unlock screen shows again and the contextual menu is hidden. - With no custom tab open, in regular tabs, all of the above follow the current mode, as before.
… tab "View certificate" from a custom tab's site security panel added a tab to the regular tabs list, selected it and showed it in the custom tab activity. That tab got the browsing mode of the activity created or restarted last, so a private custom tab's certificate could be opened in a normal tab and then allow users to interact with the full browser, outside of the custom tab UX. The certificate now loads in the custom tab itself. No tab is added and browsing mode does not change anymore. A back button/gesture will return the user to the previous webpage. To test, in split screen: - With "Open links in a private tab" on, open a link from another app and show the private custom tab next to the regular tabs activity in normal mode. "View certificate" from the custom tab's site security panel closes the panel and shows the certificate in the custom tab. The regular tabs activity stays normal and gets no new tab. Back shows the page again. - With "Open links in a private tab" off, do the same from a normal custom tab next to the regular tabs activity in private mode: it stays private. - "View certificate" from a regular tab opens the certificate in a new tab, as before.
…b in that custom tab Screens a custom tab activity can show opened their links in a new tab of the regular tabs list: "Learn more" in the tracking protection settings and exceptions (from the site security panel), opening a saved login's site (from the login prompt), an add-on's report, settings, details, permissions and warning links (after its options page), "Learn About Sending Tabs…" in "Connect another device" (from Share) and "Learn more" in "Download languages" (from the translations dialog). The custom tab activity then showed the regular browser screen with that tab, which got the browsing mode of the activity created or restarted last. These links now load in the custom tab and go back to it, like "View certificate". No tab is added and no activity changes its browsing mode. The screens get the custom tab's ID as an optional customTabSessionId argument, passed on each navigation from the custom tab. For regular tabs this ID is null and they will be opened in a new tab, as before. To test, in split screen, with "Open links in a private tab" on and a private custom tab next to the regular tabs activity in normal mode. Each of these loads the page in the custom tab, adds no tab and leaves the regular tabs activity normal, and Back shows the previous page: - Site security panel > "Privacy Settings" > "Learn more", and the same in Exceptions. - With a saved login for the site, focus a login field, tap "Manage passwords" > "Saved passwords", authenticate if asked, open the login and tap "Open site in browser" next to its site. - With an extension whose background script calls browser.runtime.openOptionsPage() when its content script on the page asks it to: open its options page from the custom tab, press Back, then tap "Report", "Settings" (for an options page that opens in a tab), a link in its details or permissions, or a warning's "Learn more". Until Part 6 the options page can also open in the regular tabs activity. - Signed in to a Mozilla account with no other devices, and "Enable Native Android Share Sheet" off in Secret Settings: menu > Share > "Connect another device" > "Learn About Sending Tabs…". - On a page in another language: menu > "Translate page" > options > "Translation settings" > "Download languages" > "Learn more". The same screens reached from Settings, Share or a regular tab open the link in a new tab, as before.
…om tabs Custom tab activities resolved their browsing mode like the regular tabs activity, from the PRIVATE_BROWSING_MODE intent extra, else from the last known mode, then saved it as the last known mode and published it as AppState.mode. This flow lead to various bugs in particular edgecase scenarios: - The Mozilla account sign-in tab is started without that extra, with a private session when the last known mode is private. Once another activity saved normal, recreating the sign-in tab gave it the normal theme and menu. - Opening a normal custom tab while the regular tabs activity was private saved normal, so the next launch from the launcher started in normal mode. - A normal custom tab counted as leaving private mode and locked private tabs on its own, and every custom tab overwrote the regular tabs activity's AppState.mode. Custom tab activities now resolve their mode from their custom tab first, then the intent extra, then the last known mode, and a change of their mode only updates their secure window flag. Only the regular tabs activity saves its mode, updates its theme manager and publishes it, so the checks that read the last known mode since Part 1 follow the regular tabs activity in split screen too. To test, with "Open links in a private tab" off unless said otherwise: - With Firefox following the device theme, switch the regular tabs activity to private and start a Mozilla account sign-in from Settings. From another app, open a link in a normal custom tab and close it. Return to Firefox's sign-in tab through recents and toggle the system dark theme: the sign-in tab keeps the private theme and menu. - Switch the regular tabs activity to private and send Firefox to the background. Open a link from another app in a normal custom tab and close it. Without returning to Firefox, force stop it and open it from the launcher: it starts in private mode. - With "Use screen lock to hide tabs in private browsing" on, open private tabs, put Firefox next to another app in split screen and unlock the private tabs. Open a link from the other app in a normal custom tab: the private tabs stay unlocked. Sending Firefox to the background still locks them. - With "Open links in a private tab" on, show a private custom tab next to the regular tabs activity in normal mode in split screen: the regular tabs activity stays normal, and on a page asking for location its prompt shows "Remember decision for this site". The custom tab keeps its private theme and menu. - Switching the regular tabs activity between normal and private, then force stopping Firefox and opening it from the launcher, starts in the last mode, as before.
… activity Every started activity opened a pending extension popup request. Even if extension actions/popups can only be started/shown from the regular tabs activity the custom tabs one also opened the popup. This won't happen anymore - custom tab activities no longer open extension popups. To test: - With an extension that has a popup, open a custom tab from another app next to the regular tabs activity in split screen and tap the extension's action in the regular tabs activity's menu: the popup shows only in the regular tabs activity. - Without a custom tab open, the popup shows as before.
…the new privacy or were applied late Some changes of the regular tabs activity's browsing mode had no tab of the new privacy, or applied the mode late: - Leaving the private tabs unlock screen switched to normal without a normal tab with homepage as a new tab on, and the enable_private_browsing deep link switched to private without a private tab. - "Open in private tab" on a top site only updated AppState.mode. The last known mode, the secure window flag and the theme followed only once the browser screen showed the new private tab. - "Open in Firefox" from a private custom tab, or a search from a custom tab, started the regular tabs activity with the "Open links in a private tab" setting's privacy, then switched it once the browser showed the tab. - Opening history or bookmark items in new tabs switched the mode even when no item had a URL, and the history screens switched to private a second time. - Opening a recently closed tab re-dispatched the current mode, which changed nothing. With homepage as a new tab on, leaving the unlock screen now selects the most recently used normal tab, or adds a normal homepage tab, and the deep link adds a private homepage tab. Top sites switch through the activity's browsing mode manager. "Open in Firefox" and custom tab searches pass the privacy of the tab they show, which IntentReceiverActivity keeps for intents without a link to open. Library items switch the mode only when they open a tab, once. The recently closed re-dispatch is removed. With homepage as a new tab off, leaving the unlock screen still selects no tab. To test, with homepage as a new tab on unless said otherwise: - With "Use screen lock to hide tabs in private browsing" on and normal tabs open, lock the private tabs and tap "Leave private tabs": the most recently used normal tab is selected and the tabs tray shows the normal tabs. With no normal tab, a normal homepage tab shows. - From normal mode, open fenix-dev://enable_private_browsing (fenix-nightly:// in Nightly): a private homepage tab shows. - With homepage as a new tab off, the same two steps behave as before. - In normal mode, long press a top site and tap "Open in private tab", from the homepage and from the shortcuts screen: the private theme applies at once and the site loads in one private tab. Force stop Firefox and open it from the launcher: it starts in private mode. - With "Open links in a private tab" on, open a link from another app in a private custom tab, then turn the setting off. With Firefox's regular tabs activity closed, tap "Open in Firefox": it opens private, without a normal frame. Do the same from a normal custom tab opened with the setting off, then turned on: it opens normal. - In a normal custom tab, select text on the page and tap "Private search" in the selection menu: Firefox opens in private mode on the search, without a normal frame. "Search" opens it in normal mode. - In normal mode, select history items and tap "Open in private tab": the private tabs tray shows them, and private mode applies once. - Recently closed tabs reopen as before.
Code read the browsing mode from AppState.mode or from an activity's BrowsingModeManager, often through a cast to HomeActivity. Since Part 4, AppState.mode and the regular tabs activity's manager hold the regular tabs activity's mode, which is also the last known mode that ActivityPrivacy.isPrivate(null) returns. A custom tab activity's manager holds its custom tab's privacy, which ActivityPrivacy.isPrivate(customTabSessionId) returns. These reads now go through ActivityPrivacy which supports informing about different browsing modes for regular or custom tabs. Classes that only read the mode take an isPrivate lambda instead of a BrowsingModeManager. Classes that also switch the mode (the toolbar middlewares, the tabs tray controller, the private browsing button, top sites, the intent processors), the private tabs lock and the homepage's filter of mode changes keep using the mode until Part 8. No intended behaviour change, except that the reader view controls of a custom tab now use the custom tab's privacy for their style. FenixBrowserUseCases keeps a default for the privacy of new tabs, the regular tabs activity's. To test, in both browsing modes and with homepage as a new tab on and off: - Homepage, toolbars and tabs strip, search and suggestions, tabs tray, history, bookmarks, recently closed tabs, extensions, Lens, report broken site and the site security panel behave as before. - With "Open links in a private tab" on, open a link from another app that uses custom tabs and show the private custom tab next to the regular tabs activity in normal mode in split screen: the custom tab's menu stays private and the regular tabs activity's screens stay normal.
…its selected tab or the last saved mode The regular tabs activity's browsing mode was held in 3 separate places: in its BrowsingModeManager, in AppState.mode and in the persited last known mode value, and mode switches had to keep them in sync. When Android restored the activity after process death, its original launch intent's mode was applied again, so an activity launched by the "New private tab" shortcut went back to private when restored from recents after switching to normal. The last known mode is now the only place where we'll write the browsing mode, and the regular tabs activity applies its changes to its secure flag, its theme and the private tabs lock. Mode switches save it where they used to set the BrowsingModeManager, so the theme switches before the new tab shows, and the browser screen saves the privacy of a tab selected while it shows. When the regular tabs activity is created, it uses its launch intent's mode only on a fresh launch, else ActivityPrivacy.isPrivate(null), which now returns the selected tab's privacy first with homepage as a new tab on. With homepage as a new tab on and private modes saved, cold starts restore the tabs without selecting one, so that no normal tab shows before the private homepage tab. BrowsingModeManager, AppState.mode and AppAction.BrowsingModeManagerModeChanged are removed in favor of HomeActivity keeping its mode in in HomeActivity.browsingMode. To test, with homepage as a new tab on unless said otherwise: - While a normal web page shows, select a private tab from an extension: the regular tabs activity switches to private once. - From the tabs tray, select a tab of the other privacy: it shows with its own theme, without the previous theme first. - Close the last private tab from the tabs tray: the activity isn't recreated while the tabs tray shows. - With "Use screen lock to hide tabs in private browsing" on and private tabs open, switch to normal mode: the private tabs are locked. - In private mode, force stop Firefox and open it from the launcher: it starts on a private homepage tab, without a normal frame. Same with "Open on" set to the last tab, and when a custom tab started the process. - Open Firefox from the "New private tab" shortcut, switch to a normal tab, close the private tabs, send Firefox to the background, kill its process with `adb shell am kill <package>`, check with `adb shell pidof <package>` that it is gone, and reopen it from recents: it stays normal. - With homepage as a new tab off, the homepage private mode button, the tabs tray, the shortcuts, links from other apps and cold starts behave as before.
… of the activity's tab After Part 8 removing the BrowsingModeManager we're left with the BrowsingMode enum which can easily be refactored to a isPrivate Boolean. The Normal/Private enum becomes a Boolean, and ActivityPrivacy becomes BrowsingMode, which also reads the privacy a launch intent asks for (the former resolveBrowsingMode). HomeActivity no longer keeps its own copy of the mode. It needs its creation privacy only for its theme and secure flag before super.onCreate. Later reads ask BrowsingMode, and the last known mode listener applies changes without comparing them, since SharedPreferences and the theme manager act only on real changes. Voice search and Lens results from the widgets and the fenix://open deep link now take their privacy from their intent, which follows "Open links in a private tab" for the widgets. With homepage as a new tab on, a fresh launch can still have a normal tab selected when they open, so reading the activity's privacy would open them in a normal tab. To test: - With "Open links in a private tab" and homepage as a new tab on, a normal tab selected and Firefox not running, search by voice from the search widget: the results open in a private tab. Same for a Lens result. - With "Use screen lock to hide tabs in private browsing" on, "Allow screenshots in private browsing" off and homepage as a new tab both on and off: switch between normal and private mode from the homepage, the tabs tray and the "New private tab" shortcut; the theme and screenshot blocking follow, and private tabs lock after switching to normal. - Open a private custom tab next to the normal regular tabs activity in split screen: each keeps its theme.
|
No new issues detected. This pull request is 🆗 |
MatthewTighe
left a comment
MatthewTighe
left a comment
Contributor
There was a problem hiding this comment.
just a few comments for now while I try to assimilate all this 😅
Comment on lines
49
to
57
Contributor
There was a problem hiding this comment.
Why is this so different than the AppStore patch? Not immediately obvious why we don't need to pass along private mode and instead handle custom tabs as a separate case
Contributor
Author
There was a problem hiding this comment.
Why is this so different than the AppStore patch?
In "the AppStore patch" we use two stores - AppStore + BrowserStore to get the information about the "last selected custom tab" (AppState.lastForegroundCustomTabId) and "last selected regular tab" (BrowserState.selectedTabId).
In this patch we need the same information but get it in another way:
- the custom tab (id) is available as a navigation argument in all screens needing it
- the regular tab is read from the
BrowserState.
Not immediately obvious why we don't need to pass along private mode and instead handle custom tabs as a separate case
Two reasons:
- The
findNavController().openToBrowser()call aboveloadUrlOrSearch. If in a custom tab we should not open the browser screen but load the new url in that same custom tab. This fixes an existent bug. - loadUrlOrSearch(..) already check internally for whether the "selectedTab" (regular) is private or not. We could check the private mode of the custom tab and send that as an argument but because of
reason 1that would still be buggy. Can do it if you'd prefer and open followup tickets to handle this kind of bugs separately.
| ) { | ||
| requireComponents.useCases.sessionUseCases.loadUrl(url, customTabSessionId, flags) | ||
| findNavController().popBackStack(R.id.externalAppBrowserFragment, false) | ||
| } |
Contributor
There was a problem hiding this comment.
Generally speaking, I think we could find a better handle to attach critical behavior like this to. For example, perhaps all Fragments that can do it should implement a TabLoader interface. Using a fragment extension function to extract a usecase and a navController feels like discoverability and intentionality are both lower than they could be
Some clarity to the name could help it become clear that this is doing the following:
- loads over the current custom tab
- pops navigation
Contributor
Author
There was a problem hiding this comment.
Generally speaking, I think we could find a better handle to attach critical behavior like this to. For example, perhaps all Fragments that can do it should implement a TabLoader interface. Using a fragment extension function to extract a usecase and a navController feels like discoverability and intentionality are both lower than they could be
IMO a TabLoader should then also handle internally fenixBrowserUseCases.loadUrlOrSearch which sounds pretty good to me. Though I think we'd generally prefer composition vs inheritance.
Can also just move this new method to FenixBrowserUseCases or another usecase.
Just say the word.
Some clarity to the name could help it become clear that this is doing the following:
- loads over the current custom tab
- pops navigation
This is all in the method documentation. Having "popping" in the name I think would cause more confusion - we do that popping just to ensure that pressing back from the newly loaded url bring the user back to before that new url being opened. I think this behaviour is expected - a requirement really, and does not need to be in the name, more so for how the method achieves this behaviour.
But will take in any naming suggestion.
Comment on lines
+35
to
+39
| fun isPrivate(customTabSessionId: String?, intent: Intent? = null): Boolean = | ||
| customTabSessionId?.let { browserStore.state.findTabOrCustomTab(it) }?.content?.private | ||
| ?: intent?.requestedPrivacy() | ||
| ?: browserStore.state.selectedTab?.content?.private?.takeIf { settings.enableHomepageAsNewTab } | ||
| ?: settings.lastKnownModeIsPrivate |
Contributor
There was a problem hiding this comment.
We are baking the answers to questions into this kind of implementation, like:
- How do we handle private intents?
- What do we do in the HNT case?
- What happens if we re-open the app when the last known mode was private?
Contributor
Author
There was a problem hiding this comment.
We are baking the answers to questions into this kind of implementation, like:
* How do we handle private intents? * What do we do in the HNT case? * What happens if we re-open the app when the last known mode was private?
Not sure if this was a question or a statement.
All those scenarios are handled in this method - it ensure a single clean API to use everywhere, and the method documentation specifically says this.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This is an alternative to reading the browser mode from
AppState- #409The change that this PR brings:
Each Fenix activity takes its private or normal mode from the tab it shows, instead of sharing one app-wide mode.
A custom tab and the regular tabs activity can sit side by side in split screen, each keeping its own theme, lock, prompts and new-tab privacy, without changing the other.
A longer description, edgecases covered and accepted trade-offs are available here.
Lando: link
🚫 This pull request has 2 blockers.