Closed Bug 507962 Opened 15 years ago Closed 15 years ago

Widget margin cleanup

Categories

(Toolkit :: Themes, defect)

All
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a1
Tracking Status
status1.9.2 --- beta4-fixed

People

(Reporter: mstange, Assigned: mstange)

Details

Attachments

(3 files, 1 obsolete file)

Attached patch v1 (obsolete) — Splinter Review
UL authors seem to like to put buttons and menulists next to each other in a stretching hbox, or on top of each other in deck. Right now, that leads to the menulist being stretched vertically because the button has such a large vertical margin.
In bug 473233, I fixed one occurence of this by adding an align="center", but there are more occurences of the same pattern in Firefox code (e.g. in the language preferences window), and extensions also do it.
So I thought that we should just support this use case in the theme by making all stretching widgets have the same margin box height. I've decided on 30px, which is 4px margin-top + 22px height + 4px margin-bottom. (That's the "regular" font size case; with the small font size, it's 2 or 3 pixels less height.)

And while I was fixing the margins, I thought it would be great if these margins also resulted in proper text baselines. That's why I ended up with asymmetric margins in some cases, e.g. buttons have 5px top margin and 3px bottom margin.

Details on these things are in the tests.
Attachment #392205 - Flags: review?(dao)
(with the patch it's completely white, of course)
> filefield {
>-  margin: 2px 4px 2px 27px;
>+  margin: 4px 4px 4px 27px;
>   -moz-appearance: textfield;
> }
> 
> .fileFieldContentBox {
>-  background-color: -moz-Dialog;
>+  margin: -3px;
>+  background-color: rgba(230, 230, 230, 0.6);
>   color: -moz-DialogText;
>-  padding: 0px 0px 0px 3px;
>+  padding: 2px 3px 2px 5px;
> }

This lacks rtl support.

> menulist[editable="true"] {
>   -moz-appearance: menulist-textfield;
>-  margin: 2px 4px;
>+  margin: 4px 2px 4px;
> }

> radio {
>   -moz-appearance: radio-container;
>   -moz-box-align: center;
>-  margin: 2px 2px 4px;
>+  margin: 4px 2px 4px;
>   -moz-user-focus: ignore;
> }

margin: 4px 2px;

> #viewGroup > * {
>   -moz-box-orient: vertical;
>   -moz-box-align: center;
>   -moz-appearance: none;
>   font: menu;
>   text-shadow: rgba(255, 255, 255, 0.4) 0 1px;
>   margin: 0;
>-  padding: 1px 4px 0;
>+  padding: 0 4px;

How does that change relate to this bug?
Attached patch v2Splinter Review
(In reply to comment #3)
> > #viewGroup > * {
> >   -moz-box-orient: vertical;
> >   -moz-box-align: center;
> >   -moz-appearance: none;
> >   font: menu;
> >   text-shadow: rgba(255, 255, 255, 0.4) 0 1px;
> >   margin: 0;
> >-  padding: 1px 4px 0;
> >+  padding: 0 4px;
> 
> How does that change relate to this bug?

I gave labels 1px more margin-top, so I need to reduce the padding-top by 1px here. Maybe it would be better to explicitly override the label margins in viewbuttons.css - do you want me to do that?

Fixed the other comments.
Attachment #392205 - Attachment is obsolete: true
Attachment #392367 - Flags: review?(dao)
Attachment #392205 - Flags: review?(dao)
Hardware: x86 → All
Comment on attachment 392367 [details] [diff] [review]
v2

> .dialog-button[dlgtype="help"] {
>   min-width: 1px;
>   padding: 0;
>   -moz-appearance: none;
>   -moz-box-align: start;
>   height: 24px;
>   width: 24px;
>-  margin: 4px;
>+  margin: 3px 4px; 
> }

you've added a trailing space

> .fileFieldContentBox {
>-  background-color: -moz-Dialog;
>+  margin: -3px;
>+  background-color: rgba(230, 230, 230, 0.6);
>   color: -moz-DialogText;
>-  padding: 0px 0px 0px 3px;
>+  padding: 2px 3px 2px;
>+  -moz-padding-start: 5px;
> }

This should be:

  padding: 2px 3px;
  -moz-padding-start: 5px;

or:

  padding: 2px;
  -moz-padding-start: 5px;
  -moz-padding-end: 3px;

or:

  padding-top: 2px;
  padding-bottom: 2px;
  -moz-padding-start: 5px;
  -moz-padding-end: 3px;

I'm leaning towards the second or third version, the shortest one isn't necessarily the clearest.

>-.small-margin {
>-  margin: 1px 2px;
>-}

While you're at it, please remove class="small-margin" from fonts.xul? It doesn't make sense the way it's used.

> menulist[editable="true"] {
>   -moz-appearance: menulist-textfield;
>-  margin: 2px 4px;
>+  margin: 4px 2px 4px;
> }

margin: 4px 2px;
Attachment #392367 - Flags: review?(dao) → review+
I went with the third version.

http://hg.mozilla.org/mozilla-central/rev/a016bdf3dd06
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
Attachment #392367 - Flags: approval1.9.2?
Attachment #392367 - Flags: approval1.9.2? → approval1.9.2+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: