53744 - select default font-family does not match other html form controls

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect, P1)

Description

[James Lariviere]

DESCRIPTION:

select renders with different font-family than other html form controls.


STEPS TO REPRODUCE:

View mozilla bug query page http://bugzilla.mozilla.org/query.cgi.

Compare the font rendered for select elements to input[type="text"]

ACTUAL RESULTS:

Select element is rendered in a sans-serif font while the input[type="text"] are
rendered in a monospace font.

EXPECTED RESULTS:

All form elements should render with at least the same font-family, font-size.

I reported a similar Bug (#42825).  Over the past few builds make this html form
bug very obvious.


DOES NOT WORK CORRECTLY ON:

Win98 build 2000092205 (9-21-00 nightly build)

Comment 1

[rods (gone)]

All pages now layout incorrectly because the form controls are too large.

This can be corrected with removing line of code.

Here is the diff:
Index: nsCSSStyleRule.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/html/style/src/nsCSSStyleRule.cpp,v
retrieving revision 3.123
diff -u -r3.123 nsCSSStyleRule.cpp
--- nsCSSStyleRule.cpp  2000/09/21 10:21:23     3.123
+++ nsCSSStyleRule.cpp  2000/09/22 18:48:58
@@ -1681,7 +1681,6 @@
               case eSystemAttr_Font_Button:
               case eSystemAttr_Font_List:
                 font->mFont.name.AssignWithConversion("sans-serif");
-                font->mFont.size = defaultFont.size;
                 break;
             }
 #endif

Comment 2

[karnaze (gone)]

Marking nsbeta3+, since this is a serious recent regression.

Comment 3

[Marc Attinasi]

Yea, I noticed that the fonts were way too big on selects too. I see that you
are now going back to using the DeviceContext-specified size for the font, and
that makes sense, but I'm worried that the patch only affects Win. What about
Mac and Linux? Do we want the Select font to be the default-font size on one
platform and not another? Pierre, do we really want to apply the default font
size to the system fonts for lists and buttons at all?

Comment 4

[rods (gone)]

I removed the font size setting in Linux and it didn't seem to make a diference.

Comment 5

[James Lariviere]

rod,

Can you also please make sure that the font-family matches the other form
controls.  I think all other form controls render with monospace exept for the
<select> (sans-serif). I think the select should match the html form elements
(use monospace).

Comment 6

[rods (gone)]

As it turns out Pierre was doing the correct thing. You want to use the correct 
variable width font size but it has to be adjusted by 2 points which is what Nav 
4.x does. So instead of removing the line we just need to supbtract 3 points (in 
twips) from the font size:

Index: nsCSSStyleRule.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/html/style/src/nsCSSStyleRule.cpp,v
retrieving revision 3.123
diff -u -r3.123 nsCSSStyleRule.cpp
--- nsCSSStyleRule.cpp  2000/09/21 10:21:23     3.123
+++ nsCSSStyleRule.cpp  2000/09/22 22:05:23
@@ -1681,7 +1681,7 @@
               case eSystemAttr_Font_Button:
               case eSystemAttr_Font_List:
                 font->mFont.name.AssignWithConversion("sans-serif");
-                font->mFont.size = defaultFont.size;
+                font->mFont.size = defaultFont.size - NSIntPointsToTwips(2);
                 break;
             }
 #endif


This is a big win, because of Pierre's checkin the form controls now obey the 
Pref setting (in NavQuirks mode) and this fix makes that final adjustment (a 
minor oversight by Pierre) so that they have the correct size.

Comment 7

[rods (gone)]

Oops, in the comment before this I meant to always mean 2 points not 3

Comment 8

[rods (gone)]

fixed

Comment 9

[Daniel Bratell]

What if defaultFont.size is less than 2 points? Then you get a negative size. 
Can that crash anything elsewhere?

Comment 10

[Blake Ross]

This should also be fixed in the branch.  Has it been, or do we need to appeal 
to PDT?

Comment 11

[James Lariviere]

It looks like the sizing problem has been fixed in build win98 2000092420.

However, the select control still renders in a sans-serif font while all other
html form controls render with a monospace font.

I want to  suggest that if you change in the html.css, the font-family for
select to "field" instead of "list" or map the keyword "list" to match whatever
font-family "field" is matched to, you then will get a consistant font-family
rendering along all form controls.

Comment 12

[rods (gone)]

No, look at Nav 4.x, it renders sans-serif for selects and monospace for the 
other controls. 

And No this didn't make it into the branch and it needs to.

Comment 13

[James Lariviere]

Rod,

I have to ask so I will not waste your/my time on this issue anymore...

Do you think it is fine that all form elements are rendered in a monospace font
except the select element (ignoring NAV4 since there are so many things NAV4
rendered wrong that are corrected in mozilla)?

Comment 14

[Peter Trudelle]

PDT marking nsbeta3-, unclear how bad it is now, nominate for rtm if 
appropriate.

Comment 15

[rods (gone)]

I need this bug for plus for nsbeta3 or RTM. The fix is a one liner for Windows 
platform only with zero risk:

nsCSSStyleRule.cpp line 1678

Old line:
        font->mFont.size = defaultFont.size;
New line:
        font->mFont.size = PR_MAX(defaultFont.size - NSIntPointsToTwips(2), 0);

This fix was checked into the trunk and just missed the branch (by an hour or 
two). I have spent 3 years trying to get form controls to size correctly and 
without this fix in the branch EVERY single page with comboboxes or button will 
not layout correctly because the form control will size to font point sizes too 
big.

I'll add the diff once I have a good branch tree.

Comment 16

[buster]

PDT: I recommend we get this in for RTM.  Very low risk, high value.  Fixes 
layout of a bunch of pages.  

Comment 17

[ekrock's old account (dead)]

PDT: I also recommend that we accept this either for nsbeta3 or, failing that, 
for rtm. Reasons: (1) HTML forms are ubiquitous on the web, particularly at 
consumer e-commerce and service sites that are frequented by the intermediate 
users who are the target of this first product release, so this is critically 
important backward compatibility. The problem is expected to affect many pages 
on the web if not fixed. (2) E-commerce sites are quite sensitive about the 
branding, UE, and look and feel of their pages, so we will likely negatively 
affect many key partners if we don't fix this. (3) The fix has already been 
checked in on the trunk and we can use that to make sure it's not causing 
problems. (4) Rod really had this done in time and checked in on the trunk and 
just missed the branch by a few hours; let's use good judgment and Do The Right 
Thing rather than mindlessly enforcing an arbitrary cutoff point by hours 
without assessing the cost-benefit of accepting the patch.

Comment 18

[Erik van der Poel]

PDT, if you view the Bugzilla query page with a recent branch build, you will
quickly see why it is so important to get this fix into the branch. The fix has
been cooking on the trunk for a few days, and the trunk builds are better
because of this. Please allow this simple and safe fix to be applied to the
branch.

Comment 19

[rods (gone)]

Here is the official diff:
Index: nsCSSStyleRule.cpp
===================================================================
RCS file: /cvsroot/mozilla/layout/html/style/src/nsCSSStyleRule.cpp,v
retrieving revision 3.123
diff -u -r3.123 nsCSSStyleRule.cpp
--- nsCSSStyleRule.cpp  2000/09/21 10:21:23     3.123
+++ nsCSSStyleRule.cpp  2000/09/26 20:09:08
@@ -1681,7 +1681,7 @@
               case eSystemAttr_Font_Button:
               case eSystemAttr_Font_List:
                 font->mFont.name.AssignWithConversion("sans-serif");
-                font->mFont.size = defaultFont.size;
+                font->mFont.size = PR_MAX(defaultFont.size - NSIntPointsToTwips
(2), 0);
                 break;
             }
 #endif

Comment 20

[Phil Peterson]

rtm++

Comment 21

[bsharma]

Updating QA contact.

Comment 22

[rods (gone)]

*** Bug 53617 has been marked as a duplicate of this bug. ***

Comment 23

[rods (gone)]

*** Bug 54573 has been marked as a duplicate of this bug. ***

Comment 24

[Kevin McCluskey (gone)]

Marking nsbeta3- since we are past nsbeta3.

Comment 25

[James Lariviere]

WOW, This amazes me that this did not get fixed. :-(

Now every form will be broken that use select and button elements. Everyone.

Comment 26

[Marc Attinasi]

Yea James,I don't think Rod tried hard enough to get this one approved
(presumably because he is a slacker) <g>

Comment 27

[James Lariviere]

Nope, but I'm guessing Rod wanted this one as bad as anyone else.

Can't wait to see what my boss thinks when she sees that these elements are
twice as big as the other form controls.  Then I have to explain it's not my
fault... as always.

I think the only thing worse that could happen for me in mozilla is to take out
:hover.  GD, did that too in the past week!  The only :hover that works is in
some form elements (selects, buttons, radios, and checkboxes).

Thanks Rod, must be your fault again, buddy. :-)

Comment 28

[rods (gone)]

fixed

Comment 29

[rods (gone)]

fixed

Comment 30

[Keyser Sose]

Works for Me
Platform: PC
OS: Windows 98
Mozilla Version: 2000100508

Marking as Verified