STR:

1. Have Firefox Sync configured (so that your Firefox View "Tab Pickup" section will be populated)
2. Trigger the Firefox View Tour (by e.g. resetting browser.firefox-view.feature-tour to its initial value and then visiting a Firefox View tab)
3. Click through the tour, carefully watching your "Tab Pickup" section.

ACTUAL RESULTS:
Whenever a tour popup fades out of view, the Tab Pickup favicon images disappear, and the "Last Active" text disappears (with the "Last Active" badge collapsing to be a little contentless circle instead of a wide badge)

EXPECTED RESULTS:
No such layout shift / blinking.

Looking at a performance profile of the tour, it looks like we get a call to getSyncedTabData every time you advance between tour "slides".

That seems unexpected. And that's probably repopulating this section from scratch.

So there are two distinct issues here:
(1) We're unnecessarily forcing this section to refresh itself between tour "slides". That seems wasteful and unnecessary.
(2) When this section refreshes itself (including if its content doesn't change), it causes this flash. Presumably this happens regardless of the tour (but probably it's rare, aside from the scenario described here)

Focusing just on issue (1)... I suspect the relevant line that's making this get called is the last one here:

class TabPickupList extends HTMLElement {
  constructor() {
    super();
    this.maxTabsLength = 3;
    this.boundObserve = (...args) => this.getSyncedTabData(...args);

https://searchfox.org/mozilla-central/rev/0948667bc62415d48abff27e1405fb4ab4d65d75/browser/components/firefoxview/tab-pickup-list.mjs#31

It looks like that boundObserve assignment is setting getSyncedTabData as some sort of observer callback, and something about the messaging tour is triggering it at every advance.

That observer was added in bug 1774169; setting some dependencies and needinfo's based on that. Sarah / Gijs, maybe you could take a look here?

(In reply to Daniel Holbert [:dholbert] from comment #3)

So there are two distinct issues here: [...]
(2) When this section refreshes itself (including if its content doesn't change), it causes this flash. Presumably this happens regardless of the tour (but probably it's rare, aside from the scenario described here)

I spun this part off as bug 1791098.

Emily/Shane: Does the tour alter the DOM that contains Firefox View's elements, and is that responsible for the flashing here?

(In reply to Daniel Holbert [:dholbert] from comment #4)

Focusing just on issue (1)... I suspect the relevant line that's making this get called is the last one here:

class TabPickupList extends HTMLElement {
  constructor() {
    super();
    this.maxTabsLength = 3;
    this.boundObserve = (...args) => this.getSyncedTabData(...args);

https://searchfox.org/mozilla-central/rev/0948667bc62415d48abff27e1405fb4ab4d65d75/browser/components/firefoxview/tab-pickup-list.mjs#31

It looks like that boundObserve assignment is setting getSyncedTabData as some sort of observer callback, and something about the messaging tour is triggering it at every advance.

That observer was added in bug 1774169; setting some dependencies and needinfo's based on that. Sarah / Gijs, maybe you could take a look here?

I'm confused - that assignment assigns a function into that property. There's no setter for that property, so this shouldn't result in anything getting called. What's the full stack of the call to getSyncedTabData when you see this happen?

(FWIW if I had to guess I'd rather think the connectedCallback at https://searchfox.org/mozilla-central/rev/0948667bc62415d48abff27e1405fb4ab4d65d75/browser/components/firefoxview/tab-pickup-list.mjs#67 is the culprit here, if the tour is messing with the DOM.)

The feature callout is just appended to the body, but the callout's "parent" element (which is specified by the onboarding messages) has its aria-owns attribute changed.

Could also have something to do with us loading React/ReactDOM.

Anyone know if there's a decent shortcut for manually testing the tab pickup card? I was thinking of trying to crib from browser_tab_pickup_list.js but figured someone's already set up something like that.

(In reply to :Gijs (he/him) from comment #7)

What's the full stack of the call to getSyncedTabData when you see this happen?

Here's a screenshot from the profiler with the full backtrace.

Here's the profile itself, where the previous comment's screenshot is taken from:
https://share.firefox.dev/3xqcOq8

(This perma-link shows the Call Tree for the getSyncedTabData calls. If you look at the Parent Process track, you can see there are three moments in time where those 3 calls (captured as 6 samples) occur -- just after t=2s, t=4s, and t=6s. Those are all moments when a tour popup is just disappearing.)

(In reply to :Gijs (he/him) from comment #8)

(FWIW if I had to guess I'd rather think the connectedCallback at [..] is the culprit here, if the tour is messing with the DOM.)

That was my first guess too, but I put a breakpoint there as well as inside of getSyncedTabData and then performed my STR, and I hit the getSyncedTabData breakpoint but not the connectedCallback breakpoint. And when I'm inside of getSyncedTabData, our JS-debugger devtools backtrace shows boundObserve as being my caller, and it points back to that this.boundObserve = assignment that I linked to / quoted above. See attached screencast.

(You should be able to test this yourself by just visiting about:view in Firefox with Sync enabled, and open devtools JS Debugger and do "find in all files" for getSyncedTabData, put a breakpoint inside of it, and then performing my STR [reset the tour pref is usually sufficient to immediately trigger a tour]. You might get a spurious getSyncedTabData call associated with the tab-switch or other background activity, so you may need to ignore some breakpoint-firings and only pay attention to the ones that happen when you click through the tour popups.

I actually couldn't reproduce the issue on either the current Nightly or a local build. I was only seeing the expected calls, once when the element is inserted on load (from connectedCallback) and again each time I sync, e.g. by going to the FxA subview in the app menu (from the notification observer callback). Interacting with the feature callout didn't seem to have any effect.

Then I tried changing my sync settings to include "Settings" and I could finally reproduce the issue. Unfortunately, I lost a ton of prefs. Super annoying that when you enable syncing settings on one instance, it automatically enables it in the other because the sync settings are apparently themselves synced. But therein seems to lie the problem... do you have this pref set, Daniel? services.sync.prefs.sync-seen.browser.firefox-view.feature-tour

I'm guessing that's our issue. At least for me, it's apparently syncing the feature tour pref, and leaving that pref as evidence. At one point, since I was syncing a local build with my normal profile, both running on the same computer, I changed the feature tour pref manually in about:config and less than a second later, it reverted back to the value from the other profile. I'm convinced that, since interacting with the feature callout changes prefs (the feature tour pref that persists the tour's state, plus impressions), that is causing a sync, and the sync service is notifying Firefox View to gather the tabs again. That might not be exactly right but that seems to be the gist. If you disable "Settings" in the "what to sync" subdialog, I think you'll be able to confirm that this bug goes away.

If I understand correctly, the underlying issue seems to be, why are we firing services.sync.tabs.changed when tabs haven't necessarily changed? We seem to be basically translating one topic into another, but these topics are not the same. One seems to fire on every sync and the other should only fire when tabs changed. There's got to be a way to compare the last value to the current value before dispatching that seemingly tabs-specific notification.

It seems that we just sync way less often when we're not syncing prefs. Apparently when syncing prefs is enabled, changing prefs can immediately trigger syncing. At least if we're actively using 2 profiles at the same time - I didn't try syncing prefs while only actively using one "device" so that might matter somehow.

But either way, it seems like we would still want to avoid triggering a sync when this particular pref is changed, just in light of the fact that it's basically a UI state. I'm not sure if this pref is intended to be synced. At least, we don't seem to be explicitly setting services.sync.prefs.sync.browser.firefox-view.feature-tour, it just seems to be getting set automatically. I think Meg might know more about that, I vaguely remember some recent discussion about syncing messaging system prefs?

(In reply to Shane Hughes [:aminomancer] from comment #14)

Then I tried changing my sync settings to include "Settings" and I could finally reproduce the issue. Unfortunately, I lost a ton of prefs.

Good find! I do indeed have Sync set up to synchronize Settings. (Sorry for the dataloss. :-/ )

do you have this pref set, Daniel? services.sync.prefs.sync-seen.browser.firefox-view.feature-tour

That pref does indeed seem to be set to true for me, yes.

(In reply to Shane Hughes [:aminomancer] from comment #14)

But either way, it seems like we would still want to avoid triggering a sync when this particular pref is changed, just in light of the fact that it's basically a UI state. I'm not sure if this pref is intended to be synced. At least, we don't seem to be explicitly setting services.sync.prefs.sync.browser.firefox-view.feature-tour, it just seems to be getting set automatically. I think Meg might know more about that, I vaguely remember some recent discussion about syncing messaging system prefs?

Note that this^ pref is different from the one that you asked me about earlier in your comment -- sync-seen vs sync.

The sync one (the one in my quote of your comment here) is set explicitly in firefox.js as of bug 1790669, with the intent of avoiding replaying the tour on each device (and was spun off in response to my question in bug 1790321 comment 5 I think)

(In reply to Daniel Holbert [:dholbert] from comment #15)

(In reply to Shane Hughes [:aminomancer] from comment #14)

Then I tried changing my sync settings to include "Settings" and I could finally reproduce the issue. Unfortunately, I lost a ton of prefs.

Good find! I do indeed have Sync set up to synchronize Settings. (Sorry for the dataloss. :-/ )

do you have this pref set, Daniel? services.sync.prefs.sync-seen.browser.firefox-view.feature-tour

That pref does indeed seem to be set to true for me, yes.

That's great! One step closer... as for the dataloss, turns out it's pretty fortunate that it saves those sync-seen prefs, since I was able to restore the prefs by just scanning for those 😂 it's a good lesson anyway, I should have been using user.js for the ones I really cared about.

I still find it really odd that it seems to sync prefs in reverse. Like, when I sign in on a new device (in this case, the local build) and enable settings syncing, you'd think it would download all the prefs from the profile that's existed on my account for years. But instead, it starts uploading prefs from the profile I just created five minutes ago and just logged into, and downloading them onto my well established main profile! I'm sure there must be a trick to it, but it sure would be nice if it asked you whether you're trying to sync up or sync down when you initially turn it on. Anyway, I'll definitely file a bug on that tomorrow.

(In reply to Daniel Holbert [:dholbert] from comment #16)

Note that this^ pref is different from the one that you asked me about earlier in your comment -- sync-seen vs sync.

The sync one is set explicitly in firefox.js as of bug 1790669, with the intent of avoiding replaying the tour on each device (and was spun off in response to bug 1790321 comment 5 I think)

Ah, good find. Searchfox hadn't updated yet so I assumed it showed up in my settings on its own - I really panicked for a minute there haha. I suppose a workaround might be to create some kind of separate pref for syncing the completion state that would be updated less frequently. One that sort of mirrors the feature-tour pref but just acts as a check before starting or something.

Set release status flags based on info from the regressing bug 1790669

I'm still pretty confused about this bug. The boundObserve assignment is a red herring, and just the stack trace being weird - it's being called because there's an observer notification (which comment #14 suggests is services.sync.tabs.changed but is "translated from another topic" - but doesn't seem to specify which topic?), and the comments seem to loosely imply it's to do with sync being triggered by a settings change. Shane, can you fill in the blanks there?

The discussion about how pref sync works between new profiles / existing profiles feels like it's almost completely orthogonal and should probably get its own bug to discuss that with the sync team.

To fix this bug (separate from fixing bug 1791098) it would seem we'd want to ensure that the tour progressing (and changing prefs, which triggers a sync) shouldn't in and of itself lead to a services.sync.tabs.changed notification. I agree - do we know why that happens?

It looks like the topic gets notified irrespective of whether we actually got new/different tabs data. So one way of addressing this would be to avoid rebuilding the tab pickup list if it's identical to the list we already had. I'm not sure if that's really a useful thing to optimize for away from fixing bug 1791098.

Yes the assignment doesn't matter, I'm guessing that is just where Daniel put a breakpoint. As for translated from another topic, I just mean it's ultimately coming from a weave:engine:sync:finish observer. We seem to be on the same page - not knowing anything about the sync service, I figured this would only happen if the tabs actually changed, but instead it seems to happen on every sync as long as the sync includes tabs at all. So if a pref changes it will start a sync, the sync will finish, it will include tabs, so the services.sync.tabs.changed notification will be dispatched and Fx View will update the UI. Semantically it seems weird because of the notification name, but pragmatically I guess it makes sense, or we would be checking the tab data for deep equality on every sync. Fixing the fluent issue does sound like a much better solution if possible

(In reply to Daniel Holbert [:dholbert][mostly away until Oct 6] from comment #13)

And when I'm inside of getSyncedTabData, our JS-debugger devtools backtrace shows boundObserve as being my caller, and it points back to that this.boundObserve = assignment that I linked to / quoted above. See attached screencast.

Just to resolve this confusion - looking at the screencast, there are arrows inline next to the contents of the arrow function being assigned - so what it's showing is that the contents of boundObserve (which are just this.observe(...)) are being executed - not the assignment. It just so happens that it's all on one line, which leads to the confusion here. If it were:

this.boundObserve = () => {
  this.observe(...);
};

then only the middle line would be shown as a stack frame.

Fundamentally, if the tour requires saving a pref, and that automatically triggers a sync, and the sync is going to also include tabs, then I don't think there's much the tour can do to remediate what's happening here, so I'm going to morph this to just be about ignoring updates to tabs that don't change the tabs we're displaying.

(In reply to Shane Hughes [:aminomancer] from comment #14)

It seems that we just sync way less often when we're not syncing prefs. Apparently when syncing prefs is enabled, changing prefs can immediately trigger syncing. At least if we're actively using 2 profiles at the same time - I didn't try syncing prefs while only actively using one "device" so that might matter somehow.

But either way, it seems like we would still want to avoid triggering a sync when this particular pref is changed, just in light of the fact that it's basically a UI state. I'm not sure if this pref is intended to be synced. At least, we don't seem to be explicitly setting services.sync.prefs.sync.browser.firefox-view.feature-tour, it just seems to be getting set automatically. I think Meg might know more about that, I vaguely remember some recent discussion about syncing messaging system prefs?

Yes, bug 1790669 added services.sync.prefs.sync.browser.firefox-view.feature-tour to ensure a user's place in the tour is synced across devices.

Setting 107 to wontfix, looks like it's too late for the 107 cycle.
Leaving 108 status clear, unless anyone thinks we should be following this for a fix in 108?

SyncedTabs.jsm will notify on the services.sync.tabs.changed topic whenever tabs get synced. It observes "weave:engine:sync:finish", but should only proceed when it was tabs that got synced. I'm not sure if the bug is that we shouldn't be getting this callback here, i.e. data is "tabs" but shouldn't be, or if this is all expected behavior and its up to the services.sync.tabs.changed observers to figure out what if any delta there is instead of blindly re-acquiring the synced tabs data and re-rendering.

(In reply to Sam Foster [:sfoster] (he/him) from comment #24)

SyncedTabs.jsm will notify on the services.sync.tabs.changed topic whenever tabs get synced. It observes "weave:engine:sync:finish", but should only proceed when it was tabs that got synced. I'm not sure if the bug is that we shouldn't be getting this callback here, i.e. data is "tabs" but shouldn't be, or if this is all expected behavior and its up to the services.sync.tabs.changed observers to figure out what if any delta there is instead of blindly re-acquiring the synced tabs data and re-rendering.

I defer to Sammy/Mark but I expect it's the latter ie this is consumers' responsibility.

(In reply to :Gijs (he/him) from comment #25)

(In reply to Sam Foster [:sfoster] (he/him) from comment #24)

SyncedTabs.jsm will notify on the services.sync.tabs.changed topic whenever tabs get synced. It observes "weave:engine:sync:finish", but should only proceed when it was tabs that got synced. I'm not sure if the bug is that we shouldn't be getting this callback here, i.e. data is "tabs" but shouldn't be, or if this is all expected behavior and its up to the services.sync.tabs.changed observers to figure out what if any delta there is instead of blindly re-acquiring the synced tabs data and re-rendering.

I defer to Sammy/Mark but I expect it's the latter ie this is consumers' responsibility.

Yes, this is correct. The notification was mainly used for knowing when to render/rerender the list and then calling the respective SyncedTabs.jsm APIs but it doesn't keep track of deltas between syncs. However if this turns out to be complicated for the View to implement, we can certainly discuss having new APIs that can help alleviate some of the issues.

Any new updates on this one?

It seems like the solution should probably be what Gijs said in comment 21, which is to ignore updates to tabs that don't change the tabs we're displaying. This would also help with bug 1791098 but I think the solution there can go further and only ever add the badge once, since the first card should always be the "last active" synced tab regardless of whether the content has changed or not.

This update should also resolve bug 1791098

https://hg.mozilla.org/mozilla-central/rev/6424e727b6ad

Reproduced the issue on Firefox 106.0a1 (2022-09-15) on macOS 13.2.1 by following the STR from Comment 0 and the screencasts provided.

The issue is fixed on Firefox 111.0 and Firefox 112.0a1 (2023-03-07). Tests were performed on macOS 13.2.1, Windows 11 and Ubuntu 22.04.