Closed Bug 1825381 Opened 3 years ago Closed 1 year ago

Move "Set to default" notification to Messaging System

Categories

(Firefox :: Messaging System, task, P1)

task
Points:
5

Tracking

()

RESOLVED FIXED
134 Branch
Iteration:
134.2 - Nov 11 - Nov 22
Tracking Status
firefox134 --- fixed

People

(Reporter: vtay, Assigned: hanna_a)

References

(Depends on 2 open bugs, Blocks 3 open bugs)

Details

Attachments

(6 files)

Move (or remove) "Set to default" notification messages to AS Router (there appears to be 2 - one is "primary" browser" and another is "set to default").

If/When we move them into AS Router, they should be part of a "set to default" group so that we can control collisions.

A couple of questions that will help us determine next steps:

  1. Might we have any data on the engagement with these notification messages?
  2. I believe this messaging component does not exist in AS Router - do we need it? When might we use it?
Attached image image (43).png

Comment on attachment 9325800 [details]
Screen Shot 2023-03-29 at 10.32.38 AM.png

"Make Firefox your primary browser"

Comment on attachment 9325801 [details]
image (43).png

"Make Firefox your default browser"

See Also: → 1825377

Just by virtue of converting these to ASRouter messages and making them trigger on defaultBrowserCheck will already prevent collisions, since there's only one trigger per startup, and we only route one message per trigger (the first matched by priority/weighting/insertion order). So the group may be unnecessary, though it still might be worth doing if we want to also prevent users from just seeing too many "set to default" affordances in general. That seems like maybe something to consider in its own bug though, as it affects many other messages, and poses the question of what to do with multi-screen messages that contain a "set to default" CTA (like about:welcome and the infobar in bug 1825377).

Also if we want to move these into ASRouter instead of removing them, we will need to do something about the strings too. Seems a bit overkill to insert them into spotlights (conditionally or not). I think we can use a Fluent migration to simply transfer them to onboarding.ftl

Edit: just realized we've been sending the wrong telemetry for this startup check since 2019:

isDefault = shellService.isDefaultBrowser(isStartupCheck, false);
Services.telemetry
  .getHistogramById("BROWSER_IS_USER_DEFAULT")
  .add(isDefault);

The first param isStartupCheck was removed in bug 1521902. So we're actually passing true for the first param, which is now aForAllTypes (what the 2nd param used to be, and which we meant to passing false for). Which is potentially skewing the data?

One other thing - this message uses the prompt service and we don't have a messaging surface for that. (It wouldn't be worth making one since we can do much more with extending Spotlight than investing in the old prompt service.) So there isn't a simple way to move this into our system with its visual appearance fully intact.

Spotlights are similar but the content layout is a little different - spotlight's images are more illustrations than little logo icons, and the content is centered instead of left aligned. CFRs have similar content layout, but they're more popups than dialogs, i.e. they don't block interaction outside of the popup, they don't draw a backdrop overlay on the window, and they're anchored to other elements.

I guess Spotlight is probably the closest fit since there's no obvious element to anchor it to (maybe the app menu hamburger button, but that places it way off to the right), but either way the end result is so different from the message as it currently exists that I'm not sure it's worth trying to replicate it, and maybe it's better to just make a new message if/when we want one.

Decision based on @vtay conversation this afternoon

Flags: needinfo?(vtay)
Priority: -- → P2
See Also: → 1826231
See Also: → 1826232
Depends on: 1828745

I found another message we might want to move into the messaging system. See code here.

@shane That looks like an easy migration to info bar?

I realised I dropped the call on the moving set-to-default messages over.
What is the next step? Use spotlights to mimic the current panel?

Flags: needinfo?(vtay)

