Closed Bug 1420287 Opened 3 years ago Closed 3 years ago

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

Categories

(Toolkit :: Themes, defect, P2)

defect

Tracking

()

RESOLVED FIXED
mozilla60
Tracking Status
firefox-esr52 --- unaffected
firefox57 --- unaffected
firefox58 --- wontfix
firefox59 --- fixed
firefox60 --- fixed

People

(Reporter: mstange, Assigned: dao)

References

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

Attached image before
They look a bit squished now. This is a regression from bug 1410540.
Attached 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: 3 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.