936273 - UITour: Allow page to specify which highlight animation to use

Status: RESOLVED FIXED

(Firefox :: General, defect, P3)

Description

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Currently, the highlight animation is chosen randomly. Would be nice for the page to be able to specify which animation to use.

Comment 1

[Blair McBride [:Unfocused] (UNAVAILABLE)]

I should note: This would only cover choosing which of the pre-set animations to use. It would not allow the page to define arbitrary animations.

Comment 2

[Matthew N. [:MattN]]

Attachment: v.1 Pass the effect through to the attribute, with tests

This depends on patches to bug 940902 and bug 936187.

Comment 3

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8339031 [details] [diff] [review]
v.1 Pass the effect through to the attribute, with tests

Review of attachment 8339031 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/modules/UITour.jsm
@@ +86,5 @@
>        case "showHighlight": {
>          let targetPromise = this.getTarget(window, data.target);
>          if (!targetPromise)
>            return false;
> +        targetPromise.then(target => this.showHighlight(target, data.effect));

Think the API should be more explicit here. Currently, if you specify an effect we don't support, it uses a random effect - which feels a bit too magic. I'd rather have the page specify "random" to get a random effect, and if it specified an effect we don't support it should default to "none".

Also, as a general rule, I've tried to do all input checking/sanitisation in this method here - keeping it all together so it's easier to audit.

Comment 4

[Matthew N. [:MattN]]

Attachment: v.2 Move validation to onPageEvent, default to "none" and update tests

(In reply to Blair McBride [:Unfocused] from comment #3)
> Comment on attachment 8339031 [details] [diff] [review]
> v.1 Pass the effect through to the attribute, with tests
>
> ::: browser/modules/UITour.jsm
> @@ +86,5 @@
> >        case "showHighlight": {
> >          let targetPromise = this.getTarget(window, data.target);
> >          if (!targetPromise)
> >            return false;
> > +        targetPromise.then(target => this.showHighlight(target, data.effect));
> 
> Think the API should be more explicit here. Currently, if you specify an
> effect we don't support, it uses a random effect - which feels a bit too
> magic. I'd rather have the page specify "random" to get a random effect, and
> if it specified an effect we don't support it should default to "none".

OK, I changed the default to "none".

> Also, as a general rule, I've tried to do all input checking/sanitisation in
> this method here - keeping it all together so it's easier to audit.

I originally did it in showHighlight so that we would validate the argument from callers other than a website but I suppose this new way allows a JSM consumer use a custom animation.

Comment 5

[Matthew N. [:MattN]]

Attachment: Pointer to Github pull request: https://github.com/Unfocused/mozilla-uitour/pull/2

Pointer to Github pull-request

Comment 6

[Matthew N. [:MattN]]

https://hg.mozilla.org/integration/fx-team/rev/166d27bf3b4b

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/166d27bf3b4b