1847939 - Race condition in feature callout tourend page events

Status: RESOLVED FIXED

(Firefox :: Messaging System, defect)

Description

[Shane Hughes [:aminomancer]]

On dismiss, callouts tear down their tourend page event listeners before the handlers can resolve. Pretty easy fix, just rearranging the order of some things.

I also want to fix another nagging issue here - the FOUC when callouts are loaded for the first time too early in startup. It's because we use a link element to load the stylesheet. There's not really a way to wait for it to truly apply to the content. A load event listener would tell you when it loads, but I guess not when it registers or applies to the content. I had to just use a different method of loading the stylesheet altogether.

Then finally, change the logic for picking an initialFocus target a bit. We should include radio buttons/checkboxes as eligible initial focus targets. That way, if the primary button is disabled (as it will be initially for survey screens, which start with no radios/checkboxes checked), we won't just focus the dismiss button (or a link), but instead focus the main element we want the user to interact with. In the survey case it makes a lot more sense to focus the first radio button than to focus the dismiss button, which we don't want them to click at all.

Comment 1

[Shane Hughes [:aminomancer]]

Attachment: Bug 1847939 - Fix a feature callout page event race condition. r=jprickett

Also fix the FOUC for newtab callouts, and refine the logic for picking
an element in the callout to initially focus.

Comment 2

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/27018b300692
Fix a feature callout page event race condition. r=jprickett

Comment 3

[Norisz Fay [:noriszfay]]

Backed out for causing bc failures on browser_feature_callout.js

Backout link

Push with failures

Failure log

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c857cfa4e594
Fix a feature callout page event race condition. r=jprickett

Comment 5

[Cristian Tuns]

https://hg.mozilla.org/mozilla-central/rev/c857cfa4e594

Comment 6

[Shane Hughes [:aminomancer]]

Comment on attachment 9348167 [details]
Bug 1847939 - Fix a feature callout page event race condition. r=jprickett

Beta/Release Uplift Approval Request

• User impact if declined: We'll need to wait until 118 to run a micro survey experiment planned for 117.
• Is this code covered by automated tests?: Yes
• Has the fix been verified in Nightly?: Yes
• Needs manual test from QE?: No
• If yes, steps to reproduce:
• List of other uplifts needed: None
• Risk to taking this patch: Low
• Why is the change risky/not risky? (and alternatives if risky): The changed code is in the FeatureCallout component, which is unused in 117. For 117 and 118, it will only be enabled by a Nimbus experiment, for which the exposed population is expected to be perhaps 3000. So the affected audience is very small. The code changes are also relatively minor and shouldn't cause issues, since we uplifted all the previous Nightly changes to this file into 117 beta as well.
• String changes made/needed:
• Is Android affected?: No

Comment 7

[Donal Meehan [:dmeehan]]

Comment on attachment 9348167 [details]
Bug 1847939 - Fix a feature callout page event race condition. r=jprickett

Approved for 117.0b7

Comment 8

[Pulsebot]

https://hg.mozilla.org/releases/mozilla-beta/rev/19a38682aee8