Build 2001-07-27-branch on WinNT and Linux The size attribute is set to 7 but it shows only 6 characters.

I cannot reach "bubblegum.mcom.com" from outside Netscape. Please, attach the testcase to the bug report. Thanks.

Reassigning to form controls.

Without seeing the testcase, I believe that this is a dupe of bug 44656. See also bug 92446 and bug 49883 (both marked dupes of 44656).

the testcase looks like this (wrapped with valid html): <form action="http://wetnap/cgi-bin/echo-form.cgi" method="post"> This input field has the size attribute set to 7: <input size=7 type=text > </form>

With the attached testcase I see 7,5 characters. WFM, 2001-08-01-03 on Windows 98 SE.

Tested on build ID 2001-09-19-05-0.9.4 with attached test case, I see 7 characters, on windows 2000 service pack 1.

see the attached test case. I have set size="15" on MacOS 10 - build ID: 2001-09-28-04-0.9.4 ---- I see 13 characters on win2k - build ID: 2001-09-28-05-0.9.4 ---- I see 14 characters on linux 7.1 - build ID: 2001-09-28-04-0.9.4 ---- I see 15 charcters (works on this platform)

This is pretty much the way it will work now

Are any of these sufficiently simmilar to warrant consolidation via duplication or dependancy change? bug 33654 bug 52500 bug 92980 bug 103293 bug 109392 -matt

Build:2002011103 mac OSX mac 9.04 I am only getting 13 characters in the form field To reproduce: 1. Open NS browser to HTML FORMS controls matrix 2. under control type:> Input text 3. select input text-size.html 4 follow test case result will be 13 chars visible inside form instead of the requested 15

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

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

I spent a whole half hour searching for this bug, since I knew it was here. Please adjust the subject to be more searchable - including the html tag might help. Thanks. Max.

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

Maybe we should use GetMaxCharWidth() intead of GetAveCharWidth. I looked at nsTextControlFrame.cpp and can't figure out why we use GetAveCharWith(). It seems that IE is using GetMaxCharWidth().( the input width in IE is always equals a upper-case string's width whild greater than a lower-case string's width) In windows: since there is a GetMaxCharWidth available, we can just use it. For unix: Since there is no GetMaxCharWidth() in nsIFontMetrics & nsRenderContext. Maybe we can simulate one, e.g using char 'X' width.

this is a patch for discussion. only for unix. ( i still dont have a windows build env. :-) patch in windows maybe like this: diff -u -r3.88 nsTextControlFrame.cpp --- layout/html/forms/src/nsTextControlFrame.cpp 7 Aug 2002 00:37:04 -0000 3.88 +++ layout/html/forms/src/nsTextControlFrame.cpp 19 Aug 2002 01:38:59 -0000 @@ -1675,7 +1675,7 @@ // and then this if def removed. We are too close to RTM to implement it in all // the platforms and ports. #if defined(_WIN32) || defined(XP_OS2) - fontMet->GetAveCharWidth(charWidth); + fontMet->GetMaxCharWidth(charWidth); // Get frame font const nsFont * font = nsnull;

I think #17 is quite reasonable. IE use max width instead of avai width.

ftang: Can you look at the bug? i think GetMaxCharWidth is better here. If so, what is the best way to simulate a GetMaxCharWidth in unix platform? i can't find such func in nsIFontMetrics/nsIRenderingContext. A hacking method is using the width of 'X' or sth like it. can u give some suggestions?

I would use 'M' instead of 'X' since it can be wider in some fonts. cc kin in case he has some ideas/input/feedback

I was guessing when I used GetAve, I am fine with GetMax if we get closer to IE.

to brade: yes, u r right. 'M' is always the widest. i think using 'M'/'X' width as a pseudo ave char width is the best method. rational: 1. the max char from GetMaxAdvance is very great, almost 2 times of GetAve. using it will make input control very very long. 2. the ave char from GetAve is too little, make input control not wide enough to contain upper-case letters 3. in first look, using 'M'/'X's width is very hacky, but in fact it's not. what we need is a pseudo char width which is greater than most frequently used character( A/../Z/a/../z ) and is not too great so text input would leave much white space. 'M'/'X's width, i belive, is the value. i have wrote a patch, rods, can u review it? BTW, it seems that ie5 use GetAve but ie6 don't. i guess ie6 is using 'A'. Absolutely.:-)

using 'M's width as pseudo ave char width

Comment on attachment 96136 [details] [diff] [review] using 'M's width as pseudo ave char width Dumb question: What happens if the font being used does not define the 'M' character and the matching glyph as zero width/height ?

i'm not familiar with font/character code issues. Can u give an example or textcase? if it does exists, we can add some code.

This width detector will show the width of each textbox when you mouse over it. In IE on my machine the formula for textbox width is (23 + 6*size), for us it is (13 + 8*size). That's just my machine, with my fonts. I don't really think either of us has it right. Especially if it's a fixed width font, we *always always* want to use (charwidth*size) where charwidth is the width of one character. In the case of variable width is when we change.

-->

Rick, what's up here? We can't use M's, that makes things way too big. Something that gets us closer to IE but still makes us bigger than we are would be acceptable. We also need to avoid being bigger than IE where possible, but it's not mandatory.

1. As we have known, for ie5: its fomula is : charWidth = GetAveCharWidth(); internalPadding = charWidth-4*pixelWidth inputWidth = internalPadding+charWidth*size mozilla has used this for a long time and its result is almost same as IE5. but it seemed that ie6 changed sth. I **guess** that in IE6: interalPadding is now: interalPadding = GetMaxAdvance() +(|-) sth; but still don't have time to test it. i will do it today or tommrrow. 2. another problem in *nix: in *nix, we don't add internalPadding like in windows. so the computed width is too short. we should fix it too. sorry for late response. a little busy these days. but i will spent more time on it from tomorrow.

1. My guess is right. with the attached patch, running the testcase from jkeiser, the caculated width is SAME as IE6. It's great. (on win2k pro english. westernEurope(ISO) encoding) 2. i also change the algorithm in *nix. test, suggestion and review welcomed

