New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.
Starred by 9 users

Issue metadata

Status: Assigned
Owner:
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 2
Type: Bug

Blocked on:
issue 893011

Blocking:
issue 850626



Sign in to add a comment
link

Issue 853593: desktop-pwas: Keep out of scope navigation in the same window with CCT UI

Reported by ortuno@chromium.org, Jun 18 2018 Project Member

Issue description

Chrome Version: 71
OS: All Desktop (Chrome OS, Windows, Linux, and macOS once we get PWAs there).

Today, when a site navigates out-of-scope, we cancel the navigation and start a new one in a tab. 

Related spec issue: https://github.com/w3c/manifest/issues/646

What steps will reproduce the problem?
(1) Go to https://killer-marmot.appspot.com/web/
(2) Install this app and open the installed app window.
(3) Click "Fork me on GitHub".

What is the expected result?
The PWA app window navigates to GitHub, but because this is out of the app manifest's scope, a pseudo-location bar or "Chrome Custom Tab" (as it's called in Android) UI shows at the top of the window, indicating the origin of GitHub.

Note: If you install a non-PWA (Bookmark App, using Create Shortcut), you actually do get the desired behaviour, but the UI is ugly and needs a rework.

What happens instead?
GitHub opens in a new browser tab.

Bonus problem:

From the same app:
(3) Click "Post!" under "POST-and-redirect".

After 3 seconds, it redirects back automatically.

The expected behaviour is that the out-of-scope site https://redirectonpost.appspot.com appears in the same window in a CCT, then redirects back to Killer Marmot, hiding the CCT UI.

The observed behaviour is that the out-of-scope site https://redirectonpost.appspot.com appears in a new browser tab, and then redirects back to Killer Marmot, resulting in a duplicate Killer Marmot page in a browser tab. This is typical of off-site authentication flows.

Note: The current behaviour is actually mandated by the Manifest spec. This is a problem. We are working on a (breaking) change to the spec, at https://github.com/w3c/manifest/issues/646. In the mean time, Chrome will be intervening here because the broken auth flow is unacceptable, and we've already intervened on Android.

List of problems with the current behaviour:

1. Sites that expected to be navigated away stay open in a weird state e.g. "Loading..."
2. Sometimes the popup blocker gets triggered and stops the new tab from being created. We can't disable the popup blocker because sites could abuse this and keep opening tabs.
3. Login flows get broken since the login flow gets moved to a tab.
4. Implementation-wise, there is no mechanism to redirect the navigation to a tab. Currently we cancel the navigation and try to replicate the original navigation in a tab. In the past, we've accidentally missed things when trying to replicate the navigation which has caused login and checkout flows to break.

Because of these reasons, we should keep navigations in the original context but show some UI to indicate that the site has navigated off scope.
 

Comment 1 by ortuno@chromium.org, Jun 18 2018

Cc: phanindra.mandapaka@chromium.org
 Issue 850095  has been merged into this issue.

Comment 2 by luizp@google.com, Jun 18 2018

Labels: Hotlist-Partner-GSuite

Comment 3 by ortuno@chromium.org, Jul 9 2018

Description: Show this description

Comment 4 by ortuno@chromium.org, Jul 9 2018

Cc: mgiuca@chromium.org ortuno@chromium.org
 Issue 859192  has been merged into this issue.

Comment 5 by ortuno@chromium.org, Aug 16

Blocking: 850626

Comment 6 by mgiuca@chromium.org, Sep 13

Cc: petele@chromium.org austinknight@chromium.org pcovell@chromium.org dominickn@google.com
 Issue 883564  has been merged into this issue.

Comment 7 by mgiuca@chromium.org, Sep 13

Summary: desktop-pwas: Keep out of scope navigation in the same window with CCT UI (was: desktop-pwas: Keep out of scope navigation in the same window)

Comment 8 by mgiuca@chromium.org, Sep 13

Description: Show this description

Comment 9 by mgiuca@chromium.org, Sep 13

Cc: -mgiuca@chromium.org
Labels: -Type-Feature -Pri-3 Pri-2 Type-Bug
Owner: mgiuca@chromium.org
Status: Assigned (was: Available)

Comment 10 by mgiuca@chromium.org, Sep 21

Cc: mgiuca@google.com
Owner: harrisjay@google.com

Comment 11 by mgiuca@chromium.org, Sep 25

Owner: harrisjay@chromium.org

Comment 12 by mgiuca@chromium.org, Oct 8

Blockedon: 893011

Comment 13 by bugdroid1@chromium.org, Oct 9

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/ee7a7bc6174f740cf9c0c58438f637ccc6c62d37

