Status

()

defect
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: mstange, Assigned: mstange)

Tracking

Trunk
mozilla1.9.3a1
All
macOS
Points:
---

Firefox Tracking Flags

(status1.9.2 beta4-fixed)

Details

Attachments

(3 attachments, 1 obsolete attachment)

Posted 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?
Posted 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: 10 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.