Closed
Bug 167001
Opened 22 years ago
Closed 20 years ago
Anything below the baseline goes out of input field
Categories
(Core :: Layout: Text and Fonts, defect)
Tracking
()
RESOLVED
FIXED
Future
People
(Reporter: kazhik, Assigned: smontagu)
References
Details
(Keywords: testcase, topembed-, Whiteboard: [adt3] [ETA Needed])
Attachments
(4 files, 13 obsolete files)
965 bytes,
patch
|
dbaron
:
review-
dbaron
:
superreview-
|
Details | Diff | Splinter Review |
902 bytes,
image/png
|
Details | |
3.37 KB,
patch
|
smontagu
:
review+
dbaron
:
superreview-
|
Details | Diff | Splinter Review |
4.23 KB,
patch
|
dbaron
:
review+
dbaron
:
superreview+
|
Details | Diff | Splinter Review |
Underline goes out of input field. Testcase: http://bugzilla.mozilla.gr.jp/showattachment.cgi?attach_id=1015 Maybe this is a side-effect of the fix for bug 156943.
Comment 1•22 years ago
|
||
This might be a regression introduced by bug 156943, which was taken on the branch to address a concern for a major embeddor. Nominating for topembed.
Comment 2•22 years ago
|
||
I still see the problem even after I disabled the change for bug 156943. Are you sure that this is a regression? Would you mind posting 2 screen shot before and after patch 156943 for comparision? Thanks
Status: NEW → ASSIGNED
Comment 4•22 years ago
|
||
In the case of Win98SE, I add "line-height: 100%;" to the place which defines the style of each attribute of the input element in the \res\forms.css. A text will come to be settled in a box about the following test cases. Isn't this patch considered to be an effective method? Please give me your advice. bug 167001: http://bugzilla.mozilla.gr.jp/showattachment.cgi?attach_id=1015 bug 90884: attachment 42348 [details], attachment 49900 [details], attachment 86782 [details] bug 70019: attachment 26070 [details], attachment 32052 [details] In \res\forms.css on Win98SE or in layout/html/document/src/forms.css: /* default inputs, text inputs, and selects */ input { padding: 1px 0 1px 0; border: 2px inset ThreeDFace; background-color: -moz-Field; color: -moz-FieldText; font: -moz-field; line-height: 100%; <- Addition text-align: start; select { margin: 0; border-color: ThreeDFace; background-color: -moz-Field; color: -moz-FieldText; font: -moz-list; line-height: 100%;¡¡¡¡¡¡¡¡¡¡<- Addition white-space: nowrap; /* buttons */ button, input[type="reset"], input[type="button"], input[type="submit"] { padding: 2px 0 2px 0; border: 2px outset ButtonFace; background-color: ButtonFace; color: ButtonText; font: -moz-button; line-height: 100%; <- Addition white-space: pre;
Comment 5•22 years ago
|
||
Bug 90884 and Bug 82265 are fixed on 20021100604-trunk/WinXP. But, I can reproduce this problem with the build.
Comment 6•22 years ago
|
||
Comment 7•22 years ago
|
||
Since the height of the font based on mMaxHeight with the CalculateSizeStandard() function of layout/html/forms/src/nsTextControlFrame.cpp differs from the line height based on mEmHeight that is asked by calculation, a font moves slightly up and down. With a patch, it is made for the height of the font and the line height to become the same. Since the patch of bug 156943 was also moving the baseline when lang was "ja", it was changed.
Comment 8•22 years ago
|
||
Updated•22 years ago
|
Attachment #110438 -
Flags: review?(shanjian)
Updated•22 years ago
|
Attachment #110438 -
Flags: review?(shanjian) → review+
Comment 10•22 years ago
|
||
A patch for the height of the TEXTAREA field. ftang, review this patch also?
Comment 11•22 years ago
|
||
Comment 12•22 years ago
|
||
When displaying on the TEXTAREA field at one more line, the patch was added so that a vertical scroll bar might not be displayed.
Comment 13•22 years ago
|
||
Comment 14•22 years ago
|
||
Comment on attachment 111188 [details] [diff] [review] patch for a vertical scroll bar The third patch is not bug fix and has become change of specification. Since examination is not fully made, it withdraws.
Attachment #111188 -
Attachment is obsolete: true
Comment 15•22 years ago
|
||
saito- are you signed up to fix this bug? Looks like you are. Please let me know which patch you want me to review when you are ready. Send me mail plesase.
Comment 16•22 years ago
|
||
Comment on attachment 111157 [details] [diff] [review] patch for textarea I am sorry to have made it confusing. The patch of which I want to ask a review is the first patch.
Attachment #111157 -
Attachment is obsolete: true
Updated•22 years ago
|
Status: NEW → ASSIGNED
Comment 17•22 years ago
|
||
I cannot reproduce the problem on the trunk with winxp any more, in all bugzilla text field. I try some japanese site, the input method underline also show up. it looks like the problem is gone. wfm
Comment 18•22 years ago
|
||
No, I can still reproduce this bug with 2003021308-trunk/WinXP. ( http://bugzilla.mozilla.gr.jp/showattachment.cgi?attach_id=1015 )
Comment 19•22 years ago
|
||
I can see the problem in the attached testcases on 02-12 trunk build /WinXP also, re-open.
Status: RESOLVED → VERIFIED
Comment 21•21 years ago
|
||
i18n triage team: nsbeta1+/adt3
Whiteboard: [adt2] [ETA Needed] → [adt3] [ETA Needed]
Comment 22•21 years ago
|
||
i18n triage team: over to Simon for a look.
Assignee: ftang → smontagu
Status: REOPENED → NEW
Component: Internationalization → Layout: Fonts and Text
Keywords: intl
Summary: Underline goes out of input field → Anything below the baseline goes out of input field
Comment 23•21 years ago
|
||
Discussed in topembed bug triage. Minusing.
Comment 25•20 years ago
|
||
I cannot reproduce this bug with this simply patch. Does it have problem?
Comment 26•20 years ago
|
||
Comment on attachment 158414 [details] [diff] [review] patch David Baron: I think that "line-height: normal !important" is mistake of bug 82265. In INPUT element, 'line-height' should be '1', not be 'normal'.
Attachment #158414 -
Flags: review?(dbaron)
Updated•20 years ago
|
Attachment #158414 -
Flags: superreview?(dbaron)
Attachment #158414 -
Flags: superreview?(dbaron)
Attachment #158414 -
Flags: superreview+
Attachment #158414 -
Flags: review?(dbaron)
Attachment #158414 -
Flags: review+
Assignee | ||
Comment 27•20 years ago
|
||
Patch checked in.
Comment 28•20 years ago
|
||
This patch broke Mac build. http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2377&action=view reopening. please back it out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment 29•20 years ago
|
||
I think the Mac build's problem is Mac widget bug. This bug was not reproduced on Mac. I think the reason is the top of editor is not content top on Mac. But on other OS, the top of editor is content top. Mike Pinkerton, Brian Ryner: Can you fix by Mac widget?
Comment 30•20 years ago
|
||
My guess: On mac, if the top of editor is same as content top (not border top), the text may be overlaped bottom-border. Isn't the top raised intentionally?
Comment 31•20 years ago
|
||
The regression on Mac occurs in Japanese locale, not in English locale. That's related to Bug 109176 and Bug 233781. Workaround for Japanese locale on Mac is setting as follows in Mozilla.app/Contents/MacOS/res/platform-forms.css: input { font-family: 'Lucida grande', sans-serif !important; } or input { line-height: normal !important; } BTW, why Mac Mozilla build uses http://lxr.mozilla.org/mozilla/source/layout/html/document/src/win/platform-forms.css instead of http://lxr.mozilla.org/mozilla/source/layout/html/document/src/mac/platform-forms.css ?
Comment 32•20 years ago
|
||
Brian Ryner: Why this patch is for Camino? not for Mac? http://bonsai.mozilla.org/cvsview2.cgi?diff_mode=context&whitespace_mode=show&file=Makefile.in&branch=&root=/cvsroot&subdir=mozilla/layout/html/document/src&command=DIFF_FRAMESET&rev1=1.48&rev2=1.49
Comment 33•20 years ago
|
||
I think that we needs new directory as following. layout/html/document/src/ +- mac +- cocoa <- new directory. +- ... And changing the role. mac ... for MacOS X builds( Mozilla/Firefox ) cocoa ... for Camino who can allow to create new directory?
Comment 34•20 years ago
|
||
For Mac OS X and Camino. But I cannot test it.
Comment 35•20 years ago
|
||
Comment on attachment 158633 [details] [diff] [review] patch for Mac OS X and Camino Brian Ryner: I cannot test this patch. Please test and review it.
Attachment #158633 -
Flags: review?(bryner)
Updated•20 years ago
|
Attachment #158634 -
Flags: review?(bryner)
Updated•20 years ago
|
Attachment #158633 -
Flags: review?(bryner)
Comment 37•20 years ago
|
||
In Bugzilla-jp, attachment 158634 [details] [diff] [review] is works fine. Please review and superreview it.
Updated•20 years ago
|
Attachment #158634 -
Flags: superreview?(dbaron)
Comment 38•20 years ago
|
||
(In reply to comment #26) > (From update of attachment 158414 [details] [diff] [review]) > David Baron: > I think that "line-height: normal !important" is mistake of bug 82265. > In INPUT element, 'line-height' should be '1', not be 'normal'. So should Bug 258889 be invalid then (it was caused by this bug here)? I'm not quite sure if a input field should look like in Attachment 158578 [details] (notice the space between input border and text in trunk build). IMO the text should still be vertically centered, but i let someone else decide.
OK, I'm thinking the original patch is a bad idea in general. Has the original patch been backed out? Perhaps it should be, and we should ensure that input fields are big enough to hold a line at 'line-height: normal', which they seem to be in most cases?
Comment 40•20 years ago
|
||
I think bug 258889 is not invalid. The new bug is new alignment problem. I think the line-height being '1' is correctly.
Comment on attachment 158414 [details] [diff] [review] patch I'm changing my mind about this. 'line-height: normal' should mean that we use the font's recommended spacing, which is what we should be doing. I really didn't like this idea from the start, and I'm not sure why I marked it review+ in the first place.
Attachment #158414 -
Flags: superreview-
Attachment #158414 -
Flags: superreview+
Attachment #158414 -
Flags: review-
Attachment #158414 -
Flags: review+
Comment 42•20 years ago
|
||
comment 39: Wait. Why backout? I want you to think this problem. On our enviroment, the text information is lacked 'normal'. e.x., We cannot understand i and j.
I don't understand comment 42, but I think 'normal' is the correct line-height. However, does this get better if you |#undef FONT_LEADING_APIS_V2| in nsHTMLReflowState.cpp (after the include of nsIFontMetrics.h)?
Original patch backed out.
Comment 45•20 years ago
|
||
See this screenshot. If the patch is backedout, we cannot understand j and y. Because decent is cutted.
I'm aware of that, but it's the wrong fix. I suspect that part of the problem is that line-heights are handled differently on Windows from other platforms, and on Windows East Asian fonts are handled differently from other fonts. This is what FONT_LEADING_APIS_V2 is about, if my memory is correct. Adding ifdefs like that leads to bugs that are in the code followed by one of the three paths (non-Windows, Windows Western fonts, Windows Asian fonts) and not the other. The solution is to fix the cross-platform API so that it does the right thing rather than adding per-platform ifdefs and fixing bugs on some platforms and leaving them on others. Then again, I'm not sure if that's the problem here -- so it would help if someone answered my question in comment 43. I'm not inclined to allow more differences between Windows and other platforms in this regard, because that's just making the underlying problems harder to fix. I'm also in general inclined to be skeptical of changes to the code for all platforms and fonts that are driven by the needs of East Asian fonts under Windows, since the code for handling of East Asian fonts under Windows is different enough from everything else that problems dealing with them seem likely to be in the specialized code for them rather than in the cross-platform cross-font code. Differences like that should be abstracted away in the platform-specific backend unless they're really needed for all platforms, in which case they should really be made for all platforms. I'm also not sure why increasing the line-height isn't increasing the size of the input. That seems like the real bug to solve.
Attachment #158634 -
Flags: superreview?(dbaron)
Attachment #158634 -
Flags: review?(bryner)
Comment 47•20 years ago
|
||
FYI, bug 218032 is about FONT_LEADING_APIS_V2 on non-Windows platforms.
Comment 48•20 years ago
|
||
(In reply to comment #43) > I don't understand comment 42, but I think 'normal' is the correct line-height. > > However, does this get better if you |#undef FONT_LEADING_APIS_V2| in > nsHTMLReflowState.cpp (after the include of nsIFontMetrics.h)? I think that we get a better result, if |FONT_LEADING_APIS_V2| is not defined, since |aLineHeight| and |aDesiredSize.height| in the attachment 110438 [details] [diff] [review] are equal, we get these height of the font from the functions of |CalculateSizeStandard| and |aReflowState.CalcLineHeight|.
Comment 49•20 years ago
|
||
The line height of the input box has normal line height that is |mEmHeight + mInternalLeading| at nsFontMetricsWin.cpp. The text line height has the computed line height that is |emHeight * NORMAL_LINE_HEIGHT_FACTOR| at nsHTMLReflowState.cpp. The base line of the input box shifts upword only the half of leading as folows. leading = |emHeight * NORMAL_LINE_HEIGHT_FACTOR| - |mEmHeight + mInternalLeading|.
Comment 50•20 years ago
|
||
Attachment #110437 -
Attachment is obsolete: true
Attachment #110438 -
Attachment is obsolete: true
Attachment #110439 -
Attachment is obsolete: true
Attachment #111186 -
Attachment is obsolete: true
Attachment #111189 -
Attachment is obsolete: true
Comment 51•20 years ago
|
||
Updated•20 years ago
|
Attachment #158634 -
Attachment is obsolete: true
Comment 52•20 years ago
|
||
Comment 53•20 years ago
|
||
I think that this bug does not restrict with the problem which occurs only with an input box.
URL: a��
Comment on attachment 159395 [details] [diff] [review] patch for shifting a baseline The leading is supposed to occur at the beginning of the block. See CSS1 or CSS2. Perhaps the correct simple solution would be a FONT_LEADING_APIS_V2 ifdef in nsTextControlFrame::CalculateSizeStandard ? (Is that involved in this code?)
Attachment #159395 -
Flags: review-
Comment 55•20 years ago
|
||
This patch increases the line-height of the input box (although the height of the textarea also increases).
Attachment #159395 -
Attachment is obsolete: true
Attachment #159399 -
Attachment is obsolete: true
Attachment #159400 -
Attachment is obsolete: true
Updated•20 years ago
|
Attachment #159448 -
Flags: review?(ftang)
Updated•20 years ago
|
Attachment #159448 -
Flags: review?(ftang)
Updated•20 years ago
|
Attachment #159448 -
Flags: review?(smontagu)
Assignee | ||
Comment 56•20 years ago
|
||
Comment on attachment 159448 [details] [diff] [review] patch for increasing the line-height >+ nsIRenderingContext* aRendContext = aReflowState.rendContext; If aRendContext is becoming a local variable instead of an argument, you should change the name to rendContext.
Comment 57•20 years ago
|
||
Attachment #159448 -
Attachment is obsolete: true
Updated•20 years ago
|
Attachment #159951 -
Flags: superreview?(smontagu)
Attachment #159951 -
Flags: review?(smontagu)
Updated•20 years ago
|
Attachment #159448 -
Flags: review?(smontagu)
Assignee | ||
Comment 58•20 years ago
|
||
Comment on attachment 159951 [details] [diff] [review] patch for increasing the line-height r=smontagu. I'm not a super-reviewer, so transferring sr request to dbaron.
Attachment #159951 -
Flags: superreview?(smontagu)
Attachment #159951 -
Flags: superreview?(dbaron)
Attachment #159951 -
Flags: review?(smontagu)
Attachment #159951 -
Flags: review+
Comment on attachment 159951 [details] [diff] [review] patch for increasing the line-height So would this change behavior if it weren't ifdef'd?
Comment 60•20 years ago
|
||
In the case of original code, the height of the line box is a |mMaxHeight| of font metrics. If FONT_LEADING_APIS_V2 is not defined, the calculated line height is NormalLineHeight that is |mEmHeight + mInternalLeading|. Because |mMaxHeight| and |mEmHeight + mInternalLeading| is equel, although that is now satisfactory, in future the same problem may occur except windows. Is this patch acceptable?
Comment 61•20 years ago
|
||
(In reply to comment #59) > (From update of attachment 159951 [details] [diff] [review]) > So would this change behavior if it weren't ifdef'd? If the meaning is related with the following code and FONT_LEADING_APIS_V2 is not defined, the patch does not change the behavior. +#ifdef FONT_LEADING_APIS_V2 + fontHeight = aReflowState.CalcLineHeight(aPresContext, rendContext, aReflowState.frame); +#else fontMet->GetHeight(fontHeight); +#endif // FONT_LEADING_APIS_V2
Comment 62•20 years ago
|
||
Comment on attachment 160033 [details] [diff] [review] patch for using calculated line-height The patch of attachment 159951 [details] [diff] [review] affects windows build, only when FONT_LEADING_APIS_V2 is defined. I think that the patch of attachment 160033 [details] [diff] [review] is better although it affects all build.
Attachment #160033 -
Flags: review?(dbaron)
Comment on attachment 160033 [details] [diff] [review] patch for using calculated line-height Agreed. Thanks for going through so many versions of this patch, and sorry for the delay.
Attachment #160033 -
Flags: review?(dbaron) → review+
Attachment #159951 -
Flags: superreview?(dbaron) → superreview-
Updated•20 years ago
|
Attachment #160033 -
Flags: superreview?(dbaron)
Attachment #160033 -
Flags: superreview?(dbaron) → superreview+
Comment 64•20 years ago
|
||
Thanks dbaron, please check into the trunk.
Comment 65•20 years ago
|
||
smontagu, could you check in attachment 160033 [details] [diff] [review], since dbaron's review and super-review were done?
Assignee | ||
Comment 66•20 years ago
|
||
Fix checked in.
Status: REOPENED → RESOLVED
Closed: 20 years ago → 20 years ago
Resolution: --- → FIXED
Comment 67•20 years ago
|
||
This fix caused Bug 263806 on Mac.
You need to log in
before you can comment on or make changes to this bug.
Description
•