1595082 - Unused Touch Bar inputs missing icons in customization window

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect)

Description

[Harry Twyford]

+++ This bug was initially created as a clone of Bug #1591839 +++

Inputs not currently in the user's Touch Bar will be missing their icon in the Customization screen. STR:

1. Open Firefox.
2. Open the customization window (Firefox > Customize Touch Bar... in the menu bar). All icons should be present.
3. Close the customization window and close the Firefox window.
4. Open a new Firefox window.
5. Open the customization window again. Some icons will be missing.
   (NB: If you open the customization window again in the same Firefox window, the icons will now be showing).

I suspect the icon-issue is related to how we pass the document principal to imgLoader. Looks like the Touch Bar icon loader code gets all the way to loading the image with imgLoader->LoadImage and then it dies somewhere in there. Presumably one of the parameters is wrong; maybe the document is outdated?

Comment 1

[Harry Twyford]

Attachment: missing_icons.png

Comment 2

[Harry Twyford]

Reader View has an icon despite not being in my Touch Bar. This is likely because it is frequently updated, to indicate if a page can be Reader Viewed or not. A possible solution might be to just run an update on every Touch Bar input right before the customization window opens. We control when it opens with toggleTouchBarCustomizationPalette so it wouldn't be that hard to just put some kind of updateAll command right before.

Comment 3

[Harry Twyford]

Dragging the icons with no icon out of the customization window into the Touch Bar reveals that they do in fact have an icon: it's just not being shown in the customization window. What I think is happening is that the NSTouchBarDelegate defers initializing the inputs not in the Touch Bar until they are needed. That is, until they are about to be displayed in the customization view. nsTouchBarInputIcon is initialized for those inputs as the customization view opens. Since the nsIconLoaderService is async, the icon isn't loaded in time for the customization view opening, and it's displayed as blank, even though the underlying input has an icon. The customization window doesn't show "live" versions of the input, but rather snapshots of how they appear as the customization window opens.

Adding an updateAll command as the customization view is opening doesn't fix this issue either, presumably because that still isn't enough time for the icons to load. I think the icons are displayed for the first Firefox window opened because the label strings aren't yet localized, so we localize those strings and then update the inputs with the new strings. This update triggers loading the icon, even on those inputs not in the Touch Bar. On subsequent window opens, the strings are already localized. We cache the localized strings, so no update occurs, and thus the icons aren't loaded for non-visible inputs.

The Back, Forward, Add Bookmark, and Reader View inputs (should) always have icons because they are frequently updated for other reasons.

What is required to fix this is manually initializing every Touch Bar input, including those that aren't in the Touch Bar, whenever nsTouchBar is initialized. We might also be able to fix this from the JS side; we could just run _updateTouchBarInputs on any inputs that haven't already been updated. We could defer this a little so it doesn't block window startup time; maybe on the first location change?

Comment 4

[Harry Twyford]

Attachment: Bug 1595082 - Ensure every Touch Bar input is updated at least once. r?mikedeboer!,spohl

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/23bffe0e868d
Ensure every Touch Bar input is updated at least once. r=mikedeboer,spohl

Comment 7

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/86d5143e79c8
fixing ESlint. On a CLOSED TREE

Comment 8

[Dorel Luca [:dluca]]

Backed out 2 changesets (bug 1595082) for Browser-chrome failures on /touchbar/tests/browser/browser_touchbar_tests.js. CLOSED TREE

Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=277000176&repo=autoland&lineNumber=16623

Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&group_state=expanded&revision=23bffe0e868deab92467bb506d84d15b058c3052

Backout:
https://hg.mozilla.org/integration/autoland/rev/9ba48d874fb6c7cdde20410f451e8bdc03f6cbe2

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/e050f35bd4c2
Ensure every Touch Bar input is updated at least once. r=mikedeboer,spohl

Comment 10

[Razvan Maries]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

Is this something we should consider uplifting to Beta with one beta build left before next week's RC gets created or can this ride Fx72 to release?

Comment 12

[Harry Twyford]

Comment on attachment 9109002 [details]
Bug 1595082 - Ensure every Touch Bar input is updated at least once. r?mikedeboer!,spohl

Beta/Release Uplift Approval Request

• User impact if declined: Icons will sometimes be missing in the Touch Bar customization view.
• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: See comment 0.
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The changes are nearly all in the Touch Bar's JS code which is less risky than its ObjC platform code. There is one change to an ObjC file but it's pretty innocuous. The Touch Bar customization window is rarely used, so potential follow-up issues (none have been reported) would be very low-visibility.
• String changes made/needed:

Comment 13

[Pascal Chevrel (relman team) -> :pascalc|PTO until Sept 2]

Comment on attachment 9109002 [details]
Bug 1595082 - Ensure every Touch Bar input is updated at least once. r?mikedeboer!,spohl

low risk, macos only, let's take it in beta 12.

Comment 14

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/releases/mozilla-beta/rev/1568ccf67812

Comment 15

[Cristian Fogel [:cfogel]]

Confirmed issue with 71.0b11 on macOS 10.14.
Fix verified with 71.0b12, 72.0a1(2019-11-21).