search

mozilla / addon-recommendation-shield-study

Stand-alone verison of Add-on Recommendation for Shield Study
Mozilla Public License 2.0
3 stars 7 forks source link

Refactoring of onboarding code (better fix for #91) #92

Closed casebenton closed 8 years ago

casebenton commented 8 years ago

There are several methods that rely on the simple pref that marks the onboarding as complete. #91 alters the way the simple pref is used, which causes several issues. This commit fixes the issue that #91 fixes, but in a way which does not break the methods that depend specific usage of the onboarding pref.

The key change is that the tabs event listener that calls the endOnboard method is now the onOpen listener, rather than the onReady. onOpen fires immediately when the tab opens, while onReady fires once the page is completely loaded. Use of onReady contributed to the repeated onboarding events some users expirienced, as closing the tab before it loads completely would mean the onboarding complete pref was never set to true.

Testing using the same procedures as #91, namely simulating a 100% loss network on my computer and installing the addon on a fresh firefox build (to test closing the onboarding tab before it fully loads), showed that this commit fixes the issue of repeated onboarding events. Additionally, the methods that rely on the onboarding pref are not broken in this commit, as the usage of the pref is returned to the way it was previously.

@mythmon r?

mythmon commented 8 years ago

Can you elaborate on the issues caused by setting the pref immediately? I can't think of any realistic ways that this would break, but I'd like to better understand the problem this is fixing.

casebenton commented 8 years ago

The method that prepares the panel, including giving it the proper height, currently relies on this pref to accurately reflect whether or not onboarding has fully completed. If the pref is false or not set, then the method will add additional height to the panel for the onboarding message. If the pref is set to true, then the additional height will not be added.

Setting the pref immediately, meaning the pref indicating that onboarding had completed was set before the prepPanel method was called (and before onboarding actually completed), caused the prepPanel method to not increase panel height for the welcome message.

casebenton commented 8 years ago

Similar mechanics are used in the method that actually opens the panel when deciding if the panel should be opened automatically. Immediately setting the pref broke the onboarding event because the panel would not open automatically (unless users were on the auto-open branch).

casebenton commented 8 years ago

The big issue is that the pref is used to indicate that onboarding has completed. There are many places that use this pref to determine if special onboarding actions should be executed based on the fact that the onboarding event has not yet occured. Marking the onboarding as complete before onboarding is complete, like #91 does, breaks a lot of stuff.

mythmon commented 8 years ago

Thanks for the explanation, that makes sense. I wish there was a way to have dynamically sized panels, but I know that isn't reasonable with the current API.