1100435 - Unable to animate toolbar icons of menu buttons properly on Windows

Status: RESOLVED INVALID

(Firefox :: Theme, defect)

Description

[Mike Kaply [:mkaply]]

Bug 960517 adjusted the toolbar buttons on Windows Australis so that the toolbar borders are part of the image, not part of the button.

Unfortunately this means you can no longer do interesting things with the icon like animate it (for instance spinning while checking for mail), because the borders are a part of the image.

This styling does not belong in the image, it belongs on the toolbar button.

Comment 1

[Mike Kaply [:mkaply]]

Attachment: spinningbutton.jpg

Comment 2

[Tim Nguyen :ntim]

This isn't because of bug 960517

Comment 3

[Mike Kaply [:mkaply]]

> This isn't because of bug 960517

It is according to the blame:

http://hg.mozilla.org/mozilla-central/annotate/134d1cfc5c9c/browser/themes/windows/browser.css#l714

Comment 4

[:Gijs (he/him)]

(In reply to Mike Kaply (:mkaply) from comment #3)
> > This isn't because of bug 960517
> 
> It is according to the blame:
> 
> http://hg.mozilla.org/mozilla-central/annotate/134d1cfc5c9c/browser/themes/
> windows/browser.css#l714

Uh, what? That link proves roughly nothing. The diff:

http://hg.mozilla.org/mozilla-central/diff/ece9cdfb15d7/browser/themes/windows/browser.css#l1.8

shows that the border styles were already set on the icon, text and badge before then...

AFAIK these borders have always been on the icon - at least since the days of Firefox 4 or thereabouts, to increase the hit area of the buttons by having padding outside the border of the icon be the button's padding, leading to that being clickable, too. I suspect wontfix, but Dão will know for sure.

Comment 5

[Mike Kaply [:mkaply]]

I just checked Firefox 24 and none of the borders, shadows, etc. are drawn on the actual toolbarbutton-icon image, they are on the button. You can see this with the DOM inspector.

The spinning works fine on Firefox 24 (and doesn't spin the entire button).

Comment 6

[:Gijs (he/him)]

(In reply to Mike Kaply (:mkaply) from comment #5)
> I just checked Firefox 24 and none of the borders, shadows, etc. are drawn
> on the actual toolbarbutton-icon image, they are on the button. You can see
> this with the DOM inspector.
> 
> The spinning works fine on Firefox 24 (and doesn't spin the entire button).

On Windows? What kind of button are you testing with? AFAICT bug 735691 changed this, which according to the bug landed in Firefox 14: http://hg.mozilla.org/mozilla-central/diff/e9938aab62e2/browser/themes/winstripe/browser.css#l1.86

Comment 7

[Mike Kaply [:mkaply]]

On Windows. It's a menu-button. I'll throw together a testcase. And again, it worked on FF24, doesn't work on FF31.

Comment 8

[Mike Kaply [:mkaply]]

Attachment: Extension that shows the problem

This is extension shows the problem.

You'll have to manually drag both buttons onto the nav-bar.

One uses the toolbarbutton-1 class and one doesn't just to show how much that affects things.

If you install this extension on Firefox 24, you'll see that both icons animate fine.

On Firefox 31, the outline animates on the toolbarbutton-1 version.

I'll narrow down the regression range, but this is definitely a regression.

Comment 9

[:Gijs (he/him)]

(In reply to Mike Kaply (:mkaply) from comment #7)
> On Windows. It's a menu-button.

That's kind of important if you initially claim that bug X adjusted *all* toolbar button borders...

I expect that we just fixed a bug relating to menu button borders. I would be surprised if we could reasonably fix this without breaking the hit area thing I outlined in comment #4, and suggest wontfix. If you want to animate the icon, I'd suggest using an animated png or SVG.

Comment 10

[Mike Kaply [:mkaply]]

It happens with all toolbarbuttons on windows (not just menu-buttons).

They shouldn't be using the image to add borders. They should be using the toolbarbutton box itself.

Comment 11

[:Gijs (he/him)]

(In reply to Mike Kaply (:mkaply) from comment #10)
> It happens with all toolbarbuttons on windows (not just menu-buttons).
> 
> They shouldn't be using the image to add borders. They should be using the
> toolbarbutton box itself.

Do the non-menu buttons work in Firefox 24? On what version of Windows are you testing?

Comment 12

[Mike Kaply [:mkaply]]

Attachment: Extension that shows all three button types animating incorrectly

Yes, it works perfectly on Firefox 24 for all three button types.

The button properly animates inside of it's own area.

This is Windows 7.

Comment 13

[Mike Kaply [:mkaply]]

Regression range is:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=a475f94bb1b1&tochange=f2adb62d07eb

Comment 14

[Mike Kaply [:mkaply]]

Which is useless. Gigantic UX merge.

Comment 15

[:Gijs (he/him)]

(In reply to Mike Kaply (:mkaply) from comment #14)
> Which is useless. Gigantic UX merge.

On some of the older mozregression instances you could use --repo and pass the url to projects/ux, and get a regression range on that branch, if you're interested.


(In reply to Mike Kaply (:mkaply) from comment #10)
> They shouldn't be using the image to add borders. They should be using the
> toolbarbutton box itself.

I've still not heard how you think we should be doing that while maintaining the click area requirements, or why you can't use the suggestions I gave you to use animated pngs or svg files instead.

Comment 16

[Mike Kaply [:mkaply]]

It's not about click area requirements. It's about drawing the actual border and dropshadow as a part of the image instead of the button.

The border and dropshadow are attributes of the button.

As far as your suggestion, I'll look into it, but one of the really nice things about CSS is that it does animations for me.

Comment 17

[:Gijs (he/him)]

(In reply to Mike Kaply (:mkaply) from comment #12)
> Created attachment 8524097 [details]
> Extension that shows all three button types animating incorrectly
> 
> Yes, it works perfectly on Firefox 24 for all three button types.
> 
> The button properly animates inside of it's own area.
> 
> This is Windows 7.

Uh, actually, I'm going to disagree with all of this. This is what your test add-on looks like on Firefox 28 when you start hovering over the button:

http://imgur.com/YDxKqyo

The regression range is presumably for the borders which are now there by default on windows 7?

Comment 18

[:Gijs (he/him)]

(In reply to :Gijs Kruitbosch from comment #17)
> (In reply to Mike Kaply (:mkaply) from comment #12)
> > Created attachment 8524097 [details]
> > Extension that shows all three button types animating incorrectly
> > 
> > Yes, it works perfectly on Firefox 24 for all three button types.
> > 
> > The button properly animates inside of it's own area.
> > 
> > This is Windows 7.
> 
> Uh, actually, I'm going to disagree with all of this. This is what your test
> add-on looks like on Firefox 28 when you start hovering over the button:
> 
> http://imgur.com/YDxKqyo
> 
> The regression range is presumably for the borders which are now there by
> default on windows 7?

And this was the case on 24 as well.

(In reply to Mike Kaply (:mkaply) from comment #16)
> It's not about click area requirements. It's about drawing the actual border
> and dropshadow as a part of the image instead of the button.
> 
> The border and dropshadow are attributes of the button.

No, you would *like* the border and the dropshadow to not be on the icon, because it interferes with what you want to do. That's fine, but for the user, there is no perceivable issue here, whereas there would be if we changed the click area.

Considering the location of the border drawing isn't a recent regression, I'm going to mark this as invalid.

Comment 19

[Ben Bucksch (:BenB)]

> No, you would *like* the border and the dropshadow to not be on the icon

I'll have to agree with Mike here. The borders *belong* to the button, not the image.

Comment 20

[Dão Gottwald [:dao]]

As Gijs said, we want the clickable area to extend beyond the drawn border, hence we can't draw the border around the whole button.

(In reply to Mike Kaply (:mkaply) from comment #16)
> As far as your suggestion, I'll look into it, but one of the really nice
> things about CSS is that it does animations for me.

Another way to deal with this would be to apply the CSS animation only on :not(:hover).

Comment 21

[Mike Kaply [:mkaply]]

> As Gijs said, we want the clickable area to extend beyond the drawn border, hence we can't draw the border around the whole button.

Somehow when Gijs said it, I didn't understand. Now I do. I guess that is the only way to do that. I'm curious as to where that design came from, though. I don't see other Windows apps behaving like that (even Windows for that matter). I don't think users expect the button to be clickable outside the borders, and it's a very large target area.

Comment 22

[Dão Gottwald [:dao]]

(In reply to Mike Kaply (:mkaply) from comment #21)
> I'm curious as to where that design came from,
> though. I don't see other Windows apps behaving like that (even Windows for
> that matter). I don't think users expect the button to be clickable outside
> the borders, and it's a very large target area.

At the time this was implemented (not sure if Windows 8 or later changed anything there), native Windows applications usually had toolbar buttons visibly fill the entire toolbar height. Our designers didn't want that, so we ended up with this compromise between visual appeal and usability. If we want to revisit this decision, I think we should do it only based on telemetry or (better) health report data confirming that clicks outside of the small bordered area hardly ever happen.