Closed Bug 1595082 Opened 4 months ago Closed 3 months ago

Unused Touch Bar inputs missing icons in customization window

Categories

(Core :: Widget: Cocoa, defect)

defect
Not set
Points:
3

Tracking

()

VERIFIED FIXED
mozilla72
Iteration:
72.3 - Nov 18 - Dec 1
Tracking Status
firefox-esr68 --- unaffected
firefox70 --- unaffected
firefox71 --- verified
firefox72 --- verified

People

(Reporter: harry, Assigned: harry)

References

(Regression)

Details

(Keywords: regression)

Attachments

(2 files)

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

Attached image missing_icons.png

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.

See Also: → 1595249

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?

Assignee: nobody → htwyford
Status: NEW → ASSIGNED
Duplicate of this bug: 1595249
Pushed by htwyford@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/23bffe0e868d
Ensure every Touch Bar input is updated at least once. r=mikedeboer,spohl
Pushed by htwyford@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/e050f35bd4c2
Ensure every Touch Bar input is updated at least once. r=mikedeboer,spohl
Status: ASSIGNED → RESOLVED
Closed: 3 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla72
Flags: needinfo?(htwyford)

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?

Flags: needinfo?(htwyford)

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:
Flags: needinfo?(htwyford)
Attachment #9109002 - Flags: approval-mozilla-beta?
Flags: qe-verify+
QA Whiteboard: [qa-triaged]

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.

Attachment #9109002 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

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

Status: RESOLVED → VERIFIED
Flags: qe-verify+
Iteration: --- → 72.3 - Nov 18 - Dec 1
Points: --- → 3
You need to log in before you can comment on or make changes to this bug.