Repository navigation
Fix external apps opening inside Signal's task - #14989
theabhishekchandra wants to merge 2 commits into
Conversation
|
Before settling on A link another app handles — the reported case
A link Signal handles itself —
Both This also confirms by measurement what the description argues from the manifest — a |
Fixes signalapp#14946 Tapping a link that a non-browser app handles (e.g. x.com with the X app installed) launches that app's activity into Signal's own task. Recents then shows a single card carrying Signal's icon and label whose contents are the other app, and the other app sits on top of Signal's back stack rather than beside it. Three separate paths hand a URL to another app, and none set FLAG_ACTIVITY_NEW_TASK: - LinkActions.openUrl, reached from CommunicationActions.openBrowserLink (~71 call sites). It set the flag only when the context was not an Activity. That condition comes from 26cb17e "Fix browser not opening in certain contexts", which added it because Android *requires* the flag from a non-Activity context. It was never a decision about task placement; on the Activity path the flag was simply absent. - ConversationFragment.openLink, used by onUrlClicked and onLinkPreviewClicked, which builds its own intent and never goes through LinkActions. The Browser.EXTRA_CREATE_NEW_TAB / EXTRA_APPLICATION_ID extras it sets only influence browsers, so they do nothing for a link claimed by a non-browser app. - LongClickCopySpan, which did not override onClick and so inherited URLSpan's bare startActivity. This is the click target wherever the UrlClickHandler declines: the story viewer (which uses the span directly), the long-message view, message details and starred messages. Set FLAG_ACTIVITY_NEW_TASK on all three so the handling app gets its own task, as it would when the same link is opened anywhere else on the device. LongClickCopySpan now routes through openBrowserLink rather than re-implementing the launch, so the number of parallel link launchers goes down rather than up. Renamed encourageNewBrowserTab() to prepareExternalLink(), since it no longer only concerns browser tabs. Note this is only observable when a non-browser app handles the link. Chrome's ChromeTabbedActivity is singleTask and always takes its own task regardless of the flag, so browser links are unaffected either way. Signal's own https handlers, StickerPackPreviewActivity and DeepLinkEntryActivity, both use the default taskAffinity, so a signal.me or signal.art link still resolves into the existing Signal task rather than creating a second one. Added LinkActionsTest and LongClickCopySpanTest; the flag assertions fail against current main.
6175509 to
dbf3b02
Compare
|
Have you looked at whether group links work appropriately? Ideally those could be just launched in the same process (since ultimately they open a group chat that you can choose to join) |
|
Yes, group links still work. This PR does not change how they are handled. I checked the code and then tested on a real device (Vivo V2036, Android 13, running this PR). Group links in a chat never leave Signal. When you tap a group link in a conversation or in the long-message view, Other places already send the link out and back. Link previews, story captions, group descriptions, and the What I measured. Same
Row 1: no activity was created at all. The join sheet was shown by the fragment itself. Row 2, run three times with the same result:
The test link points to a group that does not exist, so both sheets say "Link error". That is the normal join sheet doing its server check, same as it would for a real link. (Small note about my device: it has both the release and staging builds installed, and both are verified for If you want link previews and story captions to also handle group links inside Signal, like the chat does, I can add that here or in a follow-up. |
|
Have you tested back-gesture support after entering different applications? |
|
Yes, I tested it. Phone: Vivo V2036, Android 13, gesture navigation turned on, running this PR.
In every case Signal sent the Two things to know. Both are normal Android behaviour, not something this PR adds:
I do not have an Android 14 or newer device, so I could not watch the predictive back animation. I could only check where the gesture ends up. |
The span is only installed directly, with no UrlClickHandler, by the story viewer's captions and the submit-debug-log dialog. Message details and starred messages never reach LongClickCopySpan.onClick. Claude-Session: https://claude.ai/code/session_01DR838Sy4oDGKGyVEFxL8Xa
|
Hi! Thanks for your submission. A condensed version is in 8.27 and I attributed you as the author. In the future, it'd be great to include comments that are written in your own words summarizing your work, as Claude can be very chatty. |
|
Thank you, Alex. I am happy to contribute and work with you. I understood your feedback on my comments. Mostly, I write in my own words and just ask an AI agent to explain them in simple words so the maintainer can understand and verify easily. it was a bit too chatty because I thought I needed to explain everything. I understand now. Next time, I will write it in my own words. |
2 participants
First time contributor checklist
Contributor checklist
signal.orgwithlaunchMode="standard"and its owntaskAffinityto stand in for X, and captured before/after against real builds ofmainwith and without this patch.Fixes #1234syntaxDescription
Tapping a link handled by a non-browser app launches that app into Signal's task. Recents shows one card with Signal's icon and label whose contents are the other app, and the other app lands on top of Signal's back stack instead of beside it.
Same device, same tap on a link, only this patch differing. On the left the single Recents card is labelled Signal (Staging) but its contents are the other app; on the right the other app has its own card and Signal keeps its own. The red screen is the stub standing in for X.
Three paths hand a URL to another app, and none set
FLAG_ACTIVITY_NEW_TASK:LinkActions.openUrl— viaCommunicationActions.openBrowserLink(~71 call sites). It set the flag only when the context was not an Activity. That guard comes from 26cb17e ("Fix browser not opening in certain contexts"), added because Android requires the flag from a non-Activity context. It was never a decision about task placement — on the Activity path the flag was just missing.ConversationFragment.openLink— used byonUrlClickedandonLinkPreviewClicked. Builds its own intent and never goes throughLinkActions. TheBrowser.EXTRA_CREATE_NEW_TAB/EXTRA_APPLICATION_IDextras only influence browsers, so they do nothing for a link a non-browser app has registered to handle.LongClickCopySpan— never overrodeonClick, so it inheritedURLSpan's barestartActivity. This is the click target where the span is installed directly with noUrlClickHandler: the story viewer's captions and the submit-debug-log dialog.LongClickCopySpannow delegates toopenBrowserLinkinstead of re-implementing the launch, so this reduces the number of parallel link launchers rather than adding another.encourageNewBrowserTab()is renamedprepareExternalLink(), since it no longer only concerns browser tabs.Scope. Only observable when a non-browser app handles the link. Chrome's
ChromeTabbedActivityissingleTaskand always takes its own task regardless of the flag, so browser links behave identically before and after.Why
NEW_TASKis safe hereStickerPackPreviewActivity,DeepLinkEntryActivity) both use the defaulttaskAffinity, so asignal.me/signal.artlink reuses the existing Signal task rather than creating a second Signal card.Verification
Unit tests in
LinkActionsTestandLongClickCopySpanTest; the flag assertions fail against currentmainand pass with this change (confirmed by stashing each fix independently).On device (Vivo V2036, Android 13), same build and same tap, only the patch differing:
mainsz=2)0x00x10000000On
mainthe single Recents card is labelled "Signal" but its contents are the other app, and Back from it lands on Signal's own screen. With the fix there are two cards.