commit ee7a7bc6174f740cf9c0c58438f637ccc6c62d37
Author: Jay Harris <harrisjay@chromium.org>
Date: Tue Oct 09 01:47:49 2018

Adds a flag to open external links inside desktop PWAs

The flag `desktop-pwas-stay-in-window` controls whether
out of scope links open in an external browser. When
disabled (the default) behavior remains the same as
before this change (out of scope links open in the browser).
When enabled, out of scope links will open in the PWA
window and display the location bar (as we do already, in
bookmark apps).

This change has been made because the current flow (kicking
the user to the browser) breaks authentication flows on
some websites.

Bug: 853593
Change-Id: Id38dbd8437651133b2b5be5f05260b342066d0ce
Reviewed-on: https://chromium-review.googlesource.com/c/1242727
Commit-Queue: Jay Harris <harrisjay@chromium.org>
Reviewed-by: Matt Giuca <mgiuca@chromium.org>
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Cr-Commit-Position: refs/heads/master@{#597772}
[modify] https://crrev.com/ee7a7bc6174f740cf9c0c58438f637ccc6c62d37/chrome/browser/about_flags.cc
[modify] https://crrev.com/ee7a7bc6174f740cf9c0c58438f637ccc6c62d37/chrome/browser/chrome_content_browser_client.cc
[modify] https://crrev.com/ee7a7bc6174f740cf9c0c58438f637ccc6c62d37/chrome/browser/flag_descriptions.cc
[modify] https://crrev.com/ee7a7bc6174f740cf9c0c58438f637ccc6c62d37/chrome/browser/flag_descriptions.h
[modify] https://crrev.com/ee7a7bc6174f740cf9c0c58438f637ccc6c62d37/chrome/browser/ui/extensions/hosted_app_browsertest.cc
[modify] https://crrev.com/ee7a7bc6174f740cf9c0c58438f637ccc6c62d37/chrome/common/chrome_features.cc
[modify] https://crrev.com/ee7a7bc6174f740cf9c0c58438f637ccc6c62d37/chrome/common/chrome_features.h
[modify] https://crrev.com/ee7a7bc6174f740cf9c0c58438f637ccc6c62d37/tools/metrics/histograms/enums.xml

Comment 14 by bugdroid1@chromium.org, Oct 11

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a755e8f5209099ee6e60ee1e75cf83966e7f867e

commit a755e8f5209099ee6e60ee1e75cf83966e7f867e
Author: Jay Harris <harrisjay@chromium.org>
Date: Thu Oct 11 00:51:58 2018

Enables feature desktop-pwas-stay-in-window by default

Bug: 853593
Change-Id: I7fba430843c19e36cc75b0b51e4c3c0c5f607cd2
Reviewed-on: https://chromium-review.googlesource.com/c/1272635
Commit-Queue: Jay Harris <harrisjay@chromium.org>
Reviewed-by: Michael Wasserman <msw@chromium.org>
Reviewed-by: Giovanni Ortuño Urquidi <ortuno@chromium.org>
Reviewed-by: Ben Wells <benwells@chromium.org>
Cr-Commit-Position: refs/heads/master@{#598603}
[modify] https://crrev.com/a755e8f5209099ee6e60ee1e75cf83966e7f867e/chrome/browser/extensions/bookmark_app_navigation_throttle_browsertest.cc
[modify] https://crrev.com/a755e8f5209099ee6e60ee1e75cf83966e7f867e/chrome/browser/ui/views/intent_picker_bubble_view_browsertest.cc
[modify] https://crrev.com/a755e8f5209099ee6e60ee1e75cf83966e7f867e/chrome/common/chrome_features.cc

Comment 15 by sergej.s...@gmail.com, Oct 17

i have a video from a navigation flow described by the owner of this topic.

in chrome 69 it worked, for some reasons in the latest version of chrome (70) not anymore.
bug_video.webm
5.9 MB View Download

Comment 16 by sergej.s...@gmail.com, Oct 17

additional info:

it works in "Google Chrome Canary" as expected. the redirection happens in the application window. 

on chrome-beta and on the released version 70 it happens like in the video shown.

Comment 17 by mgiuca@chromium.org, Oct 18

@sergej.sachs: Thanks for reporting this. I think I can explain why it's working on 69 and 71 (Canary) but not 70.

- In 69, we had not yet rolled out PWAs on Windows. So when you use "Create Shortcut" you aren't installing a PWA, and thus the login works inside the app window.
- In 70, you are installing a PWA (not creating a shortcut). So when you use "Install app", you're installing a PWA, and the login takes place in a browser tab. We've found that this is broken in some cases.
- In 71, we've fixed it so that login flows from PWAs take place in the app window, just like for a shortcut.

So in some cases, this will be working properly in 69 and 71, but broken in 70.

Aside from waiting until 71 comes out, a workaround may be: instead of choosing "Install" from the menu, go to "More tools" -> "Create shortcut". That *may* get you back to the old behaviour. I would be interested to hear if that works.

Comment 18 by sergej.s...@gmail.com, Oct 18

@mgiuca thx for answer, good to hear it will be fixed in 71.

The strange thing is, if I use "Create shortcut" the app starts directly in chrome, does not open a application window at all.

But i will wait for the 71 release.

Comment 19 by sergej.s...@gmail.com, Oct 18

@mgiuca is there a possible release date for chrome 71?

Comment 20 by mgiuca@chromium.org, Oct 19

#18: If you go to chrome://apps, right-click the app icon, and tick "Open in window", it should launch in its own window. (Though I can't guarantee it will fix the navigation flow.)

#19: Planning to release Chrome 71 in early December. (Usually there is a 6-week gap between each release.)

Comment 21 by dominickn@chromium.org, Oct 19

Cc: -dominickn@google.com

Comment 22 Deleted

Comment 23 by sergej.s...@gmail.com, Oct 19

#20 thx for the information, i will wait for the next release.

not sure if this problem occurs in chrome mobile.
because chrome mobile is on version 69 but supports already pwa installations right? had no time to test it for now.

Comment 24 by dominickn@chromium.org, Oct 19

Thanks for bearing with us here. :)

Chrome on Android has had the fix here for a while - I believe since v67.

Comment 25 by bugdroid1@chromium.org, Oct 30

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/96a0c5e6d815f70ef9c63b6b05131586e858d5b3

commit 96a0c5e6d815f70ef9c63b6b05131586e858d5b3
Author: Jay Harris <harrisjay@chromium.org>
Date: Tue Oct 30 03:01:15 2018

Makes the LocationIconView more self contained.

Dependencies on the LocationBarView have been abstracted into a
delegate, so as to make the LocationIconView reusable.

This is in preparation for introducing a CustomTabBarView for
Desktop PWAs, which will have its own LocationIconView.

Bug: 853593
Change-Id: I5817041e532cd8db6065f2a818e6b203a91fb012
Reviewed-on: https://chromium-review.googlesource.com/c/1248067
Commit-Queue: Jay Harris <harrisjay@chromium.org>
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Cr-Commit-Position: refs/heads/master@{#603752}
[modify] https://crrev.com/96a0c5e6d815f70ef9c63b6b05131586e858d5b3/chrome/browser/ui/views/location_bar/location_bar_view.cc
[modify] https://crrev.com/96a0c5e6d815f70ef9c63b6b05131586e858d5b3/chrome/browser/ui/views/location_bar/location_bar_view.h
[modify] https://crrev.com/96a0c5e6d815f70ef9c63b6b05131586e858d5b3/chrome/browser/ui/views/location_bar/location_bar_view_browsertest.cc
[modify] https://crrev.com/96a0c5e6d815f70ef9c63b6b05131586e858d5b3/chrome/browser/ui/views/location_bar/location_icon_view.cc
[modify] https://crrev.com/96a0c5e6d815f70ef9c63b6b05131586e858d5b3/chrome/browser/ui/views/location_bar/location_icon_view.h
[modify] https://crrev.com/96a0c5e6d815f70ef9c63b6b05131586e858d5b3/chrome/browser/ui/views/location_bar/selected_keyword_view.cc

Comment 27 by bugdroid1@chromium.org, Nov 9

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/afecf795d7b906dc99bca36cb65b6442ab493713

commit afecf795d7b906dc99bca36cb65b6442ab493713
Author: Jay Harris <harrisjay@chromium.org>
Date: Fri Nov 09 04:41:46 2018

Displays the app title instead of page title when off scope.

Previously, we would always display the current page's title in the
window title. This updates the logic so we display the app name when
off scope (the page title will be displayed in the custom tab bar).


Bug: 853593
Change-Id: Id24d3104386a5334c331599480f0b1ce269ba9e1
Reviewed-on: https://chromium-review.googlesource.com/c/1319214
Reviewed-by: Matt Giuca <mgiuca@chromium.org>
Commit-Queue: Jay Harris <harrisjay@chromium.org>
Cr-Commit-Position: refs/heads/master@{#606736}
[modify] https://crrev.com/afecf795d7b906dc99bca36cb65b6442ab493713/chrome/browser/ui/extensions/hosted_app_browser_controller.cc
[modify] https://crrev.com/afecf795d7b906dc99bca36cb65b6442ab493713/chrome/browser/ui/extensions/hosted_app_browsertest.cc

Comment 28 by bugdroid1@chromium.org, Nov 15

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/1c9a2691f0e3ad2814ee3549abbf9decbec15161

commit 1c9a2691f0e3ad2814ee3549abbf9decbec15161
Author: Jay Harris <harrisjay@chromium.org>
Date: Thu Nov 15 05:43:58 2018

Prepares for addition of CustomTabBar.
The CustomTabBar will be added in a future CL.

* Lifts LocationBar animation into Toolbar, so the same animation
can be reused on the CustomTabBar without duplication.
* Renames ShouldShowLocationBar to ShouldShowToolbar
* Renames UpdateLocationBarVisibility to UpdateToolbarVisibility

Bug: 853593
Change-Id: Ib303275f7d560cb67cc2e30767a7dbf2783da497
Reviewed-on: https://chromium-review.googlesource.com/c/1328081
Commit-Queue: Jay Harris <harrisjay@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Matt Giuca <mgiuca@chromium.org>
Cr-Commit-Position: refs/heads/master@{#608274}
[modify] https://crrev.com/1c9a2691f0e3ad2814ee3549abbf9decbec15161/chrome/browser/ui/browser.cc
[modify] https://crrev.com/1c9a2691f0e3ad2814ee3549abbf9decbec15161/chrome/browser/ui/browser_window.h
[modify] https://crrev.com/1c9a2691f0e3ad2814ee3549abbf9decbec15161/chrome/browser/ui/extensions/hosted_app_browser_controller.cc
[modify] https://crrev.com/1c9a2691f0e3ad2814ee3549abbf9decbec15161/chrome/browser/ui/extensions/hosted_app_browser_controller.h
[modify] https://crrev.com/1c9a2691f0e3ad2814ee3549abbf9decbec15161/chrome/browser/ui/extensions/hosted_app_browsertest.cc
[modify] https://crrev.com/1c9a2691f0e3ad2814ee3549abbf9decbec15161/chrome/browser/ui/location_bar/location_bar.h
[modify] https://crrev.com/1c9a2691f0e3ad2814ee3549abbf9decbec15161/chrome/browser/ui/views/frame/browser_view.cc
[modify] https://crrev.com/1c9a2691f0e3ad2814ee3549abbf9decbec15161/chrome/browser/ui/views/frame/browser_view.h
[modify] https://crrev.com/1c9a2691f0e3ad2814ee3549abbf9decbec15161/chrome/browser/ui/views/location_bar/location_bar_view.cc
[modify] https://crrev.com/1c9a2691f0e3ad2814ee3549abbf9decbec15161/chrome/browser/ui/views/location_bar/location_bar_view.h
[modify] https://crrev.com/1c9a2691f0e3ad2814ee3549abbf9decbec15161/chrome/browser/ui/views/toolbar/toolbar_view.cc
[modify] https://crrev.com/1c9a2691f0e3ad2814ee3549abbf9decbec15161/chrome/browser/ui/views/toolbar/toolbar_view.h
[modify] https://crrev.com/1c9a2691f0e3ad2814ee3549abbf9decbec15161/chrome/test/base/test_browser_window.h

Comment 29 by bugdroid1@chromium.org, Nov 28

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3dc4c27ed6af3e7ea3fbd11522e5cb140ddc3455

commit 3dc4c27ed6af3e7ea3fbd11522e5cb140ddc3455
Author: Jay Harris <harrisjay@chromium.org>
Date: Wed Nov 28 22:18:52 2018

Fixes a bug in the LocationIconView InkDrop

This stops the LocationIconView InkDrop from getting reset if
LocationIconView::Update is called while the InkDrop is animating.

Note: This issue doesn't seem to affect the LocationBarView, only the
CustomTabBar being introduced in CL: 1328084

Bug: 853593
Change-Id: I3ab4045c7a8d0db88a069f1d35d9763c9c8594f1
Reviewed-on: https://chromium-review.googlesource.com/c/1338579
Reviewed-by: Justin Donnelly <jdonnelly@chromium.org>
Commit-Queue: Jay Harris <harrisjay@chromium.org>
Cr-Commit-Position: refs/heads/master@{#611893}
[modify] https://crrev.com/3dc4c27ed6af3e7ea3fbd11522e5cb140ddc3455/chrome/browser/ui/views/location_bar/location_icon_view.cc
[modify] https://crrev.com/3dc4c27ed6af3e7ea3fbd11522e5cb140ddc3455/chrome/browser/ui/views/location_bar/location_icon_view.h
[modify] https://crrev.com/3dc4c27ed6af3e7ea3fbd11522e5cb140ddc3455/chrome/browser/ui/views/location_bar/location_icon_view_unittest.cc

Comment 30 by bugdroid1@chromium.org, Dec 3

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/fb43687746522d4fc167d268725007c0761f57e1

commit fb43687746522d4fc167d268725007c0761f57e1
Author: Jay Harris <harrisjay@chromium.org>
Date: Mon Dec 03 23:40:31 2018

Add a custom tab bar to use for desktop PWAs.

Bug: 853593
Change-Id: I4618ca91c53a18ed7a56cb8dce0416d25318ab41
Reviewed-on: https://chromium-review.googlesource.com/c/1328084
Commit-Queue: Jay Harris <harrisjay@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Matt Giuca <mgiuca@chromium.org>
Cr-Commit-Position: refs/heads/master@{#613322}
[modify] https://crrev.com/fb43687746522d4fc167d268725007c0761f57e1/chrome/browser/ui/BUILD.gn
[modify] https://crrev.com/fb43687746522d4fc167d268725007c0761f57e1/chrome/browser/ui/browser.cc
[modify] https://crrev.com/fb43687746522d4fc167d268725007c0761f57e1/chrome/browser/ui/browser.h
[modify] https://crrev.com/fb43687746522d4fc167d268725007c0761f57e1/chrome/browser/ui/bubble_anchor_util.h
[modify] https://crrev.com/fb43687746522d4fc167d268725007c0761f57e1/chrome/browser/ui/extensions/hosted_app_browsertest.cc
[modify] https://crrev.com/fb43687746522d4fc167d268725007c0761f57e1/chrome/browser/ui/omnibox/omnibox_theme.cc
[modify] https://crrev.com/fb43687746522d4fc167d268725007c0761f57e1/chrome/browser/ui/omnibox/omnibox_theme.h
[modify] https://crrev.com/fb43687746522d4fc167d268725007c0761f57e1/chrome/browser/ui/views/bubble_anchor_util_views.cc
[add] https://crrev.com/fb43687746522d4fc167d268725007c0761f57e1/chrome/browser/ui/views/location_bar/custom_tab_bar_view.cc
[add] https://crrev.com/fb43687746522d4fc167d268725007c0761f57e1/chrome/browser/ui/views/location_bar/custom_tab_bar_view.h
[add] https://crrev.com/fb43687746522d4fc167d268725007c0761f57e1/chrome/browser/ui/views/location_bar/custom_tab_bar_view_browsertest.cc
[modify] https://crrev.com/fb43687746522d4fc167d268725007c0761f57e1/chrome/browser/ui/views/location_bar/location_bar_view.cc
[modify] https://crrev.com/fb43687746522d4fc167d268725007c0761f57e1/chrome/browser/ui/views/tabs/tab.cc
[modify] https://crrev.com/fb43687746522d4fc167d268725007c0761f57e1/chrome/browser/ui/views/toolbar/toolbar_view.cc
[modify] https://crrev.com/fb43687746522d4fc167d268725007c0761f57e1/chrome/browser/ui/views/toolbar/toolbar_view.h
[modify] https://crrev.com/fb43687746522d4fc167d268725007c0761f57e1/chrome/common/chrome_features.cc
[modify] https://crrev.com/fb43687746522d4fc167d268725007c0761f57e1/chrome/test/BUILD.gn

Comment 31 by harrisjay@chromium.org, Dec 10

Comparison of enabling and disabling AutoColorReadability on LocationIconView->label
Readability Disabled.png
202 KB View Download
Readability Enabled.png
218 KB View Download

Comment 32 by alancutter@chromium.org, Dec 10

You probably want to use (and upstream to the base class) GetReadableFrameForegroundColor() to match the colour of text in the title bar.

Comment 33 by dominickn@chromium.org, Dec 10

Is there a plan to add some sort of X or back gesture to "close" the CCT (i.e. navigate back to the first in-scope URL in the history list?). We do this on Android.

Comment 34 by harrisjay@chromium.org, Dec 11

alancutter@ that's what I'm doing. 

dominickn@ there is, but not with this changeset :)

Comment 35 by bugdroid1@chromium.org, Dec 12

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/8a4175d119c87220ce17df68adc78f58b0c96374

commit 8a4175d119c87220ce17df68adc78f58b0c96374
Author: Jay Harris <harrisjay@chromium.org>
Date: Wed Dec 12 04:06:56 2018

Use app theme color for custom tab bar.

Bug: 853593
Change-Id: I4c3a8b4e21b283cce61579973c94236dfc057904
Reviewed-on: https://chromium-review.googlesource.com/c/1318771
Commit-Queue: Jay Harris <harrisjay@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#615811}
[modify] https://crrev.com/8a4175d119c87220ce17df68adc78f58b0c96374/chrome/browser/ui/views/frame/browser_non_client_frame_view.h
[modify] https://crrev.com/8a4175d119c87220ce17df68adc78f58b0c96374/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.cc
[modify] https://crrev.com/8a4175d119c87220ce17df68adc78f58b0c96374/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash.h
[modify] https://crrev.com/8a4175d119c87220ce17df68adc78f58b0c96374/chrome/browser/ui/views/frame/browser_non_client_frame_view_mac.h
[modify] https://crrev.com/8a4175d119c87220ce17df68adc78f58b0c96374/chrome/browser/ui/views/frame/browser_non_client_frame_view_mac.mm
[modify] https://crrev.com/8a4175d119c87220ce17df68adc78f58b0c96374/chrome/browser/ui/views/frame/glass_browser_frame_view.cc
[modify] https://crrev.com/8a4175d119c87220ce17df68adc78f58b0c96374/chrome/browser/ui/views/frame/glass_browser_frame_view.h
[modify] https://crrev.com/8a4175d119c87220ce17df68adc78f58b0c96374/chrome/browser/ui/views/frame/opaque_browser_frame_view.cc
[modify] https://crrev.com/8a4175d119c87220ce17df68adc78f58b0c96374/chrome/browser/ui/views/frame/opaque_browser_frame_view.h
[modify] https://crrev.com/8a4175d119c87220ce17df68adc78f58b0c96374/chrome/browser/ui/views/location_bar/custom_tab_bar_view.cc
[modify] https://crrev.com/8a4175d119c87220ce17df68adc78f58b0c96374/chrome/browser/ui/views/location_bar/custom_tab_bar_view.h
[modify] https://crrev.com/8a4175d119c87220ce17df68adc78f58b0c96374/chrome/browser/ui/views/location_bar/location_icon_view.cc
[modify] https://crrev.com/8a4175d119c87220ce17df68adc78f58b0c96374/chrome/browser/ui/views/toolbar/toolbar_view.cc

Comment 36 by harrisjay@chromium.org, Dec 18

Close button (see top left of custom tab bar)
Screenshot from 2018-12-19 10-32-24.png
13.6 KB View Download

Comment 37 by bugdroid1@chromium.org, Dec 19

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c684323b978e024b67bcd645b61b1e8a35b7138b

commit c684323b978e024b67bcd645b61b1e8a35b7138b
Author: Jay Harris <harrisjay@chromium.org>
Date: Wed Dec 19 05:55:22 2018

Adds a 'Close' button to the custom tab bar view.

The close button navigates back to the last in scope url, if any. If no
in scope url is available (as when using window.location.replace(...))
the app's launch_url is navigated to and the history stack is cleared.

Notes:
- In future, the forward stack should be cleared when the close button
is clicked.

Screenshot:
https://bugs.chromium.org/p/chromium/issues/attachment?aid=372336&signed_aid=Zqm633OoKgTghf6XijsROQ==&inline=1

Bug: 853593
Change-Id: I0868500e6a98445ed5113b61bb6a7529bccb7d2c
Reviewed-on: https://chromium-review.googlesource.com/c/1381391
Reviewed-by: Avi Drissman <avi@chromium.org>
Commit-Queue: Jay Harris <harrisjay@chromium.org>
Cr-Commit-Position: refs/heads/master@{#617736}
[modify] https://crrev.com/c684323b978e024b67bcd645b61b1e8a35b7138b/chrome/browser/ui/views/location_bar/custom_tab_bar_view.cc
[modify] https://crrev.com/c684323b978e024b67bcd645b61b1e8a35b7138b/chrome/browser/ui/views/location_bar/custom_tab_bar_view.h
[modify] https://crrev.com/c684323b978e024b67bcd645b61b1e8a35b7138b/ui/views/controls/button/image_button_factory.cc
[modify] https://crrev.com/c684323b978e024b67bcd645b61b1e8a35b7138b/ui/views/controls/button/image_button_factory.h

Comment 38 Deleted

Comment 39 Deleted

Comment 40 by harrisjay@chromium.org, Dec 20

Adds larger screenshot, to make change more obvious. Note: this does not effect the window border itself, it stops lines from being drawn between the border and the toolbar.

This will align the Linux custom tab bars with ones on other platforms.
before.png
104 KB View Download
after.png
96.7 KB View Download

Comment 41 by harrisjay@chromium.org, Dec 20

Pictures of other platforms, for comparison
Mac.png
732 KB View Download
chromeos.png
207 KB View Download

Comment 42 by bugdroid1@chromium.org, Dec 21

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9d37d5147d8b9d9a20bd1b55f2f7daad06ed0c3e

commit 9d37d5147d8b9d9a20bd1b55f2f7daad06ed0c3e
Author: Jay Harris <harrisjay@chromium.org>
Date: Fri Dec 21 00:42:40 2018

Stop drawing border between window frame and toolbar in PWAs.

The custom tab bar should appear as an extension of the
title bar. Before this CL, this was not not the case on
Linux or Windows without DWM (disable-dwm-composition),
as we draw small borders between the window frame and the
toolbar. This CL removes them when displaying the custom
tab bar.

Before: https://bugs.chromium.org/p/chromium/issues/attachment?aid=372746&signed_aid=ybrF8gUqHfRhz-mI-R7-SQ==&inline=1
After: https://bugs.chromium.org/p/chromium/issues/attachment?aid=372747&signed_aid=-_9BQ_5YxzT6Gg67Aj49ag==&inline=1

Bug: 853593
Change-Id: Ice90aef02c074d344066f8b5dd8e50fe77310a8e
Reviewed-on: https://chromium-review.googlesource.com/c/1385685
Commit-Queue: Jay Harris <harrisjay@chromium.org>
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Cr-Commit-Position: refs/heads/master@{#618404}
[modify] https://crrev.com/9d37d5147d8b9d9a20bd1b55f2f7daad06ed0c3e/chrome/browser/ui/views/frame/opaque_browser_frame_view.cc

Comment 43 by bugdroid1@chromium.org, Jan 21

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/f3a6c253c7ae2eb42dd58de281587dbf110ef493

commit f3a6c253c7ae2eb42dd58de281587dbf110ef493
Author: Jay Harris <harrisjay@chromium.org>
Date: Mon Jan 21 04:01:20 2019

Updates the bottom border to be 15% black instead of black.

This is a UX issue that came up during  https://crbug.com/918788 

Bug: 853593
Change-Id: If0efdba2d344ee379fb560c61dc43ea4a14c0510
Reviewed-on: https://chromium-review.googlesource.com/c/1415232
Commit-Queue: Jay Harris <harrisjay@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624515}
[modify] https://crrev.com/f3a6c253c7ae2eb42dd58de281587dbf110ef493/chrome/browser/ui/views/location_bar/custom_tab_bar_view.cc

Comment 44 by bugdroid1@chromium.org, Jan 21

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4205d6e8d3370bc816d722e908a372921936dd8a

commit 4205d6e8d3370bc816d722e908a372921936dd8a
Author: Gabriel Charette <gab@chromium.org>
Date: Mon Jan 21 18:46:27 2019

Revert "Updates the bottom border to be 15% black instead of black."

This reverts commit f3a6c253c7ae2eb42dd58de281587dbf110ef493.

Reason for revert: suspected for https://bugs.chromium.org/p/chromium/issues/detail?id=923894#c14

Original change's description:
> Updates the bottom border to be 15% black instead of black.
> 
> This is a UX issue that came up during  https://crbug.com/918788 
> 
> Bug: 853593
> Change-Id: If0efdba2d344ee379fb560c61dc43ea4a14c0510
> Reviewed-on: https://chromium-review.googlesource.com/c/1415232
> Commit-Queue: Jay Harris <harrisjay@chromium.org>
> Reviewed-by: Bret Sepulveda <bsep@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#624515}

TBR=bsep@chromium.org,harrisjay@chromium.org

Change-Id: I7ff365b2fcfafdbd75f6bd506c56f07ab806579d
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 853593,  923894 
Reviewed-on: https://chromium-review.googlesource.com/c/1425997
Reviewed-by: Gabriel Charette <gab@chromium.org>
Commit-Queue: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624635}
[modify] https://crrev.com/4205d6e8d3370bc816d722e908a372921936dd8a/chrome/browser/ui/views/location_bar/custom_tab_bar_view.cc

Comment 45 by bugdroid1@chromium.org, Jan 21

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/c44ee8b46204c637a34012809857363ea88aac2f

commit c44ee8b46204c637a34012809857363ea88aac2f
Author: Gabriel Charette <gab@chromium.org>
Date: Mon Jan 21 21:25:59 2019

Reland "Updates the bottom border to be 15% black instead of black."

This reverts commit 4205d6e8d3370bc816d722e908a372921936dd8a.

Reason for reland : didn't fix it.

Original change's description:
> Revert "Updates the bottom border to be 15% black instead of black."
> 
> This reverts commit f3a6c253c7ae2eb42dd58de281587dbf110ef493.
> 
> Reason for revert: suspected for https://bugs.chromium.org/p/chromium/issues/detail?id=923894#c14
> 
> Original change's description:
> > Updates the bottom border to be 15% black instead of black.
> > 
> > This is a UX issue that came up during  https://crbug.com/918788 
> > 
> > Bug: 853593
> > Change-Id: If0efdba2d344ee379fb560c61dc43ea4a14c0510
> > Reviewed-on: https://chromium-review.googlesource.com/c/1415232
> > Commit-Queue: Jay Harris <harrisjay@chromium.org>
> > Reviewed-by: Bret Sepulveda <bsep@chromium.org>
> > Cr-Commit-Position: refs/heads/master@{#624515}
> 
> TBR=bsep@chromium.org,harrisjay@chromium.org
> 
> Change-Id: I7ff365b2fcfafdbd75f6bd506c56f07ab806579d
> No-Presubmit: true
> No-Tree-Checks: true
> No-Try: true
> Bug: 853593,  923894 
> Reviewed-on: https://chromium-review.googlesource.com/c/1425997
> Reviewed-by: Gabriel Charette <gab@chromium.org>
> Commit-Queue: Gabriel Charette <gab@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#624635}

TBR=gab@chromium.org,bsep@chromium.org,harrisjay@chromium.org

Change-Id: I6011811309c67dde14d7e390f8aabba54c5db111
No-Presubmit: true
No-Tree-Checks: true
No-Try: true
Bug: 853593,  923894 
Reviewed-on: https://chromium-review.googlesource.com/c/1426013
Commit-Queue: Gabriel Charette <gab@chromium.org>
Reviewed-by: Gabriel Charette <gab@chromium.org>
Cr-Commit-Position: refs/heads/master@{#624677}
[modify] https://crrev.com/c44ee8b46204c637a34012809857363ea88aac2f/chrome/browser/ui/views/location_bar/custom_tab_bar_view.cc

Comment 46 by bugdroid, Jan 30

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/3ba9a9c63f52d37434bc690dde3c05220c27ec04

commit 3ba9a9c63f52d37434bc690dde3c05220c27ec04
Author: Jay Harris <harrisjay@chromium.org>
Date: Wed Jan 30 22:38:41 2019

Enables CustomTabUI by default

Bug: 853593
Change-Id: I0670130efdc0e0891b449ab6ddfdf3de0019fb0f
Reviewed-on: https://chromium-review.googlesource.com/c/1405122
Reviewed-by: Alan Cutter <alancutter@chromium.org>
Commit-Queue: Jay Harris <harrisjay@chromium.org>
Cr-Commit-Position: refs/heads/master@{#627634}
[modify] https://crrev.com/3ba9a9c63f52d37434bc690dde3c05220c27ec04/chrome/common/chrome_features.cc
Labels: Needs-Feedback
Tried testing this issue on Windows 10 on the reported version 71.0.3541.0(build without fix) and the latest M-74-74.0.3689.0 (build without fix) by following the below steps.

1. Launched Chrome 
2. Opened the url " https://killer-marmot.appspot.com/web/ " clicked on post and observed after 3 seconds, it redirects back automatically 
3. Installed killer-marmot app and clicked on "Fork me on GitHub".
As we have observed that
On chrome M-74 74.0.3689.0 > GitHub opens with in PWA (killer-marmot app).
On chrome M-71 71.0.3541.0 > GitHub opens in a new browser tab.

Attached is the screen cast for reference.

Jay Harris@ Please find the above mentioned information and attached screencast for your reference and help us to verifying the fix on the latest M-74 build.

Thanks..!
853593.mp4
3.4 MB View Download

Comment 48 by harrisjay@chromium.org, Jan 31

Looks fixed to me :)

Comment 49 by bugdroid, Feb 4

Project Member
The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/a8bf717b03bc1324dbd6d6959f47053038abfba9

commit a8bf717b03bc1324dbd6d6959f47053038abfba9
Author: Jay Harris <harrisjay@chromium.org>
Date: Mon Feb 04 22:41:48 2019

Adds harrisjay as an owner as he's a committer now

Bug: 853593
Change-Id: I271f224f98b53eef7ee2cd7c81c4e2dd7c9642ec
Reviewed-on: https://chromium-review.googlesource.com/c/1451496
Reviewed-by: Peter Kasting <pkasting@chromium.org>
Reviewed-by: Bret Sepulveda <bsep@chromium.org>
Commit-Queue: Jay Harris <harrisjay@chromium.org>
Cr-Commit-Position: refs/heads/master@{#628912}
[modify] https://crrev.com/a8bf717b03bc1324dbd6d6959f47053038abfba9/chrome/browser/ui/views/location_bar/OWNERS
Labels: TE-Verified-74.0.3694.0 TE-Verified-M74
Verified the fix on Mac 10.14, Windows 10 and Ubuntu 17.10 using Chrome version #74.0.3694.0 as per the comment #0.
Attaching screen cast for reference.
Observed that the GitHub opens with in PWA (killer-marmot app).
Hence, the fix is working as expected. 
Adding the verified labels.
Note: Able to reproduce the issue on chrome version 71.0.3541.0

Thanks..!
853593.mp4
1.6 MB View Download

Sign in to add a comment