139989 - [forms.css] Fieldset needs prettier default border

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, enhancement)

Description

[Antti Näyhä]

Currently (in 1.0 RC1), the fieldset element has the following border style in
forms.css:

  border: 2px solid;

...which is rather ugly and inconsistent with other fieldset-supporting
browsers. I would suggest something like this instead:

  border: 2px groove threedface;

Comment 1

[John Keiser (jkeiser)]

-> Form Controls.

Comment 2

[Matthew T (active 1999-2002)]

Exactly. The padding should also be changed from `4px 4px 4px 4px' to `0.7em 
0.67em', which is a scalable equivalent to the `12px 10px' shown in
<http://developer.apple.com/techpubs/mac/HIGOS8Guide/thig-52.html#HEADING52-58>.

Comment 3

[Vidar Haarr (not reading bugmail)]

Attachment: Addresses issue, and also scales padding according to fontsize.

This patch changes the border to be 'groove threedface', and also changes the
padding to use 'em', in order to make it scale according to different
font-sizes.

I just did it quick, and copied the original forms.css to forms2.css, and diff
-wu'ed them.. If that causes a problem, please state so.

Comment 4

[Matthew T (active 1999-2002)]

(Just for the record, my `0.67em' was a miscalculation, and the `0.625em' in 
the patch is correct.)

Comment 5

[Antti Näyhä]

Note that legend has currently pixel-sized padding values as well:

  padding-left: 2px;
  padding-right: 2px;

This should probably be corrected as well while you're at it...

Comment 6

[Bernd]

I don't wan't to be CC'ed on the mpt class bugs ;-), this is UI, I have no clue
of UI design and I dont wan't to have one.

Comment 7

[Vidar Haarr (not reading bugmail)]

Attachment: Removed the ','

This patch removes the , between the paddings.

Comment 8

[Vidar Haarr (not reading bugmail)]

Antti:
That's not padding, that's margin. Your eyes decieve you :)

Margin is the box's distance to the outer content, while padding is the box's
contents distance to the box's edges.
http://www.w3.org/TR/REC-CSS2/box.html#mpb-examples

Did that make sense ? :)

Comment 9

[Antti Näyhä]

Hm, this is what
http://developer.apple.com/techpubs/mac/HIGOS8Guide/thig-52.html#HEADING52-58
has to say about group box titles (legends):

  "There should be at least 3 pixels on each side of the title text."

But at the same time, the legend should be left-aligned with the fieldset contents:
http://developer.apple.com/techpubs/mac/HIGOS8Guide/graphics/HID_L-20.gif

*If* we decide to implement that, just hacking forms.css doesn't seem to be
enough...

Comment 10

[Vidar Haarr (not reading bugmail)]

Antti: Right, this has nothing to do with the heading. This is about the
/content/ of the box. With the change done here, the distance from the content
to the box's edges will be dynamic, based on the font size.

Comment 11

[Axel Hecht]

Comment on attachment 80928 [details] [diff] [review]
Removed the ','

r=[e-mail]

Comment 12

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 80928 [details] [diff] [review]
Removed the ','

yeah!

sr=[e-mail]

Comment 13

[Axel Hecht]

reassigning to Mabus

Comment 14

[Vidar Haarr (not reading bugmail)]

Fix checked into trunk by [e-mail].
Marking RESOLVED FIXED.

Comment 15

[Christopher Aillon (sabbatical, not receiving bugmail)]

This bug was reported against an RC1 (branch) build.  I suggest re-opening this
bug, and requesting driver approval... Cosmetic fixes like this should go in to
releases if it is going to be the default down the line... A screenshot of
before and after would be nice also.  :)

Comment 16

[Vidar Haarr (not reading bugmail)]

Reopening, email on the way to [e-mail] requesting approval.
Lets cross our fingers.

Comment 17

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Have you tested that the LEGENDs are still positioned correctly relative to the
fieldset at different font sizes?  (i.e., that there aren't any hardcoded sizes
in the fieldset/legend code that the change to the padding would mess up?) 
Testcases attached to the bug would be nice.

Comment 18

[Vidar Haarr (not reading bugmail)]

No, I haven't, actually. But I won't get to this untill Monday, at the earliest.
If someone else could provide testcases, that would be very appreciated.

If not, I'll attach some as soon as I can.

I just took a brief look through the code in forms.css, and it seems the dynamic
padding will not affect the legend elements padding. But then again, I could be
wrong.

In fact, I don't know the correct behavior.. should the legend element change
padding (ie, follow the fieldset element), or should it stay put as the fieldset
font-size change ?

Comment 19

[Antti Näyhä]

Attachment: Screenshot before

Comment 20

[Antti Näyhä]

Attachment: Screenshot after

Comment 21

[Antti Näyhä]

Attached "before" and "after" screenshots, taken on 1.0 RC1, Win32, small fonts,
default settings.

Comment 22

[Antti Näyhä]

Attachment: Test case

Comment 23

[Antti Näyhä]

> Have you tested that the LEGENDs are still positioned correctly relative to
> the fieldset at different font sizes?

That was my exact concern in comment #5...  But it seems that legends are not
positioned in any meaningful way to begin with (see the "before" screenshot), so
our patch doesn't really make things worse.

As I stated,
http://developer.apple.com/techpubs/mac/HIGOS8Guide/graphics/HID_L-20.gif has a
nice spec for this, but implementing that would probably require some changes to
the fieldset rendering code.

Anyway, I feel that the legend issue can be taken to another bug.

Comment 24

[Vidar Haarr (not reading bugmail)]

Great work, Antti !
I have emailed [e-mail] now, and we will hopefully get a review soon.

Sorry, I just re-read comment #5, and realize that my response in comment #8
really had nothing to do with your comment :) sorry. I thought you were talking
about the fieldset element.

Comment 25

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 80928 [details] [diff] [review]
Removed the ','

a=dbaron for 1.0 branch checkin

Comment 26

[Vidar Haarr (not reading bugmail)]

Patch 80928 checked into the 1.0 branch by [e-mail]

Marking RESOLVED FIXED.

Comment 27

[Terri Preston]

Verified Mac OS X branch build 2002052803, Win 2k branch build 2002052808 and
linux branch branch build 20020522