971116 - UI Tour: Implement new UI highlight effect

Status: RESOLVED FIXED

(Firefox :: General, defect)

Description

[Michael Maslaney [:mmaslaney] ([e-mail])]

Attachment: Screen Shot 2014-02-11 at 1.31.14 PM.png

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.

Comment 1

[Matthew N. [:MattN]]

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

Comment 2

[Michael Maslaney [:mmaslaney] ([e-mail])]

This will replace the "zoom" effect.

Comment 3

[Blake Winton (:bwinton) (:☕️)]

Attachment: A first cut at the patch to animate the highlight.

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 4

[Matthew N. [:MattN]]

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.

Comment 5

[Blake Winton (:bwinton) (:☕️)]

(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!

Comment 6

[Blake Winton (:bwinton) (:☕️)]

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

Thanks,
Blake.

Comment 7

[Alex Gibson [:agibson]]

(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”?

Comment 8

[Blake Winton (:bwinton) (:☕️)]

"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…  :)

Comment 9

[Alex Gibson [:agibson]]

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.

Comment 10

[Blake Winton (:bwinton) (:☕️)]

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!

Comment 11

[Alex Gibson [:agibson]]

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)

Comment 12

[Blake Winton (:bwinton) (:☕️)]

"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.  :)

Comment 13

[Alex Gibson [:agibson]]

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?

Comment 14

[Blake Winton (:bwinton) (:☕️)]

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

Comment 15

[Alex Gibson [:agibson]]

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?

Comment 16

[Alex Gibson [:agibson]]

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?

Comment 17

[Blake Winton (:bwinton) (:☕️)]

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".

Comment 18

[Alex Gibson [:agibson]]

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

Comment 19

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(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 20

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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.

Comment 21

[Blake Winton (:bwinton) (:☕️)]

Attachment: The next version of the patch…

(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.

Comment 22

[Alex Gibson [:agibson]]

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.

Comment 23

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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 24

[Blair McBride [:Unfocused] (UNAVAILABLE)]

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%.

Comment 25

[Blake Winton (:bwinton) (:☕️)]

Attachment: The first final version of the patch.

(Carrying forward r=Unfocused.)

Nit fixed, and yes, since I'm not updating the zoom animation on purpose.  :)

Thanks!

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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

Comment 27

[Ryan VanderMeulen [:RyanVM]]

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

Comment 28

[Blake Winton (:bwinton) (:☕️)]

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!

Comment 29

[Blake Winton (:bwinton) (:☕️)]

Attachment: The next final version of the patch.

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

Comment 30

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8389167 [details] [diff] [review]
The next final version of the patch.

I'm off sick, so redirecting to Matt.

Comment 31

[Matthew N. [:MattN]]

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

Comment 32

[Blake Winton (:bwinton) (:☕️)]

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

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/187f55d9c2e5

Comment 34

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/187f55d9c2e5

Comment 35

[:Gijs (he/him)]

Should this be uplifted?

Comment 36

[Blake Winton (:bwinton) (:☕️)]

We definitely want it landed when Australis hits Beta!

Comment 37

[:Gijs (he/him)]

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

Comment 38

[Jared Wein [:jaws] (please needinfo? me)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/8b69399eb024

Comment 39

[Catalin Varga [QA][:VarCat]]

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