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

Issue 675254 link

Starred by 32 users

Issue metadata

Status: Fixed
Owner:
Last visit > 30 days ago
Closed: Jun 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Mac
Pri: 2
Type: Bug

Blocked on:
issue 694760

Blocking:
issue 660126


Show other hotlists

Hotlists containing this issue:
Touch-Bar


Sign in to add a comment

Add Touch Bar support for default browser window state

Project Member Reported by shrike@chromium.org, Dec 16 2016

Issue description

Chrome 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.

 

Comment 1 by shrike@chromium.org, Dec 16 2016

Cc: spqc...@chromium.org
 Issue 672627  has been merged into this issue.

Comment 2 Deleted

Comment 3 by shrike@chromium.org, Jan 17 2017

Cc: bettes@chromium.org
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?

Question for the star button: What happens if you tap it and the page is already bookmarked?
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?
Screen Shot 2017-01-20 at 4.22.37 PM.png
403 KB View Download
With the shortened title
Screen Shot 2017-01-20 at 4.39.27 PM.png
366 KB View Download

Comment 7 by shrike@chromium.org, Jan 23 2017

Owner: bettes@chromium.org
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.

Comment 9 by bettes@chromium.org, Jan 24 2017

Owner: spqc...@chromium.org
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? 




Owner: bettes@chromium.org
> 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

Screen Shot 2017-01-24 at 12.59.39 PM.png
422 KB View Download
Screen Shot 2017-01-24 at 1.02.27 PM.png
124 KB View Download
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.

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)

Screen Shot 2017-01-24 at 6.45.01 PM.png
1.4 MB View Download
glyph_google_translate_applied.svg
3.1 KB Download
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.

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 :)
Do you have your changes as a cl that I can play with?
Tweaked the new tab icon and managed to get it aligned and bigger. Here's the result
Screen Shot 2017-01-26 at 6.14.58 PM.png
1.6 MB View Download
#15: Sent you the link
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)
Cc: -spqc...@chromium.org
Owner: spqc...@chromium.org
Thanks!
Touch bar with the changes
Screen Shot 2017-02-06 at 11.47.23 AM.png
969 KB View Download
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.
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. 


Screen Shot 2017-02-06 at 1.00.23 PM.png
134 KB View Download
Screen Shot 2017-02-06 at 1.01.02 PM.png
62.7 KB View Download
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




Response to #23: sure thing. I'll give that a try
Try out this colorized version of g_search.icon.
g_search.icon
1.4 KB Download
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.
Screen Shot 2017-02-07 at 5.40.55 AM.png
1.5 MB View Download
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.
Sure thing, here you go
glyph_google_shifted.svg
2.8 KB Download
Can you turn it into a .icon?
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!
Screen Shot 2017-02-09 at 1.11.46 AM.png
1.5 MB View Download
Cc: pinkerton@chromium.org
Labels: -M-57 M-58
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).
Padding added. Going to refactor my code and then send it for a code review
Screen Shot 2017-02-10 at 1.53.49 PM.png
1.4 MB View Download
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?
Cc: rpop@chromium.org
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.
Awesome, I'll leave the flag as enabled then. Also sorry, here's the correct Touch Bar  :)
Screen Shot 2017-02-10 at 2.16.50 PM.png
1.2 MB View Download
Components: -UI>Browser>Core UI>Browser>Touchbar
Project Member

Comment 39 by bugdroid1@chromium.org, 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

Blockedon: 694760
Project Member

Comment 41 by bugdroid1@chromium.org, 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

Status: Fixed (was: Assigned)

Sign in to add a comment