Fix #4183, make onboarding behaviour consistent

[testeaxeax]

Fix #4183, this makes the onboarding behaviour for unsupported websites consistent between the "Page actions" menu and the context menu.

[jaredhirsch]

Thanks for submitting this pull request! I can take a look

[jaredhirsch]

First off, thanks for opening this PR!

I think the discussion in the original bug was a little ambiguous. The guiding principle IMO is to make a new user’s first interaction with Screenshots as clear and informative as possible. It can be distracting to change the UI in two places at once, so I think it would be better to always take a new user to onboarding, and not show the ‘unshootable page’ error.

The better fix here would be to add a hasSeenOnboarding check inside onClickedContextMenu, then, if the user is new, open the onboarding tab; otherwise, do the urlEnabled check.

Does this make sense? I can provide more detailed feedback if that would be helpful.

[testeaxeax]

Updated the commit. Now it just opens a new tab with the onboarding page and starts onboarding without the error message.

[sevaan]

Thanks for your help testeaxeax! I think 6a68's description of the behaviour is correct: No error pop-up for users taking their first screenshot, just pop the new tab and launch the tour. If a screenshot has already been taken successfully previously, then revert to the error message and no popped tab.

[jaredhirsch]

Looking good! Could you rearrange the context menu flow a bit? I've added some notes. I think this should be good to land after that's done.

[jaredhirsch]

I think the flow here should be something like:

hasSeenOnboarding.then(onboarded => {
  if (!onboarded) { ... }
  if (!tab) { ... }
  if (!urlEnabled) { ... }
  // then, toggleSelector()...
})

This way, no matter how you first trigger screenshots, you'll see onboarding as a new user.

[testeaxeax]

Your code would always perform onboarding in a new tab even for normal websites. I don't think that that's what we want. What do you think?

[jaredhirsch]

Good point! Thanks for continuing to work on this, I've gotten a bit behind on reviews. I'll take a look today or Monday :-)

[jaredhirsch]

watchPromise manages error handling for Promise chains. Since you've added one at line 133 above, this one's no longer needed.

I think you'll also want to return toggleSelector... to keep the promises chained.

[testeaxeax]

I updated the commit. It opens a new tab if necessary, but not for normal websites.

[testeaxeax]

@6a68 I don't know how to fix the error in automated testing without breaking the promise chain for toggleSelector. Thanks in advance.

[jaredhirsch]

@testeaxeax Sorry for the delay in getting back to you.

I don't know how to fix the error in automated testing without breaking the promise chain for toggleSelector. Thanks in advance.

You don't need to return the promise. This code is called in one spot which doesn't further chain the response, and the catcher wrapper handles any returned errors.

[jaredhirsch]

This is good to land, once the linter error is fixed. Nice work 🎉

[jaredhirsch]

We talked this over in triage today. I'm going to land this as-is, and then I'll fix the linter error in a followup PR.