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

Issue 710100 link

Starred by 3 users

Issue metadata

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

Blocking:
issue 660126



Sign in to add a comment

Add support for customizing the Touch Bar

Project Member Reported by shrike@chromium.org, Apr 10 2017

Issue description

Apps supporting the Touch Bar on macOS have a View -> Customize Touch Bar... item that brings up the system Touch Bar config dialog. We should add this menu item and general support for customizing the items in the Touch Bar. This would allow a user, for example, to disable the Touch Bar in Chrome by removing all of the items that Chrome presents by default.

Marking this for M-59 even though branch point is just a few days away. spqchan@ - can you take a quick peek and see how much work this will be to implement. That way we can assess whether this change can ship in M59 or will have to wait until M60.
 
Status: Started (was: Assigned)
Looking into it
Cc: rpop@chromium.org bettes@chromium.org
So adding support for customizing the Touch Bar isn't hard at all. I just need to set one variable and then it'll support it right away. I attached a screenshot of the outcome

However, one of the reasons why we want to allow customization is to prevent users from accidentally hitting parts of the Touch Bar. They can customize the Touch Bar to prevent this, but if they remove a "problem" button, the remaining buttons will just slide over and replace it.

Safari fixes this issue by providing spacing as a touch bar item (see screenshot). We should do the same thing. What are your thoughts?
chrome_customize_tb.png
1.3 MB View Download
safari customize tb.png
1.3 MB View Download

Comment 3 by rpop@chromium.org, Apr 10 2017

It's weird that safari adds a spacer that the OS default set doesn't provide. Yes, we should have a space element too.

Comment 4 by shrike@chromium.org, Apr 10 2017

This customization reminds me of toolbars, and in that case you need to store the list of toolbar items in user defaults and load the list from defaults when you start up. Is that how the Touch Bar works as well? If so, it seems like in addition to setting the one variable and adding the spacer, you have to also add code to load/store the Touch Bar config in defaults?
AFAIK, you don't need to load/store a Touch Bar config

Comment 6 by shrike@chromium.org, Apr 10 2017

Right now, -[BrowserWindowTouchBar makeTouchBar] has a static list of Touch Bar items. That means if you customize the Touch Bar and then restart Chrome you'll lose your customizations.

When the user makes a change to the Touch Bar you have to write out the ids of the Touch Bar items to somewhere (user defaults, or Chrome synced prefs). On startup you have to read these prefs. If the prefs don't exist, you can assume the user has not customized the Touch Bar and so fallback on the static list.

This brings up an interesting problem, which is how to know whether or not to display the home button. Presumably the home button is one of the customize options. It seems like once the user customizes the touch bar, you can no longer show/hide the home button based on the home button being present in the toolbar (because you can't know if the user customized the toolbar and explicitly added/removed it).
Sorry, I should've gave more information in #5. I already tested it out, if you customize the Touch Bar and then restart Chrome, the customizations are still there.

I can add the home button as part of the customization dialog, regardless if it's showing up on the toolbar or not. That should solve the home button issue.

Comment 8 by shrike@chromium.org, Apr 10 2017

Got it. And yes, adding the home button to the customization dialog should solve the problem.
FYI, this breaks the build with the 10.12 SDK:

../../chrome/browser/ui/cocoa/browser_window_touch_bar.mm:237:32: error: 'NSTouchBarItemIdentifierFlexibleSpace' is only available on macOS 10_12_1 or newer [-Werror,-Wunguarded-availability]
  [customIdentifiers addObject:NSTouchBarItemIdentifierFlexibleSpace];
                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.12.sdk/System/Library/Frameworks/AppKit.framework/Headers/NSTouchBarItem.h:99:46: note: 'NSTouchBarItemIdentifierFlexibleSpace' has been explicitly marked partial here
APPKIT_EXTERN NSTouchBarItemIdentifier const NSTouchBarItemIdentifierFlexibleSpace NS_AVAILABLE_MAC(10_12_1);
                                             ^
../../chrome/browser/ui/cocoa/browser_window_touch_bar.mm:237:32: note: enclose 'NSTouchBarItemIdentifierFlexibleSpace' in an @available check to silence this warning
  [customIdentifiers addObject:NSTouchBarItemIdentifierFlexibleSpace];
                               ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
1 error generated.
ninja: build stopped: subcommand failed.
Will look into this
Project Member

Comment 12 by bugdroid1@chromium.org, Apr 19 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/4b18d5f5e02254c2f2088570e66c74db7eeb8e38

commit 4b18d5f5e02254c2f2088570e66c74db7eeb8e38
Author: spqchan <spqchan@chromium.org>
Date: Wed Apr 19 19:26:32 2017

Revert of [Mac] Support for Touch Bar Customization (patchset #8 id:180001 of https://codereview.chromium.org/2814683005/ )

Reason for revert:
Broke the 10.12 build

Original issue's description:
> [Mac] Support for Touch Bar Customization
>
> Implement customization support for the default touch
> bar. Changed the item identifiers to reverse-DNS style format.
>
> BUG= 710100 
>
> Review-Url: https://codereview.chromium.org/2814683005
> Cr-Commit-Position: refs/heads/master@{#464967}
> Committed: https://chromium.googlesource.com/chromium/src/+/1ad3c00f56fbf19ecba52bdf0591ca7ad1098d3a

TBR=rsesek@chromium.org
# Not skipping CQ checks because original CL landed more than 1 days ago.
BUG= 710100 

Review-Url: https://codereview.chromium.org/2831713002
Cr-Commit-Position: refs/heads/master@{#465705}

[modify] https://crrev.com/4b18d5f5e02254c2f2088570e66c74db7eeb8e38/base/mac/sdk_forward_declarations.h
[modify] https://crrev.com/4b18d5f5e02254c2f2088570e66c74db7eeb8e38/chrome/app/generated_resources.grd
[modify] https://crrev.com/4b18d5f5e02254c2f2088570e66c74db7eeb8e38/chrome/browser/app_controller_mac.mm
[modify] https://crrev.com/4b18d5f5e02254c2f2088570e66c74db7eeb8e38/chrome/browser/ui/cocoa/browser_window_touch_bar.mm

Project Member

Comment 14 by bugdroid1@chromium.org, Apr 21 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/86da744cba65de36f2119a03773cbd734864cc1c

commit 86da744cba65de36f2119a03773cbd734864cc1c
Author: alexis.menard <alexis.menard@intel.com>
Date: Fri Apr 21 21:04:50 2017

[Mac] Build fix when using newer SDK

https://codereview.chromium.org/2828993002 broke the build
on newer SDKs.

Make sure to put assume_nonnull end with the matching begin.

BUG= 710100 

Review-Url: https://codereview.chromium.org/2835693003
Cr-Commit-Position: refs/heads/master@{#466443}

[modify] https://crrev.com/86da744cba65de36f2119a03773cbd734864cc1c/ui/base/cocoa/touch_bar_forward_declarations.h

Status: Fixed (was: Started)

Sign in to add a comment