An example of the desired animation can be found here, on frame 5: http://tobiasahlin.com/spinkit/ The UX team feels that using this animation (a little slower than the example) would better catch the user's attention during the tour.

Will this replace the existing "zoom" effect or be added as a new effect?

This will replace the "zoom" effect.

So, after talking with Madhava, this is more or less what we've come up with in terms of styling. There are a couple of changes I might suggest, but I'm not sure how to get them in, since they're coming from the webpage. 1) I'm hard-coding a default of "wobble", but that should really be coming from the page. 2) We don't want the appMenu to be highlit until the user hits "Let's go". Thanks, Blake.

Comment on attachment 8387822 [details] [diff] [review] A first cut at the patch to animate the highlight. Review of attachment 8387822 [details] [diff] [review]: ----------------------------------------------------------------- I haven't looked at the CSS changes yet but I think some of the JS changes should be left out unless there is a good reason for them. ::: browser/modules/UITour.jsm @@ +69,5 @@ > }, > widgetName: "PanelUI-fxa-status", > }], > ["addons", {query: "#add-ons-button"}], > + ["appMenu", {query: "#PanelUI-button"}], This seems like an unrelated change that would be better in a separate bug. It's not clear why this needs to change as it was done how it is for a reason. @@ +276,5 @@ > let effect = undefined; > if (this.highlightEffects.indexOf(data.effect) !== -1) { > effect = data.effect; > + } else { > + effect = "wobble"; I'm not sure we want to change the default since the page can already change this. If we did want to change the default, you should do so in the argument definition of showHighlight: 'aEffect = "none"' @@ +816,5 @@ > let maxDimension = Math.max(highlightHeight, highlightWidth); > > // If the dimensions are within 40% of eachother, make the highlight a circle with the > // largest dimension as the diameter. > + highlightHeight = highlightWidth = maxDimension; This won't work for wide widgets like the urlbar and search targets.

(In reply to Matthew N. [:MattN] from comment #4) > I haven't looked at the CSS changes yet but I think some of the JS changes > should be left out unless there is a good reason for them. Well, I _try_ not to change things randomly… ;) > ::: browser/modules/UITour.jsm > @@ +69,5 @@ > > + ["appMenu", {query: "#PanelUI-button"}], > This seems like an unrelated change that would be better in a separate bug. > It's not clear why this needs to change as it was done how it is for a > reason. It's related because the PanelUI-menu-button now has a hover style which is larger than the PanelUI-menu-button itself. (The hover style takes up the entire PanelUI-button.) This hover style leads to the highlight getting lost in the visual noise, and only highlighting the inside of the button, which isn't what we're trying to do. > @@ +276,5 @@ > > if (this.highlightEffects.indexOf(data.effect) !== -1) { > > effect = data.effect; > > + } else { > > + effect = "wobble"; > I'm not sure we want to change the default since the page can already change > this. If we did want to change the default, you should do so in the argument > definition of showHighlight: 'aEffect = "none"' That _would_ be better, although I'm hoping that we can do this in the page instead, and this code can be removed. > @@ +816,5 @@ > > // If the dimensions are within 40% of eachother, make the highlight a circle with the > > // largest dimension as the diameter. > > + highlightHeight = highlightWidth = maxDimension; > This won't work for wide widgets like the urlbar and search targets. Do we highlight those? (We don't currently, as far as I can tell, but we might have plans to later.) Perhaps I should leave the dimension check in, and just make it less strict? Thanks for the comments!

Hey Alex, I hear you might be the person to make the website part of the changes listed in comment 3… Thanks, Blake.

(In reply to Blake Winton (:bwinton) from comment #3) > 1) I'm hard-coding a default of "wobble", but that should really be coming > from the page. > 2) We don't want the appMenu to be highlit until the user hits "Let's go". Hi Blake, I can take care of the web pages changes when this lands. Can I please clarify what I should be setting the highlight animation type to for the new effect, “zoom”?

"wobble", and is there any way you could not highlight the appMenu sooner than when this lands? (I mean, if you want to wait, I'm completely fine with that, but it's a change that we'ld like to make, and doesn't really depend on the in-product parts, so there's not a lot of benefit in waiting… :)

I can make a pull request for this first thing Monday morning and get it pushed live (probably) the same day - can you wait landing this until then? I can post a comment here when it's done.

Oh yeah, my best guess is that this isn't going to land until Tuesday, and really, I think I want to wait for your changes before landing it, to make sure I didn't mess anything up… :) Thanks!

Ok, so just to be clear I should only remove the initial app menu highlight first, then once this bug lands I can add the “wobble” animation type? Or us it safe for me to add “wobble” pre-emptively as well? (Can't remember off the top of my head if this is already an existing anim name)

"wobble" already exists, so you're safe to add it, too. (And if it didn't exist, it would turn into "none", which is what we currently have, so adding it wouldn't be a problem. :)

If “wobble” already exists then maybe I shouldn't be adding it pre-emptively, since would it then display an animation type we don't want to see in the tour until this bug lands?

Hmm… That could be. Did you want to add "spiral" (which doesn't exist), and I'll use that instead of "wobble"?

I'm fine to leave it as “wobble” but then only specify the anim type in the web page once this bug has landed, if that also works for you?

Just thinking, if we did use a different (new) name for this animation type, and didn't set it to default if nothing is specified, would we still have to remove the initial highlight? Could we then leave the current page as-is, then when this bug lands I can just use the new anim name where we need it?

I'm happy to leave the animation as "wobble", and change it in the page later. But we still want to remove the initial highlight, either now or when we change the animation to "wobble".

Ok, will just remove the initial highlight for now :)

(In reply to Blake Winton (:bwinton) from comment #5) > It's related because the PanelUI-menu-button now has a hover style which is > larger than the PanelUI-menu-button itself. (The hover style takes up the > entire PanelUI-button.) This hover style leads to the highlight getting > lost in the visual noise, and only highlighting the inside of the button, > which isn't what we're trying to do. Do you mean due to bug 969376, or something else? > > @@ +816,5 @@ > > > // If the dimensions are within 40% of eachother, make the highlight a circle with the > > > // largest dimension as the diameter. > > > + highlightHeight = highlightWidth = maxDimension; > > This won't work for wide widgets like the urlbar and search targets. > > Do we highlight those? (We don't currently, as far as I can tell, but we > might have plans to later.) > Perhaps I should leave the dimension check in, and just make it less strict? We support it, and there's plans to use it, yes. We explicitly don't want to regress that.

Comment on attachment 8387822 [details] [diff] [review] A first cut at the patch to animate the highlight. Review of attachment 8387822 [details] [diff] [review]: ----------------------------------------------------------------- And the other things already mentioned. ::: browser/base/content/browser.css @@ +997,5 @@ > + 50% { > + transform: rotate(360deg) translateX(3px) rotate(-360deg); > + } > + 51% { > + transform: rotate(0deg) translateX(3px) rotate(0deg); This makes it glitchy half way through.

(In reply to Blair McBride [:Unfocused] from comment #19) > > It's related because the PanelUI-menu-button now has a hover style which is > > larger than the PanelUI-menu-button itself. > Do you mean due to bug 969376, or something else? I think it might be bug 909349 that re-added the hover style… > We support it, and there's plans to use it, yes. We explicitly don't want to > regress that. Reverted, kinda. I've switched it to 2.1, so that the bookmarks combo-button gets a circle (as Madhava requested), but wider things won't. > > + 51% { > > + transform: rotate(0deg) translateX(3px) rotate(0deg); > This makes it glitchy half way through. Turns out I was thinking of iOS, which won't let you rotate more than 359º. In CSS, apparently you can use 720º, so that's what I've gone for. (I didn't see the glitch last time, though, so please let me know if this is still glitchy for you.) Thanks, Blake.

Hi Blake, I've submitted a PR which includes removing the initial highlight on the door hanger menu: https://github.com/mozilla/bedrock/pull/1765 I'll comment here when this goes to prod.

To avoid confusing our poor engineer brains, next time could you split these issues into separate bugs. This bug is really about updating two animation styles in Firefox. (In reply to Blake Winton (:bwinton) from comment #3) > 1) I'm hard-coding a default of "wobble", but that should really be coming > from the page. > 2) We don't want the appMenu to be highlit until the user hits "Let's go". And these are about updating the page, so should be filed under www.mozilla.org :: Pages & Content, with agibson CC'ed/needinfo'ed (see eg bug 981542).

Comment on attachment 8388195 [details] [diff] [review] The next version of the patch… Review of attachment 8388195 [details] [diff] [review]: ----------------------------------------------------------------- r+ assuming it was intentional that you're not longer updating the zoom effect. ::: browser/modules/UITour.jsm @@ +812,5 @@ > let maxDimension = Math.max(highlightHeight, highlightWidth); > > // If the dimensions are within 40% of eachother, make the highlight a circle with the > // largest dimension as the diameter. > + if (maxDimension / minDimension <= 2.1) { Update the comment above this, where it mentions 40%.

(Carrying forward r=Unfocused.) Nit fixed, and yes, since I'm not updating the zoom animation on purpose. :) Thanks!

Backed out for mochitest-bc failures. https://hg.mozilla.org/integration/fx-team/rev/a996c3c2f00d https://tbpl.mozilla.org/php/getParsedLog.php?id=35896720&tree=Fx-Team

Ah poop. Still, this looks like a pretty easy test-only fix, so I'll try to get to it later tonight. Thanks for the backout, Ryan!

Test fixed, and "mach mochitest-browser browser/modules" completely passes now.

Comment on attachment 8389167 [details] [diff] [review] The next final version of the patch. I'm off sick, so redirecting to Matt.

Comment on attachment 8389167 [details] [diff] [review] The next final version of the patch. I just independently noticed that test would fail from this patch while looking at this code today and came here to see the backout. :P

(Please update the commit message to r=MattN when committing…)

Should this be uplifted?

We definitely want it landed when Australis hits Beta!

Comment on attachment 8389167 [details] [diff] [review] The next final version of the patch. [Approval Request Comment] Bug caused by (feature/regressing bug #): Australis User impact if declined: no fancy highlight effect when using the tour Testing completed (on m-c, etc.): on m-c Risk to taking this patch (and alternatives if risky): low, mostly CSS changes, minor low-risk logic changes String or IDL/UUID changes made by this patch: none

Verified as fixed using the following environment: FF Beta 29.0b4 Build Id:20140331125246 OS: Win 7 x64, Ubuntu 13.04 x 64, Mac Os 10.8.5