Add Touch Bar support for default browser window state |
||||||||||||||||||||
Issue descriptionChrome should default to placing the following controls in the Touch Bar for the main browser window: [esc] - Escape, text; not sure if it needs to be localized [<-] - Back, icon [->] - Forward, icon [reload] - Reload page, icon [home] - Home, icon; only if visible in the toolbar (so Touch Bar must update when user changes this in Settings) [X Search XXX or type address] - icon and text; see explanation below [new tab] - New Tab, icon [star] - Bookmark Star, icon; draw in blue when the current page is bookmarked For the above, "text" means the button shows the indicated text, "icon" means it shows the MD vector icon associated with this button in the toolbar. We are waiting for vector icons for "new tab" and "search". Vector images should be passed to the Touch Bar code as Appkit "template" images, so that they can be automatically tinted. Any icons that display color, like the blue bookmark star, should not be marked as templates. All buttons should follow the enable/disable state of their counterparts in the main browser window. So if it's not possible to go Back, the Touch Bar Back button should be disabled. ------- "X Search XXX or type address" button This button, when tapped, selects the text in the omnibox (or places the insertion point there, if no text). X = a search icon; you will use a vectorized Google "G" when the default search engine is Google, and the vector MD magnifying glass otherwise. XXX = name of search engine; so XXX should be replaced with "Google", "Bing", etc. This button needs to be updated when the user changes their default search engine in Settings.
,
Jan 17 2017
Here's a link to the vector assets for the "G" and new tab button. https://drive.google.com/corp/drive/u/0/folders/0BxMIIGI80eU-N3VxbnNHN0cxbDg Each part of the "G" should be a different color - although the vector image API in Chrome only takes one (or two at most) color inputs, it should be possible to just hard code the colors into the vector. bettes@ - can you specify colors for the four pieces of the G, and a color for the New Tab icon?
,
Jan 19 2017
Question for the star button: What happens if you tap it and the page is already bookmarked?
,
Jan 21 2017
Issue here, this is what I implemented so far. The problem with using the search name is that the button will get too long it'll end u push the Star button out. It gets worse if the service name is long. How about we just stick with "X Search or type URL", where X is the search/google icon?
,
Jan 23 2017
Re: c#4, it should be the same as if clicking the blue star icon in the omnibox if the page is already bookmarked (it brings up the bookmarks dialog). Re: c#5, I think we're still going to run into trouble when the localized version of the string makes the button long. It seems that Safari gets away with it by only including the forward/back buttons in the touch bar. I think we may want to make this button be an icon (magnifying glass or Google G) and the title of the search engine. bettes@ - what are your thoughts? spqchan@ - I notice that Safari groups the forward/back buttons so that there is very little space between the buttons. We probably want to group the buttons on the left and the right.
,
Jan 24 2017
Colors guide for G: https://drive.google.com/open?id=0BxMIIGI80eU-RmRWR252eHY5U00
,
Jan 24 2017
Can you attach a Safari touch bar for comparison? Few questions: - what string do we use for Google search engine? Search or type URL? - the Home button shouldn't be in the touchbar if it's not shown in the toolbar. I think we can remove that for starters. - the new tab icon looks smaller than the other icons. Is that expected or a result of the export?
,
Jan 24 2017
> what string do we use for Google search engine? Search or type URL? "Search Google or type URL" > the Home button shouldn't be in the touchbar if it's not shown in the toolbar. I think we can remove that for starters. Will do >The new tab icon looks smaller than the other icons. Is that expected or a result of the export? It's the result of the export, I might have to tweak it in order to make it bigger. I'm having trouble exporting both files, especially the G icon. I've been using estade@'s SVG tool, but I can't convert the G icon since it contains a transform value. I've been trying to get rid of that with no luck. I'm not that familiar with SVG files so I might be missing something. Would it be possible for you to simplify the file? I noticed that it looks off when I try to open it in Inkscape
,
Jan 25 2017
Chrome is able to render the svg so it should be good to use. Re: handling the transform value I guess estade@'s tool doesn't like it? If you edit it transform directive out of the svg will the tool accept what's left? The transforms appear to just be two simple translations that almost cancel each other out. Once you have the .icon output you could just add a TRANSLATE directive to the .icon file that is the net of the two translations. The vector icon system does not accept TRANSLATE but it looks like it would be simple to add it in (adjust PaintPath in paint_vector_icon.cc to call offset() on the SkPath). Re: the size of the new tab icon, I wonder if the vector image's size needs to be specified in the .icon file (if it's not already). I wrote a Cocoa tool that does a decent job of rendering the .icon files, which might help to check out bounding boxes, make sure the icons are centered, etc.
,
Jan 25 2017
That Cocoa tool sounds useful, do you mind if I can use it for the new tab icon? I played around with the new tab .icon file and I managed to get it to look bigger, but it doesn't look like it's centered. Thanks! I used https://jakearchibald.github.io/svgomg/ to clean up the .svg so I would only end up with the net translate. I'm iffy about adding an offset on the PaintPath for just one icon. If the icon gets used somewhere else, the person using it will have to be aware about adding the offset() to show it properly. I managed to applied the translate to each path in the SVG (I was doing it incorrectly earlier) and got the attached .svg file. I converted the resulting file to a .icon file but it looks kinda of weird. See the attached screenshot (the larger new tab icon is there too)
,
Jan 26 2017
I just sent you the code. Re: the net translate, I wasn't suggesting you hard code that into the Chrome code. Right now the icon files support a subset of SVG commands. That subset does not include translate, but it should be simple to add a translate command. Then, you could just add a line in the icon file that specifies the net translation amount. But... I opened the G svg in Photoshop and the size is 250x250, with the icon much smaller (there's about 50pt of padding all around). That padding is what's making it hard to center the icon. Maybe bettes@ can supply versions of these icons with much tighter bounding boxes? Re: the g looking strange, I can see that's from rects filled with white. If you look in the SVG you'll see two places that say something like <mask id="mask-2" fill="white">. If you remove the fill="white" part the white rects go away. You should also be able to get the g to use the colors in c#8. If you look in the SVG you'll see <path ... fill="#000000" ... >. Replacing those #000000 values with the appropriate hex colors will get the different parts of the icon to show color.
,
Jan 27 2017
Great, thanks! I'll give it a try on the new tab icon It looks like removing the mask makes no difference to the .icon file. It looks like estade@'s tool might be ignoring masks. If you look at the G .svg file, the icon is made up of 4 parts that are stuck together. I suspect that after I applied the translation and shrunk the icon from 60 to 16, precision issues are created, which causes the 4 parts to separate from each other. bette@, it will be great if you can supply what shrike@ had mentioned in #13 :)
,
Jan 27 2017
Do you have your changes as a cl that I can play with?
,
Jan 27 2017
#15: Sent you the link
,
Feb 4 2017
New G svg without masks: https://drive.google.com/open?id=0BxMIIGI80eU-WHV3dThjX2J3ek0 Update string to: "Search or type URL" for Google as default browser. (Same as bling)
,
Feb 4 2017
,
Feb 6 2017
Thanks!
,
Feb 6 2017
This looks great - can't wait to try it out! One thing: can you change the Google search icon to use its four colors (see c#13)? That will really make that button come alive.
,
Feb 6 2017
Looks nice spqchan! In addition to the request in #22, can we close the gap in the touch bar UI by adding additional padding to the search box AND centering the text+icon? In the same vein as Safari, I think it's beneficial to provide a large enough touch target for new searches.
,
Feb 6 2017
Not easy. The masks in the original icon were for creating the four colors, but it didn't really go so well. I also don't think we have the methods to draw a vector with multiple colors, so we will have to implement them. We'll also have to figure out how to get the vector to look okay with the four segments, even when we shrink them down. One option is to hardcoding the drawing for now and then implement the methods later. We'll still need to get the 4 segment vector icon working though
,
Feb 6 2017
Response to #23: sure thing. I'll give that a try
,
Feb 7 2017
Try out this colorized version of g_search.icon.
,
Feb 7 2017
Oh nice, thanks for it. This is what it looks like now. The issue is that the icon is a bit small. We'll have to adjust the SVG to reduce the padding and change the size from 60 to 36.
,
Feb 7 2017
If you can create a 36x36 .icon from an svg with masks I can make the same tweaks to that .icon file that I made to the one in c#26.
,
Feb 7 2017
Sure thing, here you go
,
Feb 8 2017
Can you turn it into a .icon?
,
Feb 9 2017
Ah sorry, I missed that part. I took a closer look at your .icon file and was able to make similar changes to the shifted .icon file. Here's what the touch bar now looks like. I'm just need to figure out how to add additional padding to the Touch Bar button. Thanks for your help!
,
Feb 9 2017
,
Feb 9 2017
Looks really good. Please note that M58 feature freeze is in 8 days so you'll want to start thinking about getting this landed (even if it's not visually perfect).
,
Feb 10 2017
Also quick question: I'm putting this behind a feature flag and it's enabled by default. Should I make it disabled by default? Also, we probably need to run to by UI launch review right? Are the any plans for that atm?
,
Feb 10 2017
It seems like the bookmark star is now missing? Re: your rollout questions in c#35, I think this should be enabled by default, with the flag allowing us to disable if there's a problem (I guess). I'm not actually sure it needs to live behind a flag. +rpop@ for thoughts about that, and the UI review process.
,
Feb 11 2017
,
Feb 18 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/4ad0a609fea4c17f2b2d71e446bfae18c956b243 commit 4ad0a609fea4c17f2b2d71e446bfae18c956b243 Author: spqchan <spqchan@chromium.org> Date: Sat Feb 18 09:16:53 2017 [Mac] Touch Bar support for default browser window state BUG= 675254 Review-Url: https://codereview.chromium.org/2695493002 Cr-Commit-Position: refs/heads/master@{#451448} [modify] https://crrev.com/4ad0a609fea4c17f2b2d71e446bfae18c956b243/base/mac/sdk_forward_declarations.h [modify] https://crrev.com/4ad0a609fea4c17f2b2d71e446bfae18c956b243/chrome/app/generated_resources.grd [modify] https://crrev.com/4ad0a609fea4c17f2b2d71e446bfae18c956b243/chrome/app/vector_icons/BUILD.gn [add] https://crrev.com/4ad0a609fea4c17f2b2d71e446bfae18c956b243/chrome/app/vector_icons/new_tab_mac_touchbar.icon [modify] https://crrev.com/4ad0a609fea4c17f2b2d71e446bfae18c956b243/chrome/browser/ui/BUILD.gn [modify] https://crrev.com/4ad0a609fea4c17f2b2d71e446bfae18c956b243/chrome/browser/ui/cocoa/browser_window_controller.h [modify] https://crrev.com/4ad0a609fea4c17f2b2d71e446bfae18c956b243/chrome/browser/ui/cocoa/browser_window_controller.mm [add] https://crrev.com/4ad0a609fea4c17f2b2d71e446bfae18c956b243/chrome/browser/ui/cocoa/browser_window_touch_bar.h [add] https://crrev.com/4ad0a609fea4c17f2b2d71e446bfae18c956b243/chrome/browser/ui/cocoa/browser_window_touch_bar.mm [add] https://crrev.com/4ad0a609fea4c17f2b2d71e446bfae18c956b243/chrome/browser/ui/cocoa/browser_window_touch_bar_unittest.mm [modify] https://crrev.com/4ad0a609fea4c17f2b2d71e446bfae18c956b243/chrome/browser/ui/cocoa/framed_browser_window.mm [modify] https://crrev.com/4ad0a609fea4c17f2b2d71e446bfae18c956b243/chrome/common/chrome_features.cc [modify] https://crrev.com/4ad0a609fea4c17f2b2d71e446bfae18c956b243/chrome/common/chrome_features.h [modify] https://crrev.com/4ad0a609fea4c17f2b2d71e446bfae18c956b243/chrome/test/BUILD.gn [modify] https://crrev.com/4ad0a609fea4c17f2b2d71e446bfae18c956b243/ui/base/cocoa/touch_bar_forward_declarations.h
,
Feb 21 2017
,
Feb 22 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/f402834abbe6b9b4a49306196721d390e048e05f commit f402834abbe6b9b4a49306196721d390e048e05f Author: spqchan <spqchan@chromium.org> Date: Wed Feb 22 05:50:46 2017 [Mac] Increase size of the Touch Bar New Tab Icon Follow up to estade's comments in https://codereview.chromium.org/2695493002/ BUG= 675254 Review-Url: https://codereview.chromium.org/2710493004 Cr-Commit-Position: refs/heads/master@{#451893} [modify] https://crrev.com/f402834abbe6b9b4a49306196721d390e048e05f/chrome/app/vector_icons/new_tab_mac_touchbar.icon
,
Jun 10 2017
|
||||||||||||||||||||
►
Sign in to add a comment |
||||||||||||||||||||
Comment 1 by shrike@chromium.org
, Dec 16 2016