Issue metadata
Sign in to add a comment
|
[Touchable] Hosted, Bookmark and PWA app window title bar is too tall |
||||||||||||||||||||||
Issue descriptionChrome Version: 67 OS: Chrome OS What steps will reproduce the problem? (1) Enable #top-chrome-md = Touchable in chrome:flags (I believe this is ON by default in 67). (2) Install a Hosted app (e.g., https://chrome.google.com/webstore/detail/google-finance/fcgckldmmjdbpdejkclmfnnnehhocbfp), set it to Open in Window, and open it. (3) Install a Bookmark app (e.g., from any web page, Chrome menu -> Create shortcut or Add to shelf), set it to Open in Window, and open it. (4) Install a PWA (e.g., https://santatracker.google.com, Chrome menu -> Install Santa Tracker), and open it. What is the expected result? In all three cases, the title bar should be thin, like on other system apps (like Files app, packaged apps like Google Keep or Android apps). What happens instead? Instead, it is thick like the Chrome browser. This is particularly ugly when you mouse over the close, minimize and maximize buttons. See Hwi's screenshot, showing (from top to bottom): - A system app (Files) -- as expected. - A packaged app (Keep) -- as expected. - A Bookmark app (Baseball) -- too tall. - A PWA (Starbucks) -- too tall. - Browser window (Chrome) -- tall as intended. The Hosted app window (which shares code across all three app types) needs to not have the extra height added for normal browser windows. Ahmed: Are you the right person to look into this? (I was told you implemented the tab strip changes for Touchable).
,
Apr 9 2018
Seems to also affect popup windows: Issue 830103. (If you consider these issues to be dupes, please dupe that one into this, not the other way around, due to the RVG on that one.)
,
Apr 9 2018
Update: It turns out this is the extension icon's doing. The bug doesn't appear until you install an extension. So we just need to revert the way that top-chrome-md interacts with the BrowserActionContainer when inside a HostedAppButtonContainer. +calamity knows the details but I think Ahmed is still in the best position to fix it. Thanks!
,
Apr 9 2018
Issue 830103 has been merged into this issue.
,
Apr 9 2018
,
Apr 10 2018
Agreed with sgabriel@ to make the hosted app frame menu button have the same style as that of the other caption buttons (see attached video). +pkasting@ +oshima@ FYI for review.
,
Apr 10 2018
#6: I notice there is a back button in the title bar, is that part of the change?
,
Apr 10 2018
CL is: https://chromium-review.googlesource.com/c/chromium/src/+/1005787 (please CC me on reviews pertaining to Desktop PWAs, thanks). > Agreed with sgabriel@ to make the hosted app frame menu button have the same style as that of the other caption buttons (see attached video). I'll have a look at this now. Please note that the PWA title bar was deliberately designed this way by hwi@ (see go/dpwa-animations) and I am hesitant to make snap changes to the design right before branch point. If you are going to make changes, please keep in mind that there are other (less commonly seen) icons and buttons that appear in the DPWA title bar: - Permissions icons (can be seen by installing https://permission.site and then enabling some permissions) and other browser actions (can be seen by installing https://inbox.google.com and then waiting for the protocol handler button to appear). - Extensions buttons (can be seen by installing any extension, then clicking its action button within the app menu). The DPWA design was to have permissions icons, browser actions, extensions buttons, and the app menu having the same style (small square with inkdrop) because they open menus, and the min/max/close buttons having a different style (large square with shade) because they do not open menus. After top-chrome-md touchable, all four of these surfaces have different style, so I agree we should fix them up. Happy to go with Sebastien's design but I want to loop in CrOS UI review.
,
Apr 11 2018
+omrilio@ for mgiuca@'s concerns in comment #8. Regarding the back button, it's hidden behind a flag by default, so no, it's not part of this change. I just keep it enabled to make sure I don't regress it. mgiuca@, I added you as a reviewer. I believe sgabriel@'s suggestion is the way to go in order to keep things consistent.
,
Apr 11 2018
> I believe sgabriel@'s suggestion is the way to go in order to keep things consistent. OK but to be clear, the change to the app menu button styling (both the ink drop and the size) is unrelated to fixing the title bar height, correct? If so, I'd like to split those out into separate CLs because the height fix is necessary and the app menu styling is debatable (and directly contradicts our approved design). Can you please split those out so we can segment the debate. As for the height fix, unfortunately it breaks extension icons: they are now cropped off the bottom and look awful, as shown in this screenshot. I have to run to a meeting now but then I will be available to discuss the technical fix. (calamity knows more though)
,
Apr 11 2018
Splitting into two separate CLs SGTM. Regarding the extension icons: That looks terrible! I didn't know we will show extension icons in the header frame! This will require a bit of refactoring to fix. It will also require decision about the style of the ink drop for the extension icons, as you can see in your screenshot, it's circular with a flood fill, which is different from the style of the caption buttons as well as the menu button (whether we leave it as is or follow sgabriel@'s suggestion). It would have been nice if both teams sync'd together at some point initially to avoid such conflict.
,
Apr 11 2018
#11: Yeah, it is actually the extension icon that's causing the height difference (see #3). If you uninstall all extensions then you get a normal-height title bar. I think the right fix is therefore to just change the size of the extension icon's circle when in a hosted app container, to match the other page action icons. Is that the refactoring that you mentioned? > It would have been nice if both teams sync'd together at some point initially to avoid such conflict. I agree. I guess neither of us knew the other was launching in M67 (if I did, I'd have had the touchable flag on all my devices awhile ago).
,
Apr 11 2018
> #11: Yeah, it is actually the extension icon that's causing the height difference (see #3). If you uninstall all extensions then you get a normal-height title bar. Yes, I noticed the height difference is cause by the BrowserActionsContainer, but I thought we'd always use it in the overflow mode (i.e. all extensions inside the menu). I have a bunch of extensions installed, but none of them showed on the frame. Maybe I needed to try dragging to resize that container to see if that's supported? In all cases, I'll try to fix this and leave the debatable ink drop styles for later.
,
Apr 11 2018
I've put together some screencasts showing three different states (all have #desktop-pwas on): - no-touchable-no-afakhry-patch: --top-chrome-md is off. (This is our original, approved design.) You can see that the app menu (mostly) matches the page action icons and extension icon. (Though the page action icons have an ink-drop effect while the other two have a ripple effect.) - touchable-no-afakhry-patch: --top-chrome-md is Touchable. (This is the current state, the subject of this bug.) You can see that the title bar is too tall, and you cannot access the X button in the top right corner (Fitt's Law violation). - touchable-afakhry-patch: --top-chrome-md is Touchable, with CL 1005787 applied. This fixes the title bar height, but the extension icon is cut off at the bottom (#10). The app menu is now consistent with the caption buttons, but inconsistent with the page action and extension icons. As I said earlier, I am happy for us to change the app menu to match the caption buttons, but I think it feels weird to have a menu open up with no animation (and I think that was Hwi's original design point, why it has a small ripple instead).
,
Apr 11 2018
Please note there are two CLs to fix this (due to a lack of coordination on our part): afakhry's: https://chromium-review.googlesource.com/c/chromium/src/+/1006490 calamity's: https://chromium-review.googlesource.com/c/chromium/src/+/1006135 I think we should go with calamity's because it's refactored a bit nicer. Comments on the CLs. Thanks to both of you for the extreme effort to fix this at short notice!
,
Apr 12 2018
This bug had an even weirder regression yesterday, after r549770 landed (I bisected to find it). As of that CL, the title bar is back to its usual size, EXCEPT when the extension icon is shown. See the new screencast. I think Calamity's CL is still useful to fix this properly, though.
,
Apr 12 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/6f9bcf6caf008fd582ef72bf8b88fe6984fa16bb commit 6f9bcf6caf008fd582ef72bf8b88fe6984fa16bb Author: Christopher Lam <calamity@chromium.org> Date: Thu Apr 12 07:03:00 2018 [desktop-pwas] Fix browser actions in touch optimized mode. This CL forces the hosted app browser actions to be 32x32, even in touch optimized mode. This fixes an issue where the hosted app title bar was too tall. Bug: 830285 Change-Id: I1bc3539f8e43ca332783511dbb937389f09eb20b Reviewed-on: https://chromium-review.googlesource.com/1006135 Commit-Queue: calamity <calamity@chromium.org> Reviewed-by: Peter Kasting <pkasting@chromium.org> Reviewed-by: Alan Cutter <alancutter@chromium.org> Cr-Commit-Position: refs/heads/master@{#550060} [modify] https://crrev.com/6f9bcf6caf008fd582ef72bf8b88fe6984fa16bb/chrome/browser/ui/cocoa/extensions/browser_actions_controller.mm [modify] https://crrev.com/6f9bcf6caf008fd582ef72bf8b88fe6984fa16bb/chrome/browser/ui/extensions/extension_action_view_controller_unittest.cc [modify] https://crrev.com/6f9bcf6caf008fd582ef72bf8b88fe6984fa16bb/chrome/browser/ui/toolbar/browser_actions_bar_browsertest.cc [modify] https://crrev.com/6f9bcf6caf008fd582ef72bf8b88fe6984fa16bb/chrome/browser/ui/toolbar/toolbar_actions_bar.cc [modify] https://crrev.com/6f9bcf6caf008fd582ef72bf8b88fe6984fa16bb/chrome/browser/ui/toolbar/toolbar_actions_bar.h [modify] https://crrev.com/6f9bcf6caf008fd582ef72bf8b88fe6984fa16bb/chrome/browser/ui/toolbar/toolbar_actions_bar_unittest.cc [modify] https://crrev.com/6f9bcf6caf008fd582ef72bf8b88fe6984fa16bb/chrome/browser/ui/views/frame/browser_non_client_frame_view_ash_browsertest.cc [modify] https://crrev.com/6f9bcf6caf008fd582ef72bf8b88fe6984fa16bb/chrome/browser/ui/views/frame/hosted_app_button_container.cc [modify] https://crrev.com/6f9bcf6caf008fd582ef72bf8b88fe6984fa16bb/chrome/browser/ui/views/toolbar/browser_actions_container.cc [modify] https://crrev.com/6f9bcf6caf008fd582ef72bf8b88fe6984fa16bb/chrome/browser/ui/views/toolbar/browser_actions_container.h [modify] https://crrev.com/6f9bcf6caf008fd582ef72bf8b88fe6984fa16bb/chrome/browser/ui/views/toolbar/extension_toolbar_menu_view.cc [modify] https://crrev.com/6f9bcf6caf008fd582ef72bf8b88fe6984fa16bb/chrome/browser/ui/views/toolbar/toolbar_action_view.cc [modify] https://crrev.com/6f9bcf6caf008fd582ef72bf8b88fe6984fa16bb/chrome/browser/ui/views/toolbar/toolbar_action_view.h [modify] https://crrev.com/6f9bcf6caf008fd582ef72bf8b88fe6984fa16bb/chrome/browser/ui/views/toolbar/toolbar_action_view_unittest.cc
,
Apr 12 2018
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by mgiuca@chromium.org
, Apr 9 2018