1420287 - The buttons in the toolbar of the page info window have lost their padding

Status: RESOLVED FIXED

(Toolkit :: Themes, defect, P2)

Description

[Markus Stange [:mstange]]

Attachment: before

They look a bit squished now. This is a regression from bug 1410540.

Comment 1

[Markus Stange [:mstange]]

Attachment: after

Comment 2

[Brian Grinstead [:bgrins]]

Looks like the radio > label had this (in addition to the 1px padding on the radio)"

 margin-inline-start: 6px;
 margin-inline-end: 5px;

I think just bumping up the padding on the radio should be sufficient.

Comment 3

[Brian Grinstead [:bgrins]]

Attachment: Bug 1420287 - Add extra horizontal padding on radios in the Page Info dialog on OSX to match pre-58 style;

Review commit: https://reviewboard.mozilla.org/r/203234/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/203234/

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8932187 [details]
Bug 1420287 - Add extra horizontal padding on radios in the Page Info dialog on OSX to match pre-58 style;

https://reviewboard.mozilla.org/r/203234/#review208892

::: browser/themes/osx/pageInfo.css:32
(Diff revision 1)
>    font: menu;
>    text-shadow: @loweredShadow@;
>    margin: 0;
> -  padding: 0 1px;
> +  padding: 0;
> +  padding-inline-start: 6px;
> +  padding-inline-end: 5px;

Can you just use padding: 0 6px?

Comment 5

[Brian Grinstead [:bgrins]]

Comment on attachment 8932187 [details]
Bug 1420287 - Add extra horizontal padding on radios in the Page Info dialog on OSX to match pre-58 style;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203234/diff/1-2/

Comment 6

[Dão Gottwald [:dao]]

Have you checked Windows and Linux? I think we may have lost some padding there too.

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Dão Gottwald [::dao] from comment #6)
> Have you checked Windows and Linux? I think we may have lost some padding
> there too.

No, but I can check later. The CSS and UI is pretty different for them but it's possible we had some changes after switching to the standard binding.

Comment 8

[Dão Gottwald [:dao]]

Comment on attachment 8932187 [details]
Bug 1420287 - Add extra horizontal padding on radios in the Page Info dialog on OSX to match pre-58 style;

https://reviewboard.mozilla.org/r/203234/#review209022

Comment 9

[Brian Grinstead [:bgrins]]

Comment on attachment 8932187 [details]
Bug 1420287 - Add extra horizontal padding on radios in the Page Info dialog on OSX to match pre-58 style;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203234/diff/2-3/

Comment 10

[Brian Grinstead [:bgrins]]

Confirmed that the extra spacing was reduced via a smaller margin on the .radio-label (previously "viewButtonLabel" which used the default label margin).  Going to re-request review so you can have another look before landing

Comment 11

[Brian Grinstead [:bgrins]]

Comment on attachment 8932187 [details]
Bug 1420287 - Add extra horizontal padding on radios in the Page Info dialog on OSX to match pre-58 style;

Includes the windows and linux changes

Comment 12

[Dão Gottwald [:dao]]

Comment on attachment 8932187 [details]
Bug 1420287 - Add extra horizontal padding on radios in the Page Info dialog on OSX to match pre-58 style;

https://reviewboard.mozilla.org/r/203234/#review211448

::: browser/themes/linux/pageInfo.css:25
(Diff revision 3)
>    background-color: Highlight;
>    color: HighlightText;
>  }
>  
> +#viewGroup > radio > .radio-label-box {
> +  margin: 0 6px;

The focus ring looks off with that change.

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8932187 [details]
Bug 1420287 - Add extra horizontal padding on radios in the Page Info dialog on OSX to match pre-58 style;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/203234/diff/3-4/

Comment 14

[Brian Grinstead [:bgrins]]

For windows and linux I moved the spacing into padding on .radio-label-box and zeroed out both padding on the radio parent and the .radio-label-box so that the focus ring looks better

Comment 15

[Dão Gottwald [:dao]]

Comment on attachment 8932187 [details]
Bug 1420287 - Add extra horizontal padding on radios in the Page Info dialog on OSX to match pre-58 style;

https://reviewboard.mozilla.org/r/203234/#review211812

::: browser/themes/linux/pageInfo.css:15
(Diff revision 4)
>  #viewGroup > radio {
>    list-style-image: url("chrome://browser/skin/pageInfo.png");
>    -moz-appearance: none;
>    min-width: 4.5em;
>    margin: 0;
> -  padding: 3px;
> +  padding: 0;

This doesn't look right to me either. The focus ring is pretty much invisible now on Ubuntu.

::: browser/themes/windows/pageInfo.css:31
(Diff revision 4)
>    background-color: #C1D2EE;
>    color: black;
>  }
>  
> +#viewGroup > radio > .radio-label-box {
> +  padding: 5px 9px 1px 9px;

nit: "padding: 5px 9px 1px;" is the preferred notation for horizontally symmetric values

Comment 16

[Mike Taylor [:miketaylr]]

Gonna set to fix-optional for 58, since this is kinda polish-y. But it would be great if it could land for 58 uplift.

Comment 17

[Dão Gottwald [:dao]]

Attachment: Bug 1420287 - Add horizontal padding on top bar radio buttons in the Page Info dialog to match pre-58 style.

Review commit: https://reviewboard.mozilla.org/r/216226/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/216226/

Comment 18

[Brian Grinstead [:bgrins]]

Comment on attachment 8946267 [details]
Bug 1420287 - Add horizontal padding on top bar radio buttons in the Page Info dialog to match pre-58 style.

https://reviewboard.mozilla.org/r/216226/#review222148

Thanks for taking this

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/979f77ae3677
Add horizontal padding on top bar radio buttons in the Page Info dialog to match pre-58 style. r=bgrins

Comment 20

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/979f77ae3677

Comment 21

[Dão Gottwald [:dao]]

Comment on attachment 8946267 [details]
Bug 1420287 - Add horizontal padding on top bar radio buttons in the Page Info dialog to match pre-58 style.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1410540
[User impact if declined]: see comment 0 and comment 1
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: no
[Needs manual test from QE? If yes, steps to reproduce]: /
[List of other uplifts needed for the feature/fix]: /
[Is the change risky?]: no
[Why is the change risky/not risky?]: simple styling fix
[String changes made/needed]: /

Comment 22

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8946267 [details]
Bug 1420287 - Add horizontal padding on top bar radio buttons in the Page Info dialog to match pre-58 style.

Minor css fix to one page, no reason not to uplift this, as we are still pretty early in beta.

Comment 23

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/cd0aa60ce8d9