1153952 - social buttons are too large

Status: RESOLVED FIXED

(Firefox :: Theme, defect)

Description

[Shane Caraveo (:mixedpuppy)]

Attachment: sdkbutton

Attached patch fixes button sizing bug by adding sdk-button=true to the social buttons.  However, IMO it would be better to have a constrain-size=true attribute in case anyone decides to use sdk-button for something in the future (I am not certain whether there is some code out there looking for buttons with sdk-button=true).

Comment 1

[Dão Gottwald [:dao]]

Comment on attachment 8591792 [details] [diff] [review]
sdkbutton

I'm not sure if this could cause trouble in the future. Probably best to get feedback from SDK people too.

>--- a/browser/themes/windows/browser.css
>+++ b/browser/themes/windows/browser.css
>@@ -1625,23 +1625,16 @@ richlistitem[type~="action"][actiontype=
> }
> .share-provider-button > .toolbarbutton-icon {
>   width: 16px;
>   min-height: 16px;
>   max-height: 16px;
> }
> 
> 
>-toolbarbutton[type="socialmark"] > .toolbarbutton-icon {
>-  width: auto;
>-  height: auto;
>-  max-width: 32px;
>-  max-height: 24px;
>-}

Oh, yeah, thanks for removing this too.

Comment 2

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8591792 [details] [diff] [review]
sdkbutton

Matteo, do you see any current or speculative problem with the social buttons using the sdk-button attribute?

Comment 3

[Matteo Ferretti [:zer0] [:matteo]]

The current use we have for "sdk-button" is actually only for the size, so I don't see any problem to use it atm, however I agreed that would be better having something like `constrain-size`, and then refactoring the SDK part too – I can take care of that.

However, it seems to me that we have a regression, since if I create a button via SDK, and I set a larger  image (e.g. 512x512), the button will be huge instead of be resized.

Comment 4

[Shane Caraveo (:mixedpuppy)]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #3)
> The current use we have for "sdk-button" is actually only for the size, so I
> don't see any problem to use it atm, however I agreed that would be better
> having something like `constrain-size`, and then refactoring the SDK part
> too – I can take care of that.

Should we change sdk-button everywhere to constrain-size?  Or do you want both attributes?

> However, it seems to me that we have a regression, since if I create a
> button via SDK, and I set a larger  image (e.g. 512x512), the button will be
> huge instead of be resized.

I beleive that would be bug 1153243.

Comment 5

[Matteo Ferretti [:zer0] [:matteo]]

(In reply to Shane Caraveo (:mixedpuppy) from comment #4)

> (In reply to Matteo Ferretti [:matteo] [:zer0] from comment #3)
> > The current use we have for "sdk-button" is actually only for the size, so I
> > don't see any problem to use it atm, however I agreed that would be better
> > having something like `constrain-size`, and then refactoring the SDK part
> > too – I can take care of that.
> 
> Should we change sdk-button everywhere to constrain-size?  Or do you want
> both attributes?

I would say having just `constrain-size` would be better. Unless someone else wants to have a SDK specific attribute for some reason, I'm in favor of having just one generic attribute, like the one you suggested.

Comment 6

[Shane Caraveo (:mixedpuppy)]

Attachment: sdkbutton

This changes sdk-button to constrain-size, and uses that for both sdk and social buttons.

Comment 7

[Matteo Ferretti [:zer0] [:matteo]]

Comment on attachment 8591888 [details] [diff] [review]
sdkbutton

Looks good to me, but the SDK changes needs a separate PR to http://github.com/mozilla/addon-sdk, that is main repo – any changes made directly to firefox source code will be overwritten in the next uplift, I'm afraid.

Comment 8

[Shane Caraveo (:mixedpuppy)]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #7)
> Comment on attachment 8591888 [details] [diff] [review]
> sdkbutton
> 
> Looks good to me, but the SDK changes needs a separate PR to
> http://github.com/mozilla/addon-sdk, that is main repo – any changes made
> directly to firefox source code will be overwritten in the next uplift, I'm
> afraid.

Ok, I'll remove that part of the patch, can you submit that to the addon-sdk repo?

Comment 9

[Shane Caraveo (:mixedpuppy)]

Actually, I'll leave that part in for now, and addon-sdk can get the update hopefully prior to the next uplift.

Comment 10

[Shane Caraveo (:mixedpuppy)]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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

Comment 12

[Matteo Ferretti [:zer0] [:matteo]]

Attachment: Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1936

Not sure if this change need a review Erik, but just to play safe. It's really a quick one. :)

Comment 13

[Dave Townsend [:mossop]]

Comment on attachment 8593967 [details] [review]
Link to Github pull-request: https://github.com/mozilla/addon-sdk/pull/1936

We generally assume that if code has already been reviewed and landed in m-c then the uplift to github doesn't need a review.

Comment 14

[[github robot]]

Commits pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/dcd987d3a94af8a07d4a0f93296b7c2bcc737d14
Bug 1153952 - social buttons are too large

 - Updated the sdk specific attribute to the new more generic `constrain-size`, to have the buttons that fits the toolbar and the panel.

https://github.com/mozilla/addon-sdk/commit/6fca9e732d0b145e620ed03ffe46281c3f306b66
Merge pull request #1936 from ZER0/constrain-size/1153952

fix Bug 1153952 - social buttons are too large