Closed Bug 1365901 Opened 7 years ago Closed 6 years ago

[Photon] Replace 1px toolbar button border with padding

Categories

(Firefox :: Theme, enhancement, P1)

Product:
Firefox
Component:
Theme
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 55
Iteration:
55.6 - May 29
Tracking Flags:
Tracking Status
firefox55 --- fixed

People

(Reporter: nhnt11, Assigned: nhnt11)

References

Details

(Whiteboard: [photon-visual][p1][57])

Attachments

(1 file)

Bug 1365901 - [Photon] Replace 1px toolbar button border with padding.
59 bytes, text/x-review-board-request
dao
: review+
Details 
Currently toolbar button size matches spec (28x28) but there's a 1px border that should be replaced with padding to match the spec.
Iteration: --- → 55.6 - May 29
Flags: qe-verify?
Summary: Replace 1px toolbar button border with padding → [Photon] background-{origin,clip} of toolbarbutton icons should be border-box.
Why should background-{origin,clip} be border-box?
Flags: needinfo?(nhnt11)
(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.
Flags: needinfo?(nhnt11)
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.
What's wrong with your original idea to replace the 1px border with padding? That sounds like the right thing to do.
Flags: needinfo?(nhnt11)
Comment on attachment 8868993 [details]
Bug 1365901 - [Photon] Replace 1px toolbar button border with padding.

https://reviewboard.mozilla.org/r/140672/#review144102
Attachment #8868993 - Flags: review?(dao+bmo)
Flags: needinfo?(nhnt11)
Summary: [Photon] background-{origin,clip} of toolbarbutton icons should be border-box. → [Photon] Replace 1px toolbar button border with padding
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?
Attachment #8868993 - Flags: review?(dao+bmo) → review+
Depends on: 1365195
Blocks: 1352358
Pushed by nhnt11@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/73e1bad26fe2
[Photon] Replace 1px toolbar button border with padding. r=dao
https://hg.mozilla.org/mozilla-central/rev/73e1bad26fe2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Flags: qe-verify? → qe-verify-
You need to log in before you can comment on or make changes to this bug.