(In reply to Vtay from comment #8)

I realised I dropped the call on the moving set-to-default messages over.
What is the next step? Use spotlights to mimic the current panel?

Not at all - we had a meeting about it and agreed that, for now, it isn't a critical issue because there is already a way to make sure our defaultBrowserCheck (startup) messages don't conflict with the hard-coded set-to-default message. We just have to add an extra attribute to the message targeting when we want to do a startup message.

As for next steps, I think this bug can be tackled by trying to reconstruct the message in Spotlight. There are other possibilities but I think we'll get the most long-term bang for our buck by adding whatever style and targeting features are needed to recreate the message (at maybe 90% accuracy) in Spotlight. It may be hard to get it to look exactly identical, but I figure you'd agree there's some wiggle room with respect to the visual style and some of the targeting nuances.

Right now, the most difficult part is probably that the set-to-default message uses pretty elaborate "targeting" rules. It doesn't use the messaging system for targeting, it just has some code that decides whether this message will show or not. Some of the parameters have existing targeting attributes, but at a glance it looks like some new targeting capabilities will need to be added (on train). But the engineer assigned to the bug can figure that out.


@shane That looks like an easy migration to info bar?

We can definitely make a visually identical message quite easily with InfoBar. But, as with the set-to-default message, the difficulty here is in recreating the complicated targeting in JEXL (the targeting format used by the messaging system). So it would be another thing an engineer will have to look at and most likely land some code changes.


Some notes for engineers...

These messages' maybeShow methods both read and set prefs, but it looks like those prefs are basically used to create one-off frequency caps. We should be able to remove those prefs if we can use frequency caps instead. Though we might want to do some kind of migration from pref values to ASRouter storage, so that users who already have seen the old hard-coded messages do not start seeing the new FxMS versions.

Other parameters look like they require new targeting attributes. See e.g. SessionStore.canRestoreLastSession and SessionStartup.sessionType == SessionStartup.RECOVER_SESSION

See Also: → 1876128
See Also: → 1911426
Assignee: nobody → halemu
Iteration: --- → 133.1 - Sep 30 - Oct 11
Points: --- → 5
Priority: P2 → P1
Duplicate of this bug: 1902046

In the event that:

  1. User has Firefox set to default
  2. Users unsets Firefox to default (by setting up another browser to default)
  3. User starts-up Firefox

Currently - user will not see the message.

We should change this and have the user sees the message again.

Note that checkbox corresponds to an about:setting preference "Always check if Firefox is your default browser"

Depends on: 1922646
Iteration: 133.1 - Sep 30 - Oct 11 → 133.2 - Oct 14 - Oct 25
Attachment #9431102 - Attachment description: WIP: Bug 1825381 - POC → Bug 1825381 - POC
Attachment #9431102 - Attachment description: Bug 1825381 - POC → Bug 1825381 - Create experimental set to defualt prompt-style spotlight
Attachment #9431102 - Attachment description: Bug 1825381 - Create experimental set to defualt prompt-style spotlight → Bug 1825381 - Create experimental set to default prompt-style spotlight and enable showing via Nimbus
Attachment #9431102 - Attachment description: Bug 1825381 - Create experimental set to default prompt-style spotlight and enable showing via Nimbus → Bug 1825381 - POC
Attachment #9431102 - Attachment description: Bug 1825381 - POC → Bug 1825381 - Create experimental set to default prompt-style spotlight and enable showing via Nimbus
Attachment #9431102 - Attachment description: Bug 1825381 - Create experimental set to default prompt-style spotlight and enable showing via Nimbus → WIP: Bug 1825381 - Create experimental set to default prompt-style spotlight and enable showing via Nimbus
Attachment #9431102 - Attachment description: WIP: Bug 1825381 - Create experimental set to default prompt-style spotlight and enable showing via Nimbus → Bug 1825381 - Create experimental set to default prompt-style spotlight and enable showing via Nimbus
Attachment #9431102 - Attachment description: Bug 1825381 - Create experimental set to default prompt-style spotlight and enable showing via Nimbus → WIP: Bug 1825381 - Create experimental set to default prompt-style spotlight and enable showing via Nimbus
See Also: → 1927593
Attachment #9431102 - Attachment description: WIP: Bug 1825381 - Create experimental set to default prompt-style spotlight and enable showing via Nimbus → Bug 1825381 - Create experimental set to default prompt-style spotlight and enable showing via Nimbus
Iteration: 133.2 - Oct 14 - Oct 25 → 134.1 - Oct 28 - Nov 8
Iteration: 134.1 - Oct 28 - Nov 8 → 134.2 - Nov 11 - Nov 22
Pushed by halemu@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/0e4d93673d0b Create experimental set to default prompt-style spotlight and enable showing via Nimbus r=mviar,omc-reviewers,firefox-desktop-core-reviewers ,mossop,aminomancer
Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → 134 Branch
Depends on: 1686284

Attaching a screenshot of the default prompt on Mac to highlight the OS-level difference in the position of the primary button (displayed to the right of the secondary action).

Depends on: 1950234
Depends on: 1966723
Blocks: 2021949
No longer blocks: 2021949
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: