Closed
Bug 139989
Opened 23 years ago
Closed 23 years ago
[forms.css] Fieldset needs prettier default border
Categories
(Core :: Layout: Form Controls, enhancement)
Core
Layout: Form Controls
Tracking
()
VERIFIED
FIXED
People
(Reporter: sairwas, Assigned: vhaarr+bmo)
Details
(Keywords: html4, polish)
Attachments
(5 files)
300 bytes,
patch
|
Details | Diff | Splinter Review | |
299 bytes,
patch
|
axel
:
review+
bugs
:
superreview+
dbaron
:
approval+
|
Details | Diff | Splinter Review |
2.71 KB,
image/png
|
Details | |
2.94 KB,
image/png
|
Details | |
709 bytes,
text/html
|
Details |
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•23 years ago
|
||
-> Form Controls.
Assignee: dbaron → rods
Component: Style System → HTML Form Controls
QA Contact: ian → tpreston
Comment 2•23 years ago
|
||
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>.
Keywords: polish
Assignee | ||
Comment 3•23 years ago
|
||
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•23 years ago
|
||
(Just for the record, my `0.67em' was a miscalculation, and the `0.625em' in
the patch is correct.)
Reporter | ||
Comment 5•23 years ago
|
||
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...
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.
Assignee | ||
Comment 7•23 years ago
|
||
This patch removes the , between the paddings.
Assignee | ||
Comment 8•23 years ago
|
||
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 ? :)
Reporter | ||
Comment 9•23 years ago
|
||
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...
Assignee | ||
Comment 10•23 years ago
|
||
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•23 years ago
|
||
Attachment #80928 -
Flags: review+
Comment 12•23 years ago
|
||
Attachment #80928 -
Flags: superreview+
Assignee | ||
Comment 14•23 years ago
|
||
Fix checked into trunk by axel@pike.org.
Marking RESOLVED FIXED.
Status: NEW → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Comment 15•23 years ago
|
||
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. :)
Assignee | ||
Comment 16•23 years ago
|
||
Reopening, email on the way to drivers@mozilla.org requesting approval.
Lets cross our fingers.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
Assignee | ||
Comment 18•23 years ago
|
||
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 ?
Reporter | ||
Comment 19•23 years ago
|
||
Reporter | ||
Comment 20•23 years ago
|
||
Reporter | ||
Comment 21•23 years ago
|
||
Attached "before" and "after" screenshots, taken on 1.0 RC1, Win32, small fonts,
default settings.
Reporter | ||
Comment 22•23 years ago
|
||
Reporter | ||
Comment 23•23 years ago
|
||
> 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.
Assignee | ||
Comment 24•23 years ago
|
||
Great work, Antti !
I have emailed drivers@mozilla.org 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 on attachment 80928 [details] [diff] [review]
Removed the ','
a=dbaron for 1.0 branch checkin
Attachment #80928 -
Flags: approval+
Assignee | ||
Comment 26•23 years ago
|
||
Patch 80928 checked into the 1.0 branch by timeless@mac.com
Marking RESOLVED FIXED.
Status: REOPENED → RESOLVED
Closed: 23 years ago → 23 years ago
Resolution: --- → FIXED
Keywords: fixed1.0.0
Comment 27•23 years ago
|
||
Verified Mac OS X branch build 2002052803, Win 2k branch build 2002052808 and
linux branch branch build 20020522
Status: RESOLVED → VERIFIED
Keywords: fixed1.0.0 → verified1.0.0
You need to log in
before you can comment on or make changes to this bug.
Description
•