1829210 - AboutWelcome progress bar doesn't grow from 0 width on first screen if targeting changed total screen count

Status: RESOLVED FIXED

(Firefox :: Messaging System, defect, P1)

Description

[Shane Hughes [:aminomancer]]

This is a followup to bug 1827572. Due to bad advice on my part, we only update previousOrder when the index changes, but not when screens change (relevant for totalNumberOfScreens). So it's possible for previousOrder to equal index which means it animates from, say, 15% to 15% instead of from 0% to 15%.

Comment 1

[Shane Hughes [:aminomancer]]

Attachment: image.png

It also affects what happens when navigating backwards from about:home to the last step. But that isn't fixed easily, that seems to be an issue with screen targeting in general.
This could end up being a pretty difficult fix, because there are 6 renders just on that first load.

You can see in the console, it starts out thinking it's supposed to animate from 0 to 1, then after screen targeting, it wants to animate from 1 to 4, then finally it realizes it's supposed to animate from 4 to 4 (i.e. the progress bar should start at the final position and remain there).

Comment 2

[Shane Hughes [:aminomancer]]

Pretty much everything needs to have screens as a dependency now, including all our one-time hooks. That's sort of the crux of the issue, our one-time hooks are executing before screen targeting, and there's no neat way to delay them till after screen targeting, because they necessarily have 0 hook dependencies. So we kinda have to do the same thing we did to prevent telemetry on the first (unfiltered) set of screens. We need to delay everything till didFilter.current == true. But then we also need a ref that tells us whether our "one-time" hooks already ran, so we can skip running and preserve their "one-time"-ness.

For that reason, I think it might make sense to do screen targeting outside of MultiStageAboutWelcome. IOW, the screen targeting should block all of the complexity in that component. That way we're not performing any operations on the unfiltered list of screens that only exists for a few ms at most, or using them to render anything. All of the filtering would be abstracted away from the telemetry, animations, etc., which would just be able to assume that screens always either 1) represents the filtered list of screens, or 2) represents the default, original screens, because screen filtering somehow failed.

Comment 3

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

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

:pdahiya, since you are the author of the regressor, bug 1827572, could you take a look? Also, could you set the severity field?

For more information, please visit auto_nag documentation.

Comment 4

[Punam Dahiya [:pdahiya]]

Discussed with @aminomancer over slack, this is a cosmetic fix ok to ride train. It should not be a blocker for Fx113 and not block uplift of https://bugzilla.mozilla.org/show_bug.cgi?id=1827572 Thanks

Comment 5

[Shane Hughes [:aminomancer]]

S3 seems fine, thanks!

I have an idea for merging the screen targeting with the language mismatch filtering, which I think will resolve the index issues with an added bonus of simplifying all the hooks in MultiStageAboutWelcome. Moving all screen filtering behaviors up one level to abstract it from the MultiStageAboutWelcome. Kinda structured like this:

<AboutWelcome defaultScreens={defaultScreens}>
  <ScreenTargetingFilteredAboutWelcome filteredScreens={filter(defaultScreens)}>
    <LanguageFilteredAboutWelcome languageFilteredScreens={filter(filteredScreens)}>
      <MultiStageAboutWelcome screens={languageFilteredScreens} />
    </LanguageFilteredAboutWelcome>
  </ScreenTargetingFilteredAboutWelcome>
</AboutWelcome>

That way MSAW only renders with the final result of filtering (i.e. after both screen targeting AND language filtering), and LFAW only does language filtering for the result of screen targeting. This should resolve all our index/order problems in one fell swoop, though it's a bit of a refactor, it probably requires many test changes, and the remaining problems are only visual (I think), so we shouldn't bother trying to squeeze it into 113.

Comment 6

[Shane Hughes [:aminomancer]]

Attachment: Bug 1829210 - Filter screens by targeting before rendering AboutWelcome. r=#omc-reviewers

Comment 7

[Punam Dahiya [:pdahiya]]

Should be addressed with fix of https://bugzilla.mozilla.org/show_bug.cgi?id=1823779

Comment 8

[Punam Dahiya [:pdahiya]]

Fixed with https://bugzilla.mozilla.org/show_bug.cgi?id=1823779