1815708 - Onboarding doorhanger should persist like permissions panels

Status: NEW

(Firefox :: Messaging System, enhancement, P3)

Description

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

When the CBH onboarding doorhanger is shown, it hides if the user switches tabs. This bug is to investigate whether we can keep it open across tab switches, like the behavior of the protections panel, for the v111 release.

Comment 1

[[e-mail]]

I think this is valid but this isn't something the platform team can implement, Re-assigning to more appropriate team

Comment 2

[Shane Hughes [:aminomancer]]

This actually isn't a duplicate, and it's not fixed by persistent_doorhanger. I think this is actually a bug in PopupNotifications, because it says persistent "will always persist even across tab and app changes", yet it does not. The reason being that it calls _update on TabSelect events, and that method checks which notifications to show - based on the current browser. And since the notification is initialized for only one specific browser (the tab that was selected when the cookiebannerdetected event fired), that check will produce an empty array when the tab is changed. Which means persistent is a misnomer, it does not persist across tab changes.

Edit: Okay, I think I figured out what's going on. When it says "persistent," it doesn't mean that the panel will stay open. It means the panel will close but will reopen when the mapped browser is selected again. If you do this in the browser console, you can see what I mean:

PopupNotifications.show(
  gBrowser.selectedBrowser,
  "addon-install-confirmation",
  "firefox",
  gUnifiedExtensions.getPopupAnchorID(gBrowser.selectedBrowser, window),
  {
    label: gNavigatorBundle.getString("addonInstall.acceptButton2.label"),
    accessKey: gNavigatorBundle.getString(
      "addonInstall.acceptButton2.accesskey"
    ),
    callback: () => {},
  },
  [],
  {
    persistent: true,
    hideClose: true,
    popupOptions: {
      position: "bottomright topright",
    },
  }
)

But in this case, it's not getting reopened, because we remove the notification on location changes without checking if it's persistent.

So one issue is that we're basically making CFRs non-persistent on our own, by removing them on tab/location changes. And a simple fix for that would be to leave persistent notifications alone until they are removed by PageActions. Basically, we would not call PopupNotifications.remove, we'd let it remove notifications when they're dismissed, and remove them from the notification map when that happens.

But that doesn't really fix this issue, because comment 0 says these doorhangers should stay open across tab switches. So, we don't really want persistence as it's currently implemented in PopupNotifications. We want doorhangers that are simply never closed. But I think this is difficult to implement with the PopupNotifications system, because it stores notifications in a structure like Map<browser, notification>. I think if we want popups that aren't tied to any particular tab, it's probably better to do this with a different messaging surface.

Comment 3

[Shane Hughes [:aminomancer]]

By the way, I'm not sure what you mean by "like the permissions panels" or "like the behavior of the protections panel." The protections popup has its own dedicated panel but it also closes if you switch tabs. And iirc, the permissions panels use the same PopupNotifications system, and will also hide if you switch tabs but reappear if you switch back. It's easy to test this by opening the devtools in a tab (open to a remote page like google) and entering navigator.geolocation.getCurrentPosition(()=>{}) in the console.

Anyway, if we don't want these to be hidden when switching tabs, I'm not sure we already have a messaging surface that's suitable for this purpose. It's not practical to adapt PopupNotifications for this, and FeatureCallout actually has the same kind of implementation where it's updated when the tab switches. We have other surfaces like Spotlight that open window modal dialogs, but these prevent the user from interacting with the browser until the dialog is dismissed.

I think a new surface would need to be designed that functions similarly to AppMenuNotifications. Otherwise, we could fix the persistence issue with CFRPageActions, and just accept that these CFRs are going to hide when switching tabs but reappear when reselecting the original tab.

Comment 4

[Punam Dahiya [:pdahiya]]

Current CFR support persistent_doorhanger message property that prevents the doorhanger from closing when it loses focus that is prevent the doorhanger from being dismissed if user interacts with the page or switches between applications. It doesn't account for persisting across tabs.

https://searchfox.org/mozilla-central/rev/a3a9112d4d73d1323eabbc7faa9937cd9aae6465/browser/components/newtab/content-src/asrouter/templates/CFR/templates/ExtensionDoorhanger.schema.json#39

From #comment 2 above, it seems its possible to extend CFR persistent_doorhanger property (or create a new property persistent_doorhanger_tab ) to honor persistent property as it works for PopupNotifications (i.e message hides when you navigate away from the tab that the CFR was created for, but then comes back if you navigate back to that tab) @jhirsch can you please confirm if that's the intended enhancement desired as part of this bug thanks!

Comment 5

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Hey Punam - Yes, the original request from product was to make the doorhanger persistent in the same way as the permissions panel doorhanger, that is, it would reappear if the user switched tabs and switched back. Since we're shipping our experiment in 111, and 111 is close to the end of the beta cycle, I think this probably can't be fixed for the experiment we're working on--resetting the priority to P3 is probably a reasonable step here.

Comment 6

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Optimistically bumping to the next cookie banner handling metabug (targeting 112), but this wouldn't strictly block us from shipping in 112.