1522012 - Enable built-in macOS touch bar customization support

Status: RESOLVED FIXED

(Core :: Widget: Cocoa, enhancement, P3)

Description

[Tim Nguyen :ntim]

So it's apparently a one-liner, but it crashes Firefox when :harry tested it a while ago. Might be different now, worth investigating here :)

Comment 1

[Tim Nguyen :ntim]

Hey Harry, do you think you could post what you tried ?

Comment 2

[Harry Twyford]

Hi Tim, there's an attachment bug 1313429 that contained some WIP code for scrubbers and Apple's customization palette. Link: https://bug1313429.bmoattachments.org/attachment.cgi?id=8987983

The above patch mostly changed /widget/cocoa/nsMenuBarX.mm to add the "Customize Touch Bar..." menu item using [NSApp toggleTouchBarCustomizationPalette:sender]; Apple's one-liner put the "Customize Touch Bar..." menu item beneath "Quit Firefox" in the app menu, which was a bit weird, so I placed it manually. That patch also sets self.customizationIdentifier in nsTouchBar.mm, which is necessary to use the customization menu.

My best guess as to why the customization palette crashes Firefox (after enlisting some help from :mconley and :mstange) was that the palette was attaching itself to Firefox's hidden window. When one left the customization palette, there was some freakout in Gecko about the hidden window and the main window not having the same properties, causing the crash. Again, that's just a guess.

One more issue blocking using the use of the customization palette is how the Touch Bar patch ended up being designed after this roadblock. Currently, the Cocoa code initializes the Touch Bar. nsTouchBar.mm asks MacTouchBar.js for the layout, then fills out the Touch Bar using that layout. The Cocoa code has no idea what the full possible set of inputs is: only the front-end does.

In order to use Apple's customization palette, every input needs to be predefined in the Cocoa code, then the use of some magic variables dictates which ones are the default set, and which ones appear in the customization menu. If we got this customization palette to work, then get layout() in MacTouchBar.js would have to send back every available input, then nsTouchBar.mm would do the filtering on which ones appeared. This would make adding a new input slightly less trivial, since it would have to be added to kImplementedInputs in MacTouchBar.js, then specified in nsTouchBar.mm if is part of the default set or the customization set. Either that, or we take out the front-end part of the Touch Bar code entirely...

Apple's docs on Touch Bar customization are here: https://developer.apple.com/documentation/appkit/nstouchbar?language=objc#2587638

I was thinking that if the customization palette continues to be incompatible with Gecko, then we might be able to achieve in-house developed customization in the "Customize..." menu?

Comment 3

[Harry Twyford]

Attachment: Bug 1522012 - Implement Touch Bar's native customization window and remove ui.touchbar.layout preference. r?spohl,mikedeboer

This patch also fixes the Home and Sidebar Touch Bar buttons, since using them after customizing showed that they no longer worked.

Comment 4

[Harry Twyford]

Attachment: readd_touchbar_pref.diff

I decided to remove the ui.touchbar.layout pref, since it was originally introduced as a workaround for not having proper customization. Furthermore, I think keeping it would introduce confusing behaviour: it would no longer set the layout but rather just the default layout. Any users that use this new native customization window would find that ui.touchbar.layout no longer did anything, since they have a custom layout that overrides the default.

Removing the pref will cause a one-time reset to the default for users' Touch Bars. I don't foresee this being a major issue, since telemetry shows that virtually nobody uses any Touch Bar inputs but the defaults (i.e., they do not customize).

I've attached a diff that re-introduces the ui.touchbar.layout pref, so we have it if there's strong disagreement with removing ui.touchbar.layout.

Comment 5

[Harry Twyford]

Attachment: touchbar_customization.png

To be clear, if we kept ui.touchbar.layout, we would retain the ability to modify the Default Set in this screenshot via pref experiments. I don't see huge value in that, but I thought I'd post here to see if anyone disagrees.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c1b80824ae88
Implement Touch Bar's native customization window and remove ui.touchbar.layout preference. r=spohl,mikedeboer,fluent-reviewers,Pike

Comment 7

[Andreea Pavel [:apavel]]

Backed out for failing bc at browser_touchbar_tests.js

Push that started the failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&selectedJob=254477816&resultStatus=testfailed%2Cbusted%2Cexception&revision=c1b80824ae8885c5caef8401813da38d500f853b

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=254477816&repo=autoland&lineNumber=968

Backout: https://hg.mozilla.org/integration/autoland/rev/4f8cabb3ff700627753700e83075953e9230bbfc

Comment 8

[Harry Twyford]

spohl, can you please take a look at the new revision I posted for review?

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/3e2fe70b181a
Implement Touch Bar's native customization window and remove ui.touchbar.layout preference. r=spohl,mikedeboer,fluent-reviewers,Pike

Comment 10

[Alexandru Michis [:malexandru]]

Backed out changeset 3e2fe70b181a for causing failures in browser_touchbar_tests.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/2f763c73129de66f6230720b28851ed3ec60b621

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception&fromchange=3e2fe70b181ad24fac1251077b93e1e8040785cf&tochange=8b928b1224c7fe04f89d04170ec54b5d5c6ed61f&selectedJob=254769617

Failure log: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=254762799&repo=autoland&lineNumber=974

14:29:25 INFO - TEST-UNEXPECTED-FAIL | browser/components/touchbar/tests/browser/browser_touchbar_tests.js | AddBookmark image should be filled bookmark after event. - "bookmark.pdf" == "bookmark-filled.pdf" - JS frame :: chrome://mochitests/content/browser/browser/components/touchbar/tests/browser/browser_touchbar_tests.js :: updateBookmarkButton :: line 29

Comment 11

[Stephen A Pohl [:spohl] (OOO until 4/29)]

Harry, please make sure to run a try build before landing this again.

Comment 12

[Harry Twyford]

I've posted a new patch for review that passes try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac0b6573d74625e83cf766a252492d5487eca171

Comment 13

[Stephen A Pohl [:spohl] (OOO until 4/29)]

(In reply to Harry Twyford [:harry] from comment #12)

I've posted a new patch for review that passes try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ac0b6573d74625e83cf766a252492d5487eca171

Thanks! r+

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fbab0ad2a783
Implement Touch Bar's native customization window and remove ui.touchbar.layout preference. r=spohl,mikedeboer,fluent-reviewers,Pike

Comment 15

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/fbab0ad2a783