1182187 - Remove one-off styling for .identity-popup-button

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Dão Gottwald [:dao]]

Attachment: identity-popup-button.diff

See bug 1175239 comment 28. I understand the desire to "update older styles" (although I'll note that there's nothing inherently old about native buttons; they automatically get design updates along with the host OS), but like I said this isn't the right way to implement that.

This should help with bug 1181803 too.

Comment 1

[Tim Taubert [:ttaubert] (inactive)]

I think it doesn't look too terrible... Aislinn, what do you think?

http://i.imgur.com/nIVM31B.png
http://i.imgur.com/4sB5F9x.png

Comment 2

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8631714 [details] [diff] [review]
identity-popup-button.diff

Needs to be updated due to bug 1180841.

Comment 3

[Dão Gottwald [:dao]]

Attachment: identity-popup-button.diff

Comment 4

[Ash Grigas]

(In reply to Tim Taubert [:ttaubert] from comment #1)
> I think it doesn't look too terrible... Aislinn, what do you think?
> 
> http://i.imgur.com/nIVM31B.png
> http://i.imgur.com/4sB5F9x.png

Can you increase the button height so theres more white space between the top and bottom of the text?

Comment 5

[Dão Gottwald [:dao]]

Attachment: patch v2

(In reply to agrigas from comment #4)
> (In reply to Tim Taubert [:ttaubert] from comment #1)
> > I think it doesn't look too terrible... Aislinn, what do you think?
> > 
> > http://i.imgur.com/nIVM31B.png
> > http://i.imgur.com/4sB5F9x.png
> 
> Can you increase the button height so theres more white space between the
> top and bottom of the text?

I re-added the 30px min-height

Comment 6

[Ash Grigas]

(In reply to Dão Gottwald [:dao] from comment #5)
> Created attachment 8632136 [details] [diff] [review]
> patch v2
> 
> (In reply to agrigas from comment #4)
> > (In reply to Tim Taubert [:ttaubert] from comment #1)
> > > I think it doesn't look too terrible... Aislinn, what do you think?
> > > 
> > > http://i.imgur.com/nIVM31B.png
> > > http://i.imgur.com/4sB5F9x.png
> > 
> > Can you increase the button height so theres more white space between the
> > top and bottom of the text?
> 
> I re-added the 30px min-height

This styling doesn't look like our buttons in preferences. The outline stroke is too dark and the text padding is still to narrow...

Comment 7

[Tim Taubert [:ttaubert] (inactive)]

But that's what our buttons look like...

http://i.imgur.com/sohuR8H.png
http://i.imgur.com/o12UI04.png

Not easy to find that style in the main UI nowadays, it's more or less hidden dialogs most people probably don't encounter day-to-day.

Comment 8

[Ash Grigas]

To clarify the style we're going forward with for the new design of control center, we are using the About:Preferences button style. Using the style is a deliberate choice to update the design of some menus to fit the new design styling created for other parts of the browser.

Comment 9

[Dão Gottwald [:dao]]

(In reply to agrigas from comment #8)
> To clarify the style we're going forward with for the new design of control
> center, we are using the About:Preferences button style. Using the style is
> a deliberate choice to update the design of some menus to fit the new design
> styling created for other parts of the browser.

This was already understood, let's not go round in circles. The problem here is more nuanced; it seems that this design change wasn't well-planned and thus it was executed in a way that doesn't scale and makes the UI inconsistent. It further seems that the control center doesn't actually need a new button design at all but was rather used as a vehicle for that design change. This may make sense for some design work but it doesn't in this case. What should happen is that we get a bug filed dedicated to that design change, and then we can prioritize that and think about the best way to implement it.

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8632136 [details] [diff] [review]
patch v2

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

Let's go with that button style, the patch needs some rebasing and account for the third button we have there now.

The min-height is definitely the right thing to do but it destroys the rounded corners for reasons I don't understand. It makes the button rectangular and puts a weird gradient as the background.

Comment 11

[Dão Gottwald [:dao]]

(In reply to Tim Taubert [:ttaubert] from comment #10)
> The min-height is definitely the right thing to do but it destroys the
> rounded corners for reasons I don't understand. It makes the button
> rectangular and puts a weird gradient as the background.

This sounds super weird. Is this on OS X? I'll try adding padding on .button-box instead.

Comment 12

[Dão Gottwald [:dao]]

Attachment: patch v3

Comment 13

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8637271 [details] [diff] [review]
patch v3

This looks better, thanks. The black border makes buttons very heavy though, which doesn't quite work with the light lines in the rest of the panel. Can we make it the same as for the bookmarks edit panel?

http://i.imgur.com/gpzDgUd.png

Comment 14

[Dão Gottwald [:dao]]

Attachment: patch v4

Comment 15

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8637295 [details] [diff] [review]
patch v4

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

Beautiful. Thank you very much for helping us get it right!

Comment 16

[Pulsebot]

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

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7cbcece973de