1045751 - Shrink tile (pin/block/gear) button sizes by a little bit

Status: RESOLVED FIXED

(Firefox :: New Tab Page, defect)

Description

[Ed Lee :Mardak]

From phlsa:  The buttons are very large relative to the size of the tile. Given that the 99.9% case for the user is *not* to click on those buttons, we should consider shrinking them. I tried quickly scaling them to 70% which works quite well (http://cl.ly/image/2h2P2G103938).

The current icons are 32x32, so 70% would be 22.4. What size should it be? 24? 22? These are SVG, so we can scale it to whatever we want.

Comment 1

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

Let's use 22px then.

Comment 2

[Manish Goregaokar [:manishearth]]

Note to people wanting to take this bug:

The code is scattered across browser/themes/shared/newtab (CSS and SVGs) and browser/base/content/newtab (JavaScript and XML source). Let me know if you need more help, either in a comment here or on IRC.

Comment 3

[Kumar Ayush]

Attachment: Suggested patch

Didn't change gear, debatable, like it the way it is. 

Didn't change the .svg files to keep rescaling easier in future

Comment 4

[Manish Goregaokar [:manishearth]]

Comment on attachment 8470591 [details] [diff] [review]
Suggested patch

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

LGTM, though are you going to resize the gear in a different patch?

I won't be able to grant review+ for this patch, but someone else can.

Comment 5

[Ed Lee :Mardak]

Comment on attachment 8470591 [details] [diff] [review]
Suggested patch

dcrobot says the gear should be 27px and phlsa says tile buttons 22px. Although for the tile buttons, there's a shadow so 24px could be fine and 28px for similar svg spacing?

>+++ b/browser/themes/shared/newtab/newTab.inc.css
> .newtab-control {
>   background-color: transparent;
>   border: none;
>-  height: 32px;
>-  width: 32px;
>+  height: 24px;
>+  width: 24px;
> }
> 
> .newtab-control-pin,
> .newtab-site[pinned] .newtab-control-pin:hover:active {
>   background-image: -moz-image-rect(url(chrome://browser/skin/newtab/controls.svg), 0, 96, 32, 64);
>+  background-size:24px;
nit: space after the ":"

Also, could probably move the background-size to the .newtab-control and explicitly background-size: auto for sponsored although that icon be removed soon with bug 1040369.

Comment 6

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Comment on attachment 8470591 [details] [diff] [review]
Suggested patch

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

What Ed said in comment 5, including moving background-size.

And apparently we want the gear at 27px?

Comment 7

[Kumar Ayush]

I agree that the pin should have been smaller. I made it 24 px specifically after the older versions which were pretty fine. 

One of the reasons 'pin' looks better when smaller is that it obstructs the background less. That is not true in case of the gear. My opinion is that the gear should be left as it is.

Whether we want the pin to 22/24/26 is still open to discussion.

Comment 8

[Blair McBride [:Unfocused] (UNAVAILABLE)]

Ok, over to UX then.

Comment 9

[Aaron]

24 pixels is fine with me.

Comment 10

[Blair McBride [:Unfocused] (UNAVAILABLE)]

(In reply to Aaron from comment #9)
> 24 pixels is fine with me.

What about the gear icon?

Comment 11

[Aaron]

The gear icon should be 28 x 28

Comment 12

[Ed Lee :Mardak]

Kumar, you can create the patch ignoring newtab-control-sponsored as I'm removing it in bug 1040369. And dcrobot says the gear should be 28px.

Comment 13

[Kumar Ayush]

Attachment: Version#2, pin/block to 24px, gear to 28px

Comment 14

[Ed Lee :Mardak]

Comment on attachment 8474042 [details] [diff] [review]
Version#2, pin/block to 24px, gear to 28px

>+++ b/browser/themes/shared/newtab/newTab.inc.css
> #newtab-customize-button {
>   background-color: transparent;
>   background-image: -moz-image-rect(url(chrome://browser/skin/newtab/controls.svg), 0, 32, 32, 0);
>+  background-size:28px auto;
Is the auto necessary? You don't have it below for the 24px controls and it seems to work. Also nit: space after the ":".

> .newtab-control {
>   background-color: transparent;
You should be able to set the background-size here...

> .newtab-control-sponsored {
>-  width: 24px;
>-  height: 24px;
>+  width: 32px;
>+  height: 32px;
>   background-image: url(chrome://browser/skin/newtab/controls.png);
>   background-position: -249px -1px;
... then override it here.. I think?

Comment 15

[Kumar Ayush]

Attachment: fix_1045751.patch

Made suggested changes to 8474042 (Version#2)

Comment 16

[Kumar Ayush]

Attachment: Version#3: Made suggested changes to Version#2

Comment 17

[Ed Lee :Mardak]

Comment on attachment 8474073 [details] [diff] [review]
Version#3: Made suggested changes to Version#2

>+++ b/browser/themes/shared/newtab/newTab.inc.css
> .newtab-control-sponsored {
>   background-image: url(chrome://browser/skin/newtab/controls.png);
>+  background-size: 32px;
> }
> 
> @media (min-resolution: 2dppx) {
>   .newtab-control-sponsored {
>     background-image: url(chrome://browser/skin/newtab/[e-mail]);
>     background-size: 296px;
>   }
> }
Sorry I wasn't more explicit in my previous comment: "... then override it here," but the sponsored control isn't using the svg file, so the background size needs to be of the original controls.png height/width instead of being overridden by the newly added "background-size: 24px;"

You can test if the sponsored icon is showing up correctly by creating a new profile.

Comment 18

[Ed Lee :Mardak]

Attachment: screenshot of v3 (background-size: 32px;)

Here's what it looks like with background-size: 32px with controls.png for the sponsored button.

Comment 19

[Kumar Ayush]

Attachment: v4

in .newtab-control-sponsored, setting background-size to auto does the job. In fact, I tried all other combinations. This is the only option that works.

Comment 20

[Kumar Ayush]

Also, the lines 200/201 i.e. width and height setting in .newtab-control-sponsored is overridden by another css property to be 14px. Should I remove it altogether?

Comment 21

[Manish Goregaokar [:manishearth]]

Comment on attachment 8474073 [details] [diff] [review]
Version#3: Made suggested changes to Version#2

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

LGTM

Comment 22

[Ed Lee :Mardak]

(In reply to Kumar Ayush from comment #19)
> Created attachment 8474087 [details] [diff] [review]
> in .newtab-control-sponsored, setting background-size to auto does the job.
> In fact, I tried all other combinations. This is the only option that works.
Thanks for trying out the different combinations.

> Also, the lines 200/201 i.e. width and height setting in
> .newtab-control-sponsored is overridden by another css property to be 14px.
> Should I remove it altogether?
Nice find! Yeah, go ahead and clean that up.

Comment 23

[Kumar Ayush]

Attachment: v5, all done.

Comment 24

[Ed Lee :Mardak]

Comment on attachment 8474107 [details] [diff] [review]
v5, all done.

Thanks Kumar! Looks good to me.

Comment 25

[Manish Goregaokar [:manishearth]]

Comment on attachment 8474087 [details] [diff] [review]
v4

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

Oops

Comment 26

[Ed Lee :Mardak]

Just in case someone was thinking of prepping Kumar's patch for landing once it gets reviewed, I have it ready with commit author "Kumar Ayush <[e-mail]>" etc.

Comment 27

[Drew Willcoxon :adw]

Comment on attachment 8474107 [details] [diff] [review]
v5, all done.

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

Thanks Kumar!

Ed, are you going to land this then?

Comment 28

[Ed Lee :Mardak]

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

Comment 29

[Ed Morley [:emorley]]

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

Comment 30

[Sylvestre Ledru [:Sylvestre]]

Uplift has been managed in bug 1057602

Comment 31

[Ed Lee :Mardak]

https://hg.mozilla.org/releases/mozilla-aurora/rev/7f6e4da5d103