PWA opens _blank links in PWA whern an external app (e.g. YouTube) is more suitable. |
|||||||||||
Issue descriptionReported in crbug.com/709889 by nekr.fabula@gmail.com: I tested this a bit further and it turns out that with this features added Chrome stopped opening external links other apps. For example twitter/youtube/google maps links previously would be opened in its native app if the one is installed, but now everything is being opened in CCT. This is clearly a regression to me. (now sure how other browsers behave here, e.g. Safari, but iOS home screen webapps are so poor and bad so I'm sure if that is even a good example) Also one more note (might be useful to know too): Previously, only when one specify rel="noopener" on a link, it was opening the link Chrome. Example: <a href="https://youtube.com/channel/..." target="_blank">Youtube</a> -- this would open native app. <a href="https://youtube.com/channel/..." target="_blank" rel="noopener">Youtube</a> -- this would NOT open a native app, but rather open the link in Chrome. With CCT feature it always opens in CCT. I can file a separate bug for rel="noopener" if that makes sense. That might not be a bug though, I'm not sure, but I haven't found any specs/docs about such specific behavior.
,
Jul 10 2017
Piotr, thank you for filing the bug! Having a separate bug to track this work makes what is being fixed clearer (and will likely make my future self happier)
,
Jul 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/727468b41b2fe48bf73983ab0c3aa2632b47d89e commit 727468b41b2fe48bf73983ab0c3aa2632b47d89e Author: piotrs <piotrs@chromium.org> Date: Wed Jul 12 04:06:43 2017 Fixes the redirect to external apps when navigating from PWA. This is done with avoiding explicit chrome package name (using component name instead) and using EXTRA_SEND_TO_EXTERNAL_DEFAULT_HANDLER on the intent. With this patch the redirect works, however it takes a long time for CCT to consult ExternalNavigationHandler. This results in suboptimal UX. Same redirect in ChromeTabbedActivity is almost instantaneous. Result is that for _blank links CCT shows up briefly before redirecting to an external app. In a follow up it should be investigated why it is the case and if it cannot be improved, we might need to bypass CCT in such case, which would diverge from existing navigation paths for _blank links in Clank. BUG= 740402 Review-Url: https://codereview.chromium.org/2969143002 Cr-Commit-Position: refs/heads/master@{#485831} [modify] https://crrev.com/727468b41b2fe48bf73983ab0c3aa2632b47d89e/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java [modify] https://crrev.com/727468b41b2fe48bf73983ab0c3aa2632b47d89e/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java [modify] https://crrev.com/727468b41b2fe48bf73983ab0c3aa2632b47d89e/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappTabDelegate.java [modify] https://crrev.com/727468b41b2fe48bf73983ab0c3aa2632b47d89e/chrome/android/java_sources.gni [delete] https://crrev.com/28132e66abebde370e168fff9ff2fa157c979d55/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/TopActivityListener.java [modify] https://crrev.com/727468b41b2fe48bf73983ab0c3aa2632b47d89e/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebApkIntegrationTest.java [modify] https://crrev.com/727468b41b2fe48bf73983ab0c3aa2632b47d89e/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappNavigationTest.java
,
Jul 12 2017
This should work now.
,
Jul 12 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/a1b6731c183e2273c3f3b013bf6db0bfe66fdb2f commit a1b6731c183e2273c3f3b013bf6db0bfe66fdb2f Author: aberent <aberent@chromium.org> Date: Wed Jul 12 10:03:45 2017 Revert of Fixes redirects to external apps when navigating from PWA. (patchset #9 id:280001 of https://codereview.chromium.org/2969143002/ ) Reason for revert: Test failures from new test WebappNavigationTest#testNewTabLinkToExternalApp on hromium.android/Marshmallow 64 bit Tester. BUG=741568 Original issue's description: > Fixes the redirect to external apps when navigating from PWA. > > This is done with avoiding explicit chrome package name (using > component name instead) and using > EXTRA_SEND_TO_EXTERNAL_DEFAULT_HANDLER on the intent. > > With this patch the redirect works, however it takes a long time for CCT to > consult ExternalNavigationHandler. This results in suboptimal UX. Same redirect > in ChromeTabbedActivity is almost instantaneous. Result is that > for _blank links CCT shows up briefly before redirecting to an external app. > In a follow up it should be investigated why it is the case and if it cannot > be improved, we might need to bypass CCT in such case, which would diverge > from existing navigation paths for _blank links in Clank. > > BUG= 740402 > > Review-Url: https://codereview.chromium.org/2969143002 > Cr-Commit-Position: refs/heads/master@{#485831} > Committed: https://chromium.googlesource.com/chromium/src/+/727468b41b2fe48bf73983ab0c3aa2632b47d89e TBR=dominickn@chromium.org,yusufo@chromium.org,tedchoc@chromium.org,piotrs@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 740402 Review-Url: https://codereview.chromium.org/2975883003 Cr-Commit-Position: refs/heads/master@{#485910} [modify] https://crrev.com/a1b6731c183e2273c3f3b013bf6db0bfe66fdb2f/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java [modify] https://crrev.com/a1b6731c183e2273c3f3b013bf6db0bfe66fdb2f/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java [modify] https://crrev.com/a1b6731c183e2273c3f3b013bf6db0bfe66fdb2f/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappTabDelegate.java [modify] https://crrev.com/a1b6731c183e2273c3f3b013bf6db0bfe66fdb2f/chrome/android/java_sources.gni [add] https://crrev.com/a1b6731c183e2273c3f3b013bf6db0bfe66fdb2f/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/TopActivityListener.java [modify] https://crrev.com/a1b6731c183e2273c3f3b013bf6db0bfe66fdb2f/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebApkIntegrationTest.java [modify] https://crrev.com/a1b6731c183e2273c3f3b013bf6db0bfe66fdb2f/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappNavigationTest.java
,
Jul 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4dbcecfb630ac1a2dba1910f0e429b82bf3a0b5f commit 4dbcecfb630ac1a2dba1910f0e429b82bf3a0b5f Author: piotrs <piotrs@chromium.org> Date: Thu Jul 13 05:16:27 2017 Fixes the redirect to external apps when navigating from PWA. This is done with avoiding explicit chrome package name (using component name instead) and using EXTRA_SEND_TO_EXTERNAL_DEFAULT_HANDLER on the intent. With this patch the redirect works, however it takes a long time for CCT to consult ExternalNavigationHandler. This results in suboptimal UX. Same redirect in ChromeTabbedActivity is almost instantaneous. Result is that for _blank links CCT shows up briefly before redirecting to an external app. In a follow up it should be investigated why it is the case and if it cannot be improved, we might need to bypass CCT in such case, which would diverge from existing navigation paths for _blank links in Clank. BUG= 740402 Review-Url: https://codereview.chromium.org/2969143002 Cr-Original-Commit-Position: refs/heads/master@{#485831} Committed: https://chromium.googlesource.com/chromium/src/+/727468b41b2fe48bf73983ab0c3aa2632b47d89e Review-Url: https://codereview.chromium.org/2969143002 Cr-Commit-Position: refs/heads/master@{#486275} [modify] https://crrev.com/4dbcecfb630ac1a2dba1910f0e429b82bf3a0b5f/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java [modify] https://crrev.com/4dbcecfb630ac1a2dba1910f0e429b82bf3a0b5f/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java [modify] https://crrev.com/4dbcecfb630ac1a2dba1910f0e429b82bf3a0b5f/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappTabDelegate.java [modify] https://crrev.com/4dbcecfb630ac1a2dba1910f0e429b82bf3a0b5f/chrome/android/java_sources.gni [delete] https://crrev.com/184a75bcf8dda55ceef7f6df67154ce822c7cc5d/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/TopActivityListener.java [modify] https://crrev.com/4dbcecfb630ac1a2dba1910f0e429b82bf3a0b5f/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebApkIntegrationTest.java [modify] https://crrev.com/4dbcecfb630ac1a2dba1910f0e429b82bf3a0b5f/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappNavigationTest.java
,
Jul 13 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b2fa8274b92136d9dd2d347b6e33b4e56bfe21f7 commit b2fa8274b92136d9dd2d347b6e33b4e56bfe21f7 Author: aberent <aberent@chromium.org> Date: Thu Jul 13 10:18:13 2017 Revert of Fixes redirects to external apps when navigating from PWA. (patchset #10 id:300001 of https://codereview.chromium.org/2969143002/ ) Reason for revert: Still failing on Marshmallow 64 bit Tester. See, for example https://uberchromegw.corp.google.com/i/chromium.android/builders/Marshmallow%2064%20bit%20Tester/builds/13798 BUG=741568 Original issue's description: > Fixes the redirect to external apps when navigating from PWA. > > This is done with avoiding explicit chrome package name (using > component name instead) and using > EXTRA_SEND_TO_EXTERNAL_DEFAULT_HANDLER on the intent. > > With this patch the redirect works, however it takes a long time for CCT to > consult ExternalNavigationHandler. This results in suboptimal UX. Same redirect > in ChromeTabbedActivity is almost instantaneous. Result is that > for _blank links CCT shows up briefly before redirecting to an external app. > In a follow up it should be investigated why it is the case and if it cannot > be improved, we might need to bypass CCT in such case, which would diverge > from existing navigation paths for _blank links in Clank. > > BUG= 740402 > > Review-Url: https://codereview.chromium.org/2969143002 > Cr-Original-Commit-Position: refs/heads/master@{#485831} > Committed: https://chromium.googlesource.com/chromium/src/+/727468b41b2fe48bf73983ab0c3aa2632b47d89e > Review-Url: https://codereview.chromium.org/2969143002 > Cr-Commit-Position: refs/heads/master@{#486275} > Committed: https://chromium.googlesource.com/chromium/src/+/4dbcecfb630ac1a2dba1910f0e429b82bf3a0b5f TBR=dominickn@chromium.org,yusufo@chromium.org,tedchoc@chromium.org,piotrs@chromium.org # Skipping CQ checks because original CL landed less than 1 days ago. NOPRESUBMIT=true NOTREECHECKS=true NOTRY=true BUG= 740402 Review-Url: https://codereview.chromium.org/2977023002 Cr-Commit-Position: refs/heads/master@{#486333} [modify] https://crrev.com/b2fa8274b92136d9dd2d347b6e33b4e56bfe21f7/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java [modify] https://crrev.com/b2fa8274b92136d9dd2d347b6e33b4e56bfe21f7/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java [modify] https://crrev.com/b2fa8274b92136d9dd2d347b6e33b4e56bfe21f7/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappTabDelegate.java [modify] https://crrev.com/b2fa8274b92136d9dd2d347b6e33b4e56bfe21f7/chrome/android/java_sources.gni [add] https://crrev.com/b2fa8274b92136d9dd2d347b6e33b4e56bfe21f7/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/TopActivityListener.java [modify] https://crrev.com/b2fa8274b92136d9dd2d347b6e33b4e56bfe21f7/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebApkIntegrationTest.java [modify] https://crrev.com/b2fa8274b92136d9dd2d347b6e33b4e56bfe21f7/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappNavigationTest.java
,
Jul 14 2017
Looks like re-landign this will fix issue 742449 too. Would appreciate it if you can get this in for 61
,
Jul 16 2017
Definitely will do everything to get this in and will test WebAPK -> CCT -> WebAPK case explicitly.
,
Jul 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/471df3926c09fda0af15d9157eb29bd326b0f9fc commit 471df3926c09fda0af15d9157eb29bd326b0f9fc Author: piotrs <piotrs@chromium.org> Date: Thu Jul 20 13:41:20 2017 Fixes the redirect to external apps when navigating from PWA. This is done with avoiding explicit chrome package name (using component name instead) and using EXTRA_SEND_TO_EXTERNAL_DEFAULT_HANDLER on the intent. With this patch the redirect works, however it takes a long time for CCT to consult ExternalNavigationHandler. This results in suboptimal UX. Same redirect in ChromeTabbedActivity is almost instantaneous. Result is that for _blank links CCT shows up briefly before redirecting to an external app. In a follow up it should be investigated why it is the case and if it cannot be improved, we might need to bypass CCT in such case, which would diverge from existing navigation paths for _blank links in Clank. BUG= 740402 Review-Url: https://codereview.chromium.org/2969143002 Cr-Original-Original-Commit-Position: refs/heads/master@{#485831} Committed: https://chromium.googlesource.com/chromium/src/+/727468b41b2fe48bf73983ab0c3aa2632b47d89e Review-Url: https://codereview.chromium.org/2969143002 Cr-Original-Commit-Position: refs/heads/master@{#486275} Committed: https://chromium.googlesource.com/chromium/src/+/4dbcecfb630ac1a2dba1910f0e429b82bf3a0b5f Review-Url: https://codereview.chromium.org/2969143002 Cr-Commit-Position: refs/heads/master@{#488228} [modify] https://crrev.com/471df3926c09fda0af15d9157eb29bd326b0f9fc/chrome/android/java/src/org/chromium/chrome/browser/tabmodel/document/TabDelegate.java [modify] https://crrev.com/471df3926c09fda0af15d9157eb29bd326b0f9fc/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappActivity.java [modify] https://crrev.com/471df3926c09fda0af15d9157eb29bd326b0f9fc/chrome/android/java/src/org/chromium/chrome/browser/webapps/WebappTabDelegate.java [modify] https://crrev.com/471df3926c09fda0af15d9157eb29bd326b0f9fc/chrome/android/java_sources.gni [delete] https://crrev.com/9219f3033f0f56ec5477336b11915e7e2e9f46f2/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/TopActivityListener.java [modify] https://crrev.com/471df3926c09fda0af15d9157eb29bd326b0f9fc/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebApkIntegrationTest.java [modify] https://crrev.com/471df3926c09fda0af15d9157eb29bd326b0f9fc/chrome/android/javatests/src/org/chromium/chrome/browser/webapps/WebappNavigationTest.java
,
Jul 20 2017
Hopefully the patch with stick this time and it will be fixed for good.
,
Jul 24 2017
I would like to merge commit 471df3926c09fda0af15d9157eb29bd326b0f9fc into m61, thanks!
,
Jul 24 2017
This bug requires manual review: Reverts referenced in bugdroid comments after merge request. Please contact the milestone owner if you have questions. Owners: amineer@(Android), cmasso@(iOS), ketakid @(ChromeOS), govind@(Desktop) For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Jul 25 2017
,
Jul 25 2017
Reason for Merge request: Usage of CCT in standalone PWAs has only been added in m61, this final patch fixes the regression where we don't favor native specialized handlers (e.g. native YouTube or Google Maps apps). It would be a shame to bring this to users, though arguably it's not very severe issue. Up for your consideration.
,
Jul 31 2017
Hi Alex, it's been almost a week since I proposed the merge. It seems to me I followed the right procedure, but let me know if there's something I've missed.
,
Aug 4 2017
Merge approved for M61 branch 3163. Please merge before end of next week, preferably by Monday so that we can start getting coverage on this as it's a bit larger than I'd like take later in the release cycle.
,
Aug 8 2017
This issue has been approved for a merge. Please merge the fix to any appropriate branches as soon as possible! If all merges have been completed, please remove any remaining Merge-Approved labels from this issue. Thanks for your time! To disable nags, add the Disable-Nags label. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Aug 9 2017
I must have made a mistake. Branch point is #488528, which is after the commit in question. I've also checked Chrome Beta and YT links do get open in a native app. Taking off the merge labels.
,
Aug 17 2017
|
|||||||||||
►
Sign in to add a comment |
|||||||||||
Comment 1 by piotrs@chromium.org
, Jul 9 2017