PWA with white border has an incomplete grey line |
|||||||||||||||||||||
Issue descriptionChrome Version: 72.0.3623.3 OS: Chrome OS What steps will reproduce the problem? (1) Go to https://killer-marmot.appspot.com/custom/eyJuYW1lIjogIktpbGxlciBNYXJtb3QgKHdoaXRlIHRpdGxlKSIsICJzaG9ydF9uYW1lIjogIk1hcm1vdCAod2hpdGUgdGl0bGUpIiwgImRpc3BsYXkiOiAic3RhbmRhbG9uZSIsICJzdGFydF91cmwiOiAiLiIsICJ0aGVtZV9jb2xvciI6ICIjZmZmIn0=/ (2) Chrome menu -> Install Killer Marmot What is the expected result? Horizontal grey line runs across the width of the title, underneath the title. What happens instead? Horizontal grey line is cut off on the left side. This only reproduces occasionally. See screenshot (it's example.com, not killer-marmot; I couldn't actually repro on killer marmot, but the example.com case requires hacking in a manifest). Note: This only happens with theme_color: #fff. If you don't have a theme_color, it doesn't paint a line.
,
Jan 10
Interestingly, it only happens on PWAs (creating a shortcut that opens as a window doesn't show this)
,
Jan 11
,
Jan 11
The line is being drawn here: https://cs.chromium.org/chromium/src/chrome/browser/ui/views/frame/browser_root_view.cc?sq=package:chromium&dr=CSs&g=0&l=342 I have a fairly trivial fix for removing it in PWAs but it looks like this code may no longer be used, and whether we can remove it? Previously, it was rendering a border around the outside of our tabs, but we no longer seem to do that. @thomas, it looks like you were the last one to touch the lines that draw the rectangle (https://chromium-review.googlesource.com/c/chromium/src/+/1194674/), do you know anything more about this? CL for fixing PWAs is here: https://chromium-review.googlesource.com/c/chromium/src/+/1405115
,
Jan 11
It's still necessary when using certain themes. The attached image shows what removing it does on the Linux/GTK theme. You should also be able to reproduce with certain web themes where the contrast between the frame/toolbar is low
,
Jan 11
Cool, thanks Thomas. I'll go ahead with out patch (we want there to be no contrast between the frame and toolbar in hosted apps)
,
Jan 11
Screenshots of behavior before and after patch.
,
Jan 11
Honestly, the after-cct screenshot (what is a "cct"?) looks kinda buggy, because the window controls and the page title aren't vertically aligned. Is there a plan to fix that? If not, maybe we should leave the line?
,
Jan 11
,
Jan 14
Sounds like something to be raised with UX. Without a window title it does look a bit odd having nothing but the vertical misalignment to distinguish the CCT controls from the window frame controls.
,
Jan 14
#E0E0E0 looks like it works for white theme colours. We should probably have a generic solution for any possible site theme colour.
,
Jan 14
For non-white, I suggest we hide the top border.
,
Jan 14
I've attached a screenshot where the site's theme_color is #E0E0E0. If I'm understanding you correctly, we wouldn't draw a line in this case? Or are you saying the custom tab bar should always be white? Let's talk it over in our UX sync.
,
Jan 15
Yes the custom tab bar should be always white. It's bad to see two vibrant colors, and we would rather keep the parent PWA. Please let me know if any points have been missed and need to be addressed. Thanks harrisjay@!
,
Jan 16
Also updated suggestion re: condition to draw the line How about - the line color is 15% #000 (instead of hex e0e0e0), and draw the line when the contrast of the app color to white is 1.1 or below (15% and 1.1 - picked these from a quick try from Sketch, we might need to adjust in the future) / Probably subtle, the % black might blend better to the app title bar color than hex gray. - And probably *bottom align* the line to the title bar. Thanks!
,
Jan 17
(6 days ago)
Screenshots of old/new custom tab bar (after removing the broken line, always making the custom tab bar white, and adding a proper separator).
,
Jan 17
(6 days ago)
More clarification per discussion with harrisjay@ 1. No CCT, No line: if it's a regular state, i.e. app title bar only without CCT, no line should be drawn even if the app bar color is white. 2. CCT is always white bg + 15% black bottom border 3. CCT conditionally gets the top border (15% black) if the app bar is white/close-to-white (contrast to white is 1.1 or below) Please call out if there's anything strange or unclear. Thanks!
,
Jan 17
(6 days ago)
c17 - Thanks harrisjay@ Probably tricky, but, could we make the bottom separator "15% black" as well instead of black?
,
Jan 17
(6 days ago)
We certainly can. I was just about to start on this.
,
Jan 17
(6 days ago)
There's a CL for changing the bottom separator color here https://chromium-review.googlesource.com/c/chromium/src/+/1415232
,
Jan 17
(6 days ago)
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/81cfd76abd6e45673dd64196e2ffadc3a0f8a070 commit 81cfd76abd6e45673dd64196e2ffadc3a0f8a070 Author: Jay Harris <harrisjay@chromium.org> Date: Thu Jan 17 02:53:37 2019 Removes grey line between title bar and content for desktop PWAs. Note: The line this disables was being drawn unintentionally (and, in some cases, not properly). In future, we will add a similar line controlled by the custom tab bar, which will allow us to blend and control whether or not it displays based on theme and custom tab bar color. Bug: 918788 Change-Id: I5b6e7a9afe55a0fd337dce12db142c473b64af26 Reviewed-on: https://chromium-review.googlesource.com/c/1414650 Commit-Queue: Jay Harris <harrisjay@chromium.org> Reviewed-by: Bret Sepulveda <bsep@chromium.org> Cr-Commit-Position: refs/heads/master@{#623538} [modify] https://crrev.com/81cfd76abd6e45673dd64196e2ffadc3a0f8a070/chrome/browser/ui/views/frame/browser_non_client_frame_view.cc
,
Jan 21
(2 days ago)
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/24d10eb4179c64f7929d92f7e75a4e16b68219f4 commit 24d10eb4179c64f7929d92f7e75a4e16b68219f4 Author: Jay Harris <harrisjay@chromium.org> Date: Mon Jan 21 01:33:24 2019 Updates the custom tab bar to always be white. This emphasizes the fact that when the custom tab bar is showing, you are no longer in the PWA. Note: A future CL will introduce a separator when the app's theme color is close to white. Bug: 918788 Change-Id: I2106f36bb92dac49b7d0a76b3233df37bf749150 Reviewed-on: https://chromium-review.googlesource.com/c/1416791 Commit-Queue: Jay Harris <harrisjay@chromium.org> Reviewed-by: Bret Sepulveda <bsep@chromium.org> Cr-Commit-Position: refs/heads/master@{#624503} [modify] https://crrev.com/24d10eb4179c64f7929d92f7e75a4e16b68219f4/chrome/browser/ui/views/frame/browser_non_client_frame_view.cc [modify] https://crrev.com/24d10eb4179c64f7929d92f7e75a4e16b68219f4/chrome/browser/ui/views/frame/browser_non_client_frame_view.h [modify] https://crrev.com/24d10eb4179c64f7929d92f7e75a4e16b68219f4/chrome/browser/ui/views/location_bar/custom_tab_bar_view.cc [modify] https://crrev.com/24d10eb4179c64f7929d92f7e75a4e16b68219f4/chrome/browser/ui/views/location_bar/custom_tab_bar_view.h
,
Jan 21
(2 days ago)
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f207ec9653d036442e5738a4740b04506d7fe346 commit f207ec9653d036442e5738a4740b04506d7fe346 Author: Jay Harris <harrisjay@chromium.org> Date: Mon Jan 21 03:25:21 2019 Adds a separator between custom tab bar and title, based on UX feedback. Previously, a similar separator was drawn. However, this separator was always shown (when the theme color was white-ish), and the desired behavior is to *only* render the separator when the custom tab bar is visible AND the custom tab bar is white-ish. This CL will cause a 15% black separator to be drawn at the top of the custom tab bar, blended with the apps theme color, so it looks like the separator is drawn onto the title bar. Bug: 918788 Change-Id: Iba020fe4f059558fe355d18236d5226d35f3a2aa Reviewed-on: https://chromium-review.googlesource.com/c/1414535 Commit-Queue: Jay Harris <harrisjay@chromium.org> Reviewed-by: Bret Sepulveda <bsep@chromium.org> Cr-Commit-Position: refs/heads/master@{#624514} [modify] https://crrev.com/f207ec9653d036442e5738a4740b04506d7fe346/chrome/browser/ui/views/location_bar/custom_tab_bar_view.cc [modify] https://crrev.com/f207ec9653d036442e5738a4740b04506d7fe346/chrome/browser/ui/views/location_bar/custom_tab_bar_view.h
,
Yesterday
(35 hours ago)
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/de610823c81c9740c5e5174dc748389caeb37f35 commit de610823c81c9740c5e5174dc748389caeb37f35 Author: Gabriel Charette <gab@chromium.org> Date: Mon Jan 21 18:50:42 2019 Revert "Adds a separator between custom tab bar and title, based on UX feedback." This reverts commit f207ec9653d036442e5738a4740b04506d7fe346. Reason for revert: suspected for https://bugs.chromium.org/p/chromium/issues/detail?id=923894#c14 Original change's description: > Adds a separator between custom tab bar and title, based on UX feedback. > > Previously, a similar separator was drawn. However, this separator > was always shown (when the theme color was white-ish), and the desired > behavior is to *only* render the separator when the custom tab bar > is visible AND the custom tab bar is white-ish. > > This CL will cause a 15% black separator to be drawn at the top of the > custom tab bar, blended with the apps theme color, so it looks like > the separator is drawn onto the title bar. > > Bug: 918788 > Change-Id: Iba020fe4f059558fe355d18236d5226d35f3a2aa > Reviewed-on: https://chromium-review.googlesource.com/c/1414535 > Commit-Queue: Jay Harris <harrisjay@chromium.org> > Reviewed-by: Bret Sepulveda <bsep@chromium.org> > Cr-Commit-Position: refs/heads/master@{#624514} TBR=bsep@chromium.org,harrisjay@chromium.org Change-Id: I16e4607f127301e3c579277f92b2549b51a92266 No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 918788, 923894 Reviewed-on: https://chromium-review.googlesource.com/c/1425999 Reviewed-by: Gabriel Charette <gab@chromium.org> Commit-Queue: Gabriel Charette <gab@chromium.org> Cr-Commit-Position: refs/heads/master@{#624638} [modify] https://crrev.com/de610823c81c9740c5e5174dc748389caeb37f35/chrome/browser/ui/views/location_bar/custom_tab_bar_view.cc [modify] https://crrev.com/de610823c81c9740c5e5174dc748389caeb37f35/chrome/browser/ui/views/location_bar/custom_tab_bar_view.h
,
Yesterday
(33 hours ago)
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/02f2825ab10a2d6bae105d2a6c94c28f055193e9 commit 02f2825ab10a2d6bae105d2a6c94c28f055193e9 Author: Gabriel Charette <gab@chromium.org> Date: Mon Jan 21 21:06:50 2019 Reland "Adds a separator between custom tab bar and title, based on UX feedback." This reverts commit de610823c81c9740c5e5174dc748389caeb37f35. Reason for reland : didn't fix it. Original change's description: > Revert "Adds a separator between custom tab bar and title, based on UX feedback." > > This reverts commit f207ec9653d036442e5738a4740b04506d7fe346. > > Reason for revert: suspected for https://bugs.chromium.org/p/chromium/issues/detail?id=923894#c14 > > Original change's description: > > Adds a separator between custom tab bar and title, based on UX feedback. > > > > Previously, a similar separator was drawn. However, this separator > > was always shown (when the theme color was white-ish), and the desired > > behavior is to *only* render the separator when the custom tab bar > > is visible AND the custom tab bar is white-ish. > > > > This CL will cause a 15% black separator to be drawn at the top of the > > custom tab bar, blended with the apps theme color, so it looks like > > the separator is drawn onto the title bar. > > > > Bug: 918788 > > Change-Id: Iba020fe4f059558fe355d18236d5226d35f3a2aa > > Reviewed-on: https://chromium-review.googlesource.com/c/1414535 > > Commit-Queue: Jay Harris <harrisjay@chromium.org> > > Reviewed-by: Bret Sepulveda <bsep@chromium.org> > > Cr-Commit-Position: refs/heads/master@{#624514} > > TBR=bsep@chromium.org,harrisjay@chromium.org > > Change-Id: I16e4607f127301e3c579277f92b2549b51a92266 > No-Presubmit: true > No-Tree-Checks: true > No-Try: true > Bug: 918788, 923894 > Reviewed-on: https://chromium-review.googlesource.com/c/1425999 > Reviewed-by: Gabriel Charette <gab@chromium.org> > Commit-Queue: Gabriel Charette <gab@chromium.org> > Cr-Commit-Position: refs/heads/master@{#624638} TBR=gab@chromium.org,bsep@chromium.org,harrisjay@chromium.org Change-Id: I107e52387e2fd3a5072c484e39c883cfabe7349c No-Presubmit: true No-Tree-Checks: true No-Try: true Bug: 918788, 923894 Reviewed-on: https://chromium-review.googlesource.com/c/1425968 Reviewed-by: Gabriel Charette <gab@chromium.org> Commit-Queue: Gabriel Charette <gab@chromium.org> Cr-Commit-Position: refs/heads/master@{#624671} [modify] https://crrev.com/02f2825ab10a2d6bae105d2a6c94c28f055193e9/chrome/browser/ui/views/location_bar/custom_tab_bar_view.cc [modify] https://crrev.com/02f2825ab10a2d6bae105d2a6c94c28f055193e9/chrome/browser/ui/views/location_bar/custom_tab_bar_view.h |
|||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||
Comment 1 by mgiuca@chromium.org
, Jan 3