Widget margin cleanup

RESOLVED FIXED in mozilla1.9.3a1

Status

()

Toolkit
Themes
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: mstange, Assigned: mstange)

Tracking

Trunk
mozilla1.9.3a1
All
Mac OS X
Points:
---

Firefox Tracking Flags

(status1.9.2 beta4-fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

8 years ago
Created attachment 392205 [details] [diff] [review]
v1

XUL 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)
(Assignee)

Comment 1

8 years ago
Created attachment 392206 [details]
appearance of the baseline test without patch

(with the patch it's completely white, of course)
(Assignee)

Comment 2

8 years ago
Created attachment 392207 [details]
screenshot of the nostretch test without patch
> 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?
(Assignee)

Comment 4

8 years ago
Created attachment 392367 [details] [diff] [review]
v2

(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)
(Assignee)

Updated

8 years ago
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+
(Assignee)

Comment 6

8 years ago
I went with the third version.

http://hg.mozilla.org/mozilla-central/rev/a016bdf3dd06
Status: ASSIGNED → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a1
(Assignee)

Updated

8 years ago
Attachment #392367 - Flags: approval1.9.2?
Attachment #392367 - Flags: approval1.9.2? → approval1.9.2+
Comment on attachment 392367 [details] [diff] [review]
v2

a192=beltzner
(Assignee)

Comment 8

8 years ago
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/bf75b0252dc4
status1.9.2: --- → final-fixed
You need to log in before you can comment on or make changes to this bug.