1591839 - Touchbar labels/icons missing some of the time

Status: VERIFIED FIXED

(Core :: Widget: Cocoa, defect, P1)

Description

[:Gijs (he/him)]

The icon is visible for some windows but not others. The button is there, it just doesn't have an icon. Pressing it still works (ie offers sharing options), but it obviously looks sloppy.

I haven't worked out why this is happening. Seeing this on 71 beta. I'll keep it open for a bit in case there's debugging I can do from JS land. Harry or :ntim, what should I be looking for?

Comment 1

[Tim Nguyen :ntim]

If that's helpful, I've been able to reproduce this reliably using this STR:

1. Click on the "Customize touch bar..." menu item inside the "Firefox" menubar button
2. Drag the share button out of the set
3. Restart Firefox
4. Re-do step 1.

-> See the share button missing the icon

I'll see if I can get a regression range.

I haven't worked out why this is happening. Seeing this on 71 beta. I'll keep it open for a bit in case there's debugging I can do from JS land. Harry or :ntim, what should I be looking for?

FWIW, the share button is a special touch bar item, since it uses the "scrubber" type:
https://searchfox.org/mozilla-central/rev/74cc0f4dce444fe0757e2a6b8307d19e4d0e0212/browser/components/touchbar/MacTouchBar.js#164-172

I guess trying to spot the difference between the implementation of "scrubber" and other touchbar types could help, but Harry would be the better person to ask here since he's done pretty much all the work on the touchbar.

Comment 2

[Tim Nguyen :ntim]

My guess is that bug 1521893 regressed this, but I'll wait for mozregression to confirm.

Comment 3

[Tim Nguyen :ntim]

https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=480000073f4668def97fda2a50fef3e29225308e&tochange=7c3489dfee6082bec00f05d0f02f502ec4686743

Comment 4

[:Gijs (he/him)]

I'm also seeing this happen to the "Search in" options in the location bar. Confusingly, I'm currently typing this in a window that has the share icon present and correct, but when I focus the location bar all the "buttons"/items that appear in the touch bar are blank.

Comment 5

[Harry Twyford]

The Share scrubber is an Apple-made component that we put our own share icon on, so it behaves quite differently from the other Touch Bar inputs. It's NSSharingServicePickerTouchBarItem.

The strings missing on the "Search in" items might be an unrelated issue, since the icons and strings are set in different places/ways. I'll look into both these issues and file a separate bug for missing strings in "Search in" if necessary. Leaving needinfo so this is on my radar.

Comment 6

[Harry Twyford]

There are two issues here.

The first is a race condition. There is an incoming patch for this and I'll include a more detailed description of what the race is on the Phabricator page. This patch should solve the missing strings in the "Search in..." item, among other intermittent Touch Bar issues. In my own testing it also fixes the missing search icon although that might be a false positive. There are more icon-related issues:

The second issue is that 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 and strings will be missing.
   (NB: If you open the customization window again in the same Firefox window, the icons will now be showing).

The patch for the race condition fixes the missing strings, but the missing icons seem to be an unrelated issue. 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?

Since these are two separate issues I'm going to post the patch for the race and file a follow-up.

Comment 7

[Harry Twyford]

Attachment: Bug 1591839 - Close Touch Bar race condition. r?spohl!

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2c3613eaf377
Close Touch Bar race condition. r=spohl

Comment 9

[BugBot [:suhaib / :marco/ :calixte]]

The priority flag is not set for this bug.
:spohl, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 10

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/2c3613eaf377

Comment 11

[Harry Twyford]

Comment on attachment 9107514 [details]
Bug 1591839 - Close Touch Bar race condition. r?spohl!

Beta/Release Uplift Approval Request

• User impact if declined: Inconsistent and annoying behaviour in the Touch Bar:

• Labels sometimes missing in the "Search in" feature new to 71
• Buttons in the "Search in" feature sometimes do nothing
• Icon sometimes missing on Share button
• Labels missing in the Touch Bar customization view

• Is this code covered by automated tests?: No
• Has the fix been verified in Nightly?: No
• Needs manual test from QE?: Yes
• If yes, steps to reproduce: Unfortunately, most of the issues solved by this patch were intermittent with no clear STR. One issue is always reproducible, though: labels missing in the Touch Bar customization view. STR:

1. Open Firefox on a MacBook with Touch Bar.
2. Close the Firefox window.
3. Open a new Firefox window.
4. Open the Touch Bar customization window (Firefox > Customize Touch Bar... in the menu bar). If all strings are present, then the patch is working. NB: Some icons may be missing. This is tracked in bug 1595082 and is considered a separate issue.

Please report if you encounter any of the other issues listed above during your testing (missing labels/unresponsive buttons in the "Search in" Touch Bar shown when the address bar is focused; missing icon on the Share button).

• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): Low-medium. This patch has baked on Nightly for 3 days and no issues have been reported. The nature of the changes in this patch are very Touch Bar-specific, so any potential issues would only cause more unwanted behaviour in the Touch Bar (again, no such new unwanted behaviour has been reported).
  I labelled this as low-medium rather than low simply because the changes are in platform code and are thus a little more sensitive than front-end-only changes.
• String changes made/needed:

Comment 12

[Pascal Chevrel:pascalc]

Comment on attachment 9107514 [details]
Bug 1591839 - Close Touch Bar race condition. r?spohl!

That's not entirely risk-free but since this is a P1 and we have STR for QA, let's uplift into 71 beta 11 and verify the fix in beta, thanks.

Comment 13

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/152811e4837c

Comment 14

[Maria Berlinger [:mberlinger], Services QA]

I’ve managed to reproduce this issue on Firefox 71.0b10 following the str from Comment 6, this is verified fixed using Firefox 72.0a1 (2019-11-18) on macOS 10.15.

Comment 15

[Maria Berlinger [:mberlinger], Services QA]

This is also verified fixed using Firefox 71.0b11 on macOS 10.15.

Comment 16

[:Gijs (he/him)]

(In reply to Maria Berlinger [:maria_berlinger], Release Desktop QA from comment #15)

This is also verified fixed using Firefox 71.0b11 on macOS 10.15.

I've just updated to b11 on 10.14 and I'm still seeing the share item having no icon after the restart (as a result of the update), for the first window that opened. Harry, what can I do to help debug?

Comment 17

[Harry Twyford]

A share-button-specific issue was fixed in bug 1595082 and I expect any remaining missing-icon issues will be fixed when that patch is uplifted to beta 12. Nonetheless, I'll take a look today to see if there's anything you can do to debug. Thanks.

Comment 18

[Harry Twyford]

Gijs, have you experienced this problem since beta 12 landed?

Comment 19

[:Gijs (he/him)]

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

Gijs, have you experienced this problem since beta 12 landed?

No, the share icon is OK now (though I can see it loading when I restart Firefox, ie first the button appears without an icon), though I now see the main urlbar control sometimes only have the magnifying glass (centered) without any text, and when focusing the location bar in that window, the labels for bookmarks/history/open tabs etc. are also missing. So this still seems somewhat broken.

Comment 20

[Harry Twyford]

Thanks. The labels missing in "Search in" is tracked in Bug 1596723. I'll note your observation of the label in the location bar missing there.