Comment on attachment 98392 [details] [diff] [review] proposed patch Yay! Good patch. r=jkeiser with a few minor changes: Could you move the definition of charMaxAdvance down to where it's being used? Also, change that comment from "take the size" to "take the maximum character width". And even though it wasn't your fault, please change "internalPadding%NSToCoordRound(p2t)" to "internalPadding % NSToCoordRound(p2t)" and change if( rest < NSToCoordRound(p2t) - rest) { to if (rest < NSToCoordRound(p2t) - rest) { Upload that patch and I'll r= it. Thanks!

Comment on attachment 98547 [details] [diff] [review] changed according to jkeiser's comments r=jkeiser Missed a teeny bit of whitespace in that last if() but I can change that at checkin.

I'd like rods to see this. He spent a long time tuning our form control width to not be much greater than IE's (which NS4's _is_).

This patch is in the "standard" calculation which is where we try to size close to IE, so if the patch comes closer or is an exact match compared to IE. I say use it. In fact, I don't think we ever use the Nav4 quirks mode sizing anymore and it can probably be removed. r=rods (if it works)

sr=?

Comment on attachment 98547 [details] [diff] [review] changed according to jkeiser's comments -- This comment should probably read like "Still not sure if Font Metrics are implemented on all platforms.": + //XXX Still Not assure that Font Metrics has been implemented on all platforms + NS_ASSERTION( fontMet, "No fontMetrics"); -- If |fontMet| is ever null, we will crash because fontMet will be dereferenced when calling |GetAveCharWidth()| and |GetMaxAdvance()|. We should either return with a failure before it is used, or perhaps wrap the code that uses it to check before dereferencing it and default to some reasonable values if it is not present. For example we could probably re-work the WIN32/OS2 code like this: #if defined(_WIN32) || defined(XP_OS2) if (fontMet) { fontMet->GetAveCharWidth(charWidth); } else #endif { <The xp method of calculating charWidth> } And wrap the code you moved that calculates |internalPadding| with a check for |fontMet|.

Good catch. I actually like the idea of NS_ENSURE_TRUE(fontMet, "No font metrics!");. I think we're safe doing that, since these spots would crash if platforms did not have font metrics: http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsLineLayout.cpp#2408 http://lxr.mozilla.org/seamonkey/source/layout/mathml/base/src/nsMathMLmfencedFrame.cpp#282 http://lxr.mozilla.org/seamonkey/source/layout/mathml/base/src/nsMathMLmtableFrame.cpp#559 Many other spots look like they would exhibit weird behavior in that case, but they would not crash. Kin, if you are OK with this, I'm willing to watch ports and fix if it goes orange.

Comment on attachment 98547 [details] [diff] [review] changed according to jkeiser's comments It looks like it would be good to attach a patch that is against the most recent version of nsTextControlFrame.cpp, since there have been some changes in this area recently. A new patch should also respond to kin's comments.

thanks. kin, john, david, maybe we can say there is a font metrics on all platforms now? if so, NS_ASSERTION is still better. or adding a default value for not debug like this? ... if( .. && fontMet ){ Fontmet->GetAveWidth(charWidth); // we still dont have GetAve on all flatforms. but just wait a minute FontMet->GetMaxAdvance(charMaxAdvance); else{ NS_ASSERTION( fontMet, "No fontMetrics"); aRendContext->GetWidth('x', charWidth); aRendContext->GetWidth('X', charMaxAdvance); } ... now no need to check fontMet since it won't be used elsewhere. this will be more OK when we get GetAve on all platforms ( #50998 ) we can fix #50998 first.

have submitted a patch for #50998

From #50998 i will change it according to rds's comment. Additional Comment #20 From rbs@maths.uq.edu.au 2002-09-17 02:26 ------- There is a remaining cleanup of #if defined(_WIN32) || defined(XP_OS2) in mozilla/layout/html/forms/src/nsGfxTextControlFrame.cpp While there, also saw a code that computes 'NSToCoordRound(p2t)' too many times (caming from the patch in bug 99948): // round to a multiple of p2t nscoord rest = internalPadding%NSToCoordRound(p2t); if( rest < NSToCoordRound(p2t) - rest) { internalPadding = internalPadding - rest; } else { internalPadding = internalPadding + NSToCoordRound(p2t) - rest; } While you are there, mind cleaning up that code a little bit, e.g., use: nscoord t = NSToCoordRound(p2t); nscoord rest = internalPadding % t; if( rest < t - rest) { // same as rest < half-of-pixel internalPadding -= rest; // round down } else { internalPadding += t - rest; // round up }

add depend on 50998. And i will write another patch which can be checked in after 50998 is fixed.

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

all-in-one: 1. the #50998 has got r= and sr=, this patch use GetAve on all platforms 2. clean up the NSToCoordRound() code according to rbs' commnent (#43) 3. using GUESS_AVE_CHAR_width... as default value for font metrics missing case

Comment on attachment 99654 [details] [diff] [review] new proposed Please get rid of the guessing. Instead of guessing and doing that if(), do NS_ENSURE_SUCCESS(rv, rv); http://lxr.mozilla.org/seamonkey/source/layout/html/base/src/nsTextFrame.cpp#56 8 shows that FontMetrics always exists so that even ENSURE_SUCCESS is not strictly necessary--but better to be safe anyway, and check NS_ENSURE_SUCCESS.

y. agree that there is FontM on all platforms. but it is still possibile that the GetFrameFontFM() may fail?

john, do you mean: ... if ( ... && fontMet ) {...} else{ NS_ENSURE_SUCCESS(fontMet, fontMet); ... } or sth else? if we do think we should take care the possibility of GetFrameFontFM failure, using a default value maybe better. if we think the GetFrameFontFM will never fail, we can...we dont need an else{} at all.

Well, the only way GetFontMet can fail is: (a) there is no font for this frame (in which case we can't render) (b) there is no font metrics for this frame (in which case we can't render) So we should simply do: // get leading nsCOMPtr<nsIFontMetrics> fontMet; nsresult rv = nsFormControlHelper::GetFrameFontFM(aPresContext, this, getter_AddRefs(fontMet)); NS_ENSURE_SUCCESS(rv, rv); aRendContext->SetFont(fontMet); fontMet->GetHeight(fontHeight); This guessing stuff is just a bad idea in general.

hm..maybe u r right. it should be ok now.

Comment on attachment 99803 [details] [diff] [review] is it the last one? :) r=jkeiser Yes, it's the last one for me anyway :) Woohoo!

The last I knew GetAveCharWidth was not implementd on all platforms. And it is being used here: aDesiredSize.width = GetCols() * charWidth;

Comment on attachment 99803 [details] [diff] [review] is it the last one? :) sr=kin@netscape.com as long as *all* platforms now implement GetAveCharWidth(). Looking at bug 50998, I think this might be true now. I did notice that some of the patches in bug 50998 use the width of 'x' as the average char width, so text widgets may be sized differently from the XP average char width code being removed here, so if that's ok with everyone (and rods) that's fine with me. Minor formatting nits: ==== Put in a return before the commment, so it stands out: + nscoord charWidth = 0; + nscoord charMaxAdvance = 0; + // Get leading and the Average/MaxAdvance char width nsCOMPtr<nsIFontMetrics> fontMet; ==== Remove the extra space after the 't': + if (rest < t - rest) {

rods, kin: see bug 50998, which Rick was gracious enough to fix as well. Any platform that does *not* implement GetAveCharWidth will no longer link--and there is no new orange on ports.

Thanks all of you. In fact, #92980 is my first bug here. So all your helps are very appreciated.

This patch includes Kin's nits and also makes it compile :) NS_ENSURE_SUCCESS(rv, rv) doesn't work unless the function returns nsresult, which it really should in this case.

Fix checked in.

OK, let's try uploading the right patch again

tested attachment 51340 [details] .... got the following ... on winXP ----------- seeing upto 15 visible charaters in the textbox on linux ------------ seeing upto 14 visible characters in the text box on macos 10.1 ------- seeing upto 16 characters in the text box. i suppose , this bug needs some more work. re-opening

Madhur, are those lowercase or uppercase? What exact string did you use? We should be fitting 15 lowercase average-width chars (eg 15 'w' chars may not fit) in that textbox. But there are easily created char combination (eg all capital 'W') that would not show 15 chars. The idea is to look reasonable in most cases, not make the form controls wider than IE, and curse the variable-width font we use for form controls....

Agreed. If this bug were "size=n should always show n characters," we would probably mark wontfix due to compatibility, but I think it's better to say this bug is "make the textbox a bit bigger to fit more characters and aid compatibility." As it stands, the Width Detector shows that the boxes are identical in size to IE on WinXP. On other platforms we use the same algorithm, but the fonts are different, which probably accounts for the discrepancies you see.

Somewhat off-topic, but I wonder if there is there a way to specify a fixed width font? Max.

tried a variety of strings ... number string == "123456789012345" on winXP ----------- seeing upto 15 visible charaters in the textbox "123456789012345" on linux ------------ seeing upto 14 visible characters in the text box "12345678901234" on macos 10.1 ------- seeing upto 16 characters in the text box "1234567890123456" lowercase string == "abcdefghijklmnopqrst" on winXP ----------- seeing upto 16 visible charaters in the textbox "abcdefghijklmnop" on linux ------------ seeing upto 17 visible characters in the text box "abcdefghijklmnopq" on macos 10.1 ------- seeing upto 19 characters in the text box "abcdefghijklmnopqrs" uppercase string == "ABCDEFGHIJKLMNOP" on winXP ----------- seeing upto 13 visible charaters in the textbox "ABCDEFGHIJKLM" on linux ------------ seeing upto 13 visible characters in the text box "ABCDEFGHIJKLM" on macos 10.1 ------- seeing upto 14 characters in the text box "ABCDEFGHIJKLMN" uppercase + lowercase string == "AaBbCcDdEeFfGgHhIiJj" on winXP ----------- seeing upto 13 visible charaters in the textbox "AaBbCcDdEeFfG" on linux ------------ seeing upto 13-14 visible characters in the text box "AaBbCcDdEeFfGg" on macos 10.1 ------- seeing upto 14 characters in the text box ""AaBbCcDdEeFfGg"

Max: you just set the font-family appropriately.

got that... verifing based on comment 62