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

RESOLVED FIXED in Firefox 59

Status

()

defect
P2
normal
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: mstange, Assigned: dao)

Tracking

({regression})

Trunk
mozilla60
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox57 unaffected, firefox58 wontfix, firefox59 fixed, firefox60 fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Posted image before
They look a bit squished now. This is a regression from bug 1410540.
Posted image after
Priority: -- → P2
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.
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
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?
Attachment #8932187 - Flags: review?(dao+bmo)
Have you checked Windows and Linux? I think we may have lost some padding there too.
Flags: needinfo?(bgrinstead)
(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 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
Attachment #8932187 - Flags: review?(dao+bmo) → review+
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
Flags: needinfo?(bgrinstead)
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
Attachment #8932187 - Flags: review+ → review?(dao+bmo)
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.
Attachment #8932187 - Flags: review?(dao+bmo) → review-
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 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
Attachment #8932187 - Flags: review?(dao+bmo) → review-
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.
Assignee: bgrinstead → dao+bmo
OS: Mac OS X → All
Attachment #8932187 - Attachment is obsolete: true
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
Attachment #8946267 - Flags: review?(bgrinstead) → review+
Pushed by dgottwald@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/979f77ae3677
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla60
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]: /
Attachment #8946267 - Flags: approval-mozilla-beta?
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.
Attachment #8946267 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.