(inactive) Nihanth Subramanya [:nhnt11] Assignee
	Description • 7 years ago

Currently toolbar button size matches spec (28x28) but there's a 1px border that should be replaced with padding to match the spec.

	Dão Gottwald [:dao]
	Comment 2 • 7 years ago

Why should background-{origin,clip} be border-box?

	(inactive) Nihanth Subramanya [:nhnt11] Assignee
	Comment 3 • 7 years ago

(In reply to Dão Gottwald [::dao] from comment #2)
> Why should background-{origin,clip} be border-box?

The spec suggests that icons have 6px of padding on each side, meaning that the overall background size is 6+16+6=28px.

When I increased the vertical padding, I simply increased the padding to a value which would result in the final 28px size, but I didn't take into account that padding-box is set, which means that currently the background size is only 26x26px; i.e. the buttons appear smaller than in the spec.

	(inactive) Nihanth Subramanya [:nhnt11] Assignee
	Comment 4 • 7 years ago

Alternatively, we can increase the padding by 1px in each direction, but I think that will affect the overall dimensions of the toolbar. I'll provide comparison screenshots.

	Dão Gottwald [:dao]
	Comment 5 • 7 years ago

What's wrong with your original idea to replace the 1px border with padding? That sounds like the right thing to do.

	Dão Gottwald [:dao]
	Comment 6 • 7 years ago
mozreview-review

Comment on attachment 8868993 [details]
Bug 1365901 - [Photon] Replace 1px toolbar button border with padding.

https://reviewboard.mozilla.org/r/140672/#review144102

	Dão Gottwald [:dao]
	Comment 8 • 7 years ago
mozreview-review

Comment on attachment 8868993 [details]
Bug 1365901 - [Photon] Replace 1px toolbar button border with padding.

https://reviewboard.mozilla.org/r/140672/#review144108

::: browser/themes/shared/toolbarbuttons.inc.css:390
(Diff revision 2)
>  #back-button > .toolbarbutton-icon {
>  %ifdef MOZ_PHOTON_THEME
> -  border-color: var(--backbutton-border-color);
> +  border: 1px solid var(--backbutton-border-color);
>    background-color: var(--backbutton-background);
> +  background-origin: padding-box !important;
> +  background-clip: padding-box !important;

Do you actually need !important here?

	Pulsebot
	Comment 12 • 7 years ago

Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/73e1bad26fe2
[Photon] Replace 1px toolbar button border with padding. r=dao

	Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)
	Comment 13 • 7 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/73e1bad26fe2

	Johann Hofmann [:johannh]
	Comment 14 • 7 years ago

Screenshots:

https://screenshots.mattn.ca/compare/?oldProject=mozilla-central&oldRev=8d60d0f825110cfb646ac31dc16dc011708bcf34&newProject=mozilla-central&newRev=51736db6772339152f72297c30817ab0f2c78dca