958645 - Scaling effect in customization view doesn't work on some icons.

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

When grabbing items in the customization view, they usually get scaled up a little. This doesn't happen with a number of items:
- Email link
- Bookmarks
- Copy/Cut/Paste
- Zoom

and also with some add-ons
- Adblock Edge
- Thumbnail Zoom

It is quite subtle now, but it will become very apparent once bug 932947 gets fixed.

Comment 1

[u428464]

Bookmarks toolbar items can be added to the list.

This should be marked as an Australis priority.

Comment 2

[:Gijs (he/him)]

Bug 932947 is a P3, so I think we should do the same here. The current state doesn't warrant that, but after 932947 lands this will just look broken.

Comment 3

[:Gijs (he/him)]

So:

(In reply to Philipp Sackl [:phlsa] from comment #0)
> When grabbing items in the customization view, they usually get scaled up a
> little. This doesn't happen with a number of items:
> - Email link

This is dependent on bug 932235.

> - Bookmarks
> - Copy/Cut/Paste
> - Zoom

These could/should be fixed.

> and also with some add-ons
> - Adblock Edge
> - Thumbnail Zoom

I don't know that we could fix this, if they don't use straight toolbar buttons with icons. :-(

(background: there's toolbarbuttons and toolbaritems and weird things. Toolbarbuttons should normally work unless they have their own transform applied already (email link). Toolbaritems can be anything - the search bar is a toolbaritem, and we wouldn't want that to have that animation... So all we can really do is special-case here, for the zoom/copy/cut/paste buttons which are also inside a toolbaritem. Then there's weird menu-button-style things like the bookmarks button. I... don't know how much we can do there. I also don't know if fixing that might help with fixing the add-on case. We should investigate more.)

Comment 4

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

(In reply to :Gijs Kruitbosch from comment #3)

> the search bar is a toolbaritem, and we wouldn't want that to have that animation...
Why not? It is an element that can be grabbed and moved like the others, so it should also behave the same way. Or am I missing something here?

Comment 5

[:Gijs (he/him)]

(In reply to Philipp Sackl [:phlsa] from comment #4)
> (In reply to :Gijs Kruitbosch from comment #3)
> 
> > the search bar is a toolbaritem, and we wouldn't want that to have that animation...
> Why not? It is an element that can be grabbed and moved like the others, so
> it should also behave the same way. Or am I missing something here?

I would imagine this particular animation would look odd, especially if the item includes text inside it (e.g. the labels on cut/copy/paste or zoom in/reset/out). We can investigate once bug 932947 lands.

Comment 6

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #5)
> (In reply to Philipp Sackl [:phlsa] from comment #4)
> > (In reply to :Gijs Kruitbosch from comment #3)
> > 
> > > the search bar is a toolbaritem, and we wouldn't want that to have that animation...
> > Why not? It is an element that can be grabbed and moved like the others, so
> > it should also behave the same way. Or am I missing something here?
> 
> I would imagine this particular animation would look odd, especially if the
> item includes text inside it (e.g. the labels on cut/copy/paste or zoom
> in/reset/out). We can investigate once bug 932947 lands.

So I just checked, and especially the cut/copy/paste and zoom controls just look huge if we apply the same transition + transform there...

Comment 7

[:Gijs (he/him)]

Philipp, do you have other ideas here and/or can you do a mockup of what you would expect for the wide items?

Comment 8

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Do you have a build with that enabled that I can play with?
I think that would be the fastest way to find a solution.

Comment 9

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

OK, I just played with this for a while and I think this is a good way to go:
Keeping the same animation but reducing the scaling to 1.1 on wide items makes them look nice again. In addition, when the combined buttons get dragged, they should also get some background color so that they don't look so faint.

As I don't know any better way to communicate this, here's what I did:

/* panelUIOverlay.css */
[mousedown=true] #edit-controls:-moz-any(:not([cui-areatype="toolbar"]),.overflowedItem),
[mousedown=true] #zoom-controls:-moz-any(:not([cui-areatype="toolbar"]),.overflowedItem) {
    background: hsla(210,4%,90%,1);
    border-radius: 3px;
    border: 1px solid #ccc;
    transition-property: background-color, transform;
    transition-duration: .3s;
    transition-timing-function: ease, cubic-bezier(.6, 2, .75, 1.5);
}

/* browser.css */
toolbarpaletteitem[mousedown] > toolbaritem.panel-wide-item,
toolbarpaletteitem[mousedown] > toolbarbutton.panel-wide-item > .toolbarbutton-icon {
  transform: scale(1.1);
}

This still causes two problems on the combined items (ordered by severity):
1) There is a white flash when picking them up.
2) Once the animation is finished, the anti-aliasing of the text changes. This is weird because it usually happens the other way around (when the element goes from having a transform to having no transform).
3) The »bounce back« animation when releasing the item in place doesn't work

Comment 11

[:Gijs (he/him)]

Seems Blake volunteered to work on this! :-)

Comment 12

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

So, to fix #3, you need to put the transition in the plain class, not the mousedown class, like so:
/* panelUIOverlay.css */
#edit-controls:-moz-any(:not([cui-areatype="toolbar"]),.overflowedItem),
#zoom-controls:-moz-any(:not([cui-areatype="toolbar"]),.overflowedItem) {
    transition-property: background-color, transform;
    transition-duration: .3s;
    transition-timing-function: ease, cubic-bezier(.6, 2, .75, 1.5);
}

toolbarpaletteitem[mousedown=true] #edit-controls:-moz-any(:not([cui-areatype="toolbar"]),.overflowedItem),
toolbarpaletteitem[mousedown=true] #zoom-controls:-moz-any(:not([cui-areatype="toolbar"]),.overflowedItem) {
    background: hsla(210,4%,90%,1);
    border-radius: 3px;
    border: 1px solid #ccc;
}

/* browser.css */
toolbarpaletteitem[mousedown] > toolbaritem.panel-wide-item,
toolbarpaletteitem[mousedown] > toolbarbutton.panel-wide-item > .toolbarbutton-icon {
  transform: scale(1.1);
}

But this is still feeling kind of janky to me.  And it looks odd to have the combined buttons only scale by 1.1 in the toolbar, when the rest of the icons there scale by 1.3…

Comment 13

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

Attachment: bug958645.patch

Philipp: The backgrounds were problematic in a couple of ways, so I left them out.
(1. They seemed to be janky on my computer.
 2. I couldn't get them to not show up when the item was in the toolbar.
 3. They ran off the end of the panel, which seemed kind of ugly.)

Gijs: I needed to include the lines like:
+toolbarpaletteitem > #edit-controls:not([place="toolbar"]),
+toolbarpaletteitem > #zoom-controls:not([place="toolbar"]) {
because there's a rule in panelUIOverlay.css that would override the transition if I didn't.

Comment 14

[:Gijs (he/him)]

Comment on attachment 8365323 [details] [diff] [review]
bug958645.patch

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

This is the right idea, but I think there's quite a bit of selector confusion going on here and I'd like to see a cleaner version first.

::: browser/themes/shared/customizableui/customizeMode.inc.css
@@ +79,5 @@
>  toolbarpaletteitem[notransition][place="panel"] {
>    transition: none;
>  }
>  
> +toolbarpaletteitem > toolbarbutton .toolbarbutton-icon,

I'm quite weary of making this a descendant selector. Why was it necessary?

@@ +82,5 @@
>  
> +toolbarpaletteitem > toolbarbutton .toolbarbutton-icon,
> +toolbarpaletteitem > toolbarbutton .dropmarker-icon,
> +toolbarpaletteitem > toolbaritem.panel-wide-item,
> +toolbarpaletteitem > toolbarbutton.panel-wide-item > .toolbarbutton-icon,

Items that match this should match the top rule in this selector too, right? Also, what items match this selector? I didn't think we had buttons that were a panel wide item...

@@ +83,5 @@
> +toolbarpaletteitem > toolbarbutton .toolbarbutton-icon,
> +toolbarpaletteitem > toolbarbutton .dropmarker-icon,
> +toolbarpaletteitem > toolbaritem.panel-wide-item,
> +toolbarpaletteitem > toolbarbutton.panel-wide-item > .toolbarbutton-icon,
> +toolbarpaletteitem > toolbaritem.toolbaritem-combined-buttons,

All of these should have .panel-wide-item as well, I think, and the specificity is the same, so this shouldn't be necessary.

@@ +90,4 @@
>    transition: transform .3s cubic-bezier(.6, 2, .75, 1.5);
>  }
>  
> +toolbarpaletteitem[mousedown] > toolbarbutton .toolbarbutton-icon,

Ditto about the descendant selector...

@@ +95,5 @@
>    transform: scale(1.3);
>  }
>  
> +toolbarpaletteitem[mousedown] > toolbaritem.panel-wide-item,
> +toolbarpaletteitem[mousedown] > toolbaritem.toolbaritem-combined-buttons {

Again, don't think you need the second selector here.

Comment 15

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Overall this is great! Though there are two minor issues.

1) The combined buttons for bookmarks currently animate separately (the star has a different transform origin than the list icon and the line doesn't animate at all). This looks odd because you'll be dragging the item as one element.
2) There is still the issue with the anti-aliasing. When you grab the cut/copy/paste controls and wait for the bounce to finish, you'll see a change in text rendering right at the end of the animation.

There certainly is a trade off between time investment and level of polish at this point, but it would still be good to get at least #1 fixed.

Comment 16

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

Attachment: bug958645.patch

So, the separator disappears, but this should be more like what you were looking for, Philipp.  Let me know, and if you can figure out why the separator vanishes, I'ld love to hear it!  :)

Comment 17

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

Attachment: bug958645.patch

And I figured out why the separator disappears, and have fixed it!  \o/  (On Mac only, but asking for review, to get the styles correct before I work on the Linux and Windows versions.)

Comment 18

[:Gijs (he/him)]

Comment on attachment 8366744 [details] [diff] [review]
bug958645.patch

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

::: browser/themes/osx/browser.css
@@ +402,5 @@
>    border: 0;
>  }
>  
> +toolbar .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker {
> +  -moz-box-orient: horizontal;

I'm pretty weary about this, to be honest, as it's set to vertical just above here... Why was this change necessary?

::: browser/themes/shared/customizableui/customizeMode.inc.css
@@ +81,5 @@
>  }
>  
> +toolbarpaletteitem > toolbarbutton > .toolbarbutton-icon,
> +toolbarpaletteitem > toolbaritem.panel-wide-item,
> +toolbarpaletteitem > toolbarbutton[type="menu-button"],

This enlarges the entire button, including the label. Is that intentional?

Comment 19

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

(In reply to :Gijs Kruitbosch from comment #18)
> > +toolbar .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker {
> > +  -moz-box-orient: horizontal;
> I'm pretty wary about this, to be honest, as it's set to vertical just
> above here... Why was this change necessary?

If we don't change this here, then the separator appears above the dropmarker-icon, instead of beside it.
(It didn't used to be a problem, because the separator was taken out of the page flow by the "position: absolute", but that caused havoc when we tried to transform it.  So I switched it to "position: relative", but then it took up space.)

Oh, and I'm only doing it when it's in the toolbar, because otherwise the label appeared beside the icon in the panel menu.  (But the label's hidden in the toolbar, so being horizontal isn't a problem.)

> > +toolbarpaletteitem > toolbarbutton > .toolbarbutton-icon,
> > +toolbarpaletteitem > toolbaritem.panel-wide-item,
> > +toolbarpaletteitem > toolbarbutton[type="menu-button"],
> This enlarges the entire button, including the label. Is that intentional?

I believe so, but will defer to phlsa.

Thanks!

Comment 20

[:Gijs (he/him)]

Comment on attachment 8366744 [details] [diff] [review]
bug958645.patch

OK. This looks good to me, then, but I'm nervous enough about it that I'd like Dão to look at this once you've got a patch that fixes the Windows/Linux styling (or now, if you prefer - up to you).

Comment 21

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Comment on attachment 8366744 [details] [diff] [review]
bug958645.patch

Looks good to me!

Comment 22

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

Attachment: bug958645.patch

(Carrying forward r=Gijs and ui-r=phlsa.)

Dão, this version works (for me, with the built-in icons and Firebug, on Windows, Mac, and Linux).  Could you give it a once-over, and let me know what I can change to make it even better?  :)

Thanks!

Comment 23

[Dão Gottwald [:dao]]

Comment on attachment 8367569 [details] [diff] [review]
bug958645.patch

>+toolbar .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker {
>+  -moz-box-orient: horizontal;
>+}

Seems like you should use '#nav-bar' rather than 'toolbar' and move it next to this rule:

>@@ -419,17 +423,17 @@ toolbar .toolbarbutton-1:not([type="menu
> /**
>  * Draw seperators before toolbar button dropmarkers, as well as between
>  * consecutive toolbarbutton-1's within a toolbaritem.
>  */
> #nav-bar .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker::before,
> #nav-bar .toolbaritem-combined-buttons > .toolbarbutton-1 + .toolbarbutton-1::before {
>   content: "";
>   display: -moz-box;
>-  position: absolute;
>+  position: relative;


>-toolbarpaletteitem > toolbarbutton > .toolbarbutton-icon {
>+toolbarpaletteitem > toolbarbutton > .toolbarbutton-icon,
>+toolbarpaletteitem > toolbaritem.panel-wide-item,
>+toolbarpaletteitem > toolbarbutton[type="menu-button"],
>+/* Override the rules in panelUIOverlay.css and windows/linux browser.css. */

Seems like a valid use case for !important.

>+toolbarpaletteitem > #edit-controls:not([place="toolbar"]),
>+toolbarpaletteitem > #zoom-controls:not([place="toolbar"]),
>+#TabsToolbar .toolbarbutton-1 > .toolbarbutton-icon,
>+#nav-bar .toolbarbutton-1 > .toolbarbutton-icon {
>   transition: transform .3s cubic-bezier(.6, 2, .75, 1.5);
> }

We only want this while customizing, right? The last two selectors match toolbar buttons when not customizing.

Comment 24

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #23)
> Comment on attachment 8367569 [details] [diff] [review]
> bug958645.patch
> 
> >+toolbar .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker {
> >+  -moz-box-orient: horizontal;
> >+}
> 
> Seems like you should use '#nav-bar' rather than 'toolbar' and move it next
> to this rule:

I think instead we just want to use 'toolbar' everywhere - these effects should work on any toolbar.

Comment 25

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #24)
> (In reply to Dão Gottwald [:dao] from comment #23)
> > Comment on attachment 8367569 [details] [diff] [review]
> > bug958645.patch
> > 
> > >+toolbar .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker {
> > >+  -moz-box-orient: horizontal;
> > >+}
> > 
> > Seems like you should use '#nav-bar' rather than 'toolbar' and move it next
> > to this rule:
> 
> I think instead we just want to use 'toolbar' everywhere - these effects
> should work on any toolbar.

This isn't about effects, it's about the separator being laid out correctly since Blake made it position: relative instead of absolute.

Comment 26

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

Attachment: bug958645.patch

(In reply to Dão Gottwald [:dao] from comment #23)
> >+toolbar .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker {
> Seems like you should use '#nav-bar' rather than 'toolbar' and move it next
> to this rule:
> >@@ -419,17 +423,17 @@ toolbar .toolbarbutton-1:not([type="menu
> > #nav-bar .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker::before,

Fixed!

> >+/* Override the rules in panelUIOverlay.css and windows/linux browser.css. */
> Seems like a valid use case for !important.

Changed!

> >+#TabsToolbar .toolbarbutton-1 > .toolbarbutton-icon,
> >+#nav-bar .toolbarbutton-1 > .toolbarbutton-icon {
> >   transition: transform .3s cubic-bezier(.6, 2, .75, 1.5);
> > }
> We only want this while customizing, right? The last two selectors match
> toolbar buttons when not customizing.

Done!

Comment 27

[Dão Gottwald [:dao]]

Comment on attachment 8368710 [details] [diff] [review]
bug958645.patch

>+#nav-bar .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker {
>+  -moz-box-orient: horizontal;
>+}
>+
> /**
>  * Draw seperators before toolbar button dropmarkers, as well as between
>  * consecutive toolbarbutton-1's within a toolbaritem.
>  */
> #nav-bar .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker::before,
> #nav-bar .toolbaritem-combined-buttons > .toolbarbutton-1 + .toolbarbutton-1::before {

nit: move the new rule after the existing one such that the comment comes before all related rules:

>/**
> * Draw seperators before toolbar button dropmarkers, as well as between
> * consecutive toolbarbutton-1's within a toolbaritem.
> */
>#nav-bar .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker::before,
>#nav-bar .toolbaritem-combined-buttons > .toolbarbutton-1 + .toolbarbutton-1::before {
>  ...
>}
>
>#nav-bar .toolbarbutton-1 > .toolbarbutton-menubutton-dropmarker {
>  -moz-box-orient: horizontal;
>}

Comment 28

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

Attachment: The final version of the patch.

Carrying forward r=dao, r=Gijs, and ui-r=phlsa.

Comment 29

[Matthew N. [:MattN]]

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

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a36e188dbc74