Closed Bug 1522012 Opened 1 year ago Closed 11 months ago

Enable built-in macOS touch bar customization support

Categories

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

enhancement
Points:
5

Tracking

()

RESOLVED FIXED
mozilla69
Iteration:
69.4 - Jun 24 - Jul 7
Tracking Status
firefox69 --- fixed

People

(Reporter: ntim, Assigned: harry)

References

(Blocks 2 open bugs)

Details

Attachments

(3 files)

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 :)

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

Flags: needinfo?(htwyford97)

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?

Flags: needinfo?(htwyford97)
Priority: -- → P3

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

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.

Assignee: nobody → htwyford
Status: NEW → ASSIGNED

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.

Pushed by htwyford@mozilla.com:
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

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

Flags: needinfo?(htwyford)
Iteration: --- → 69.4 - Jun 24 - Jul 7
Points: --- → 5
Pushed by htwyford@mozilla.com:
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

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

Flags: needinfo?(htwyford)

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

Flags: needinfo?(htwyford)

(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+

Pushed by htwyford@mozilla.com:
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
Status: ASSIGNED → RESOLVED
Closed: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69
Regressions: 1564868
Blocks: 1566728
Blocks: 1566729
You need to log in before you can comment on or make changes to this bug.