If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Anything below the baseline goes out of input field

RESOLVED FIXED in Future

Status

()

Core
Layout: Text
--
major
RESOLVED FIXED
15 years ago
13 years ago

People

(Reporter: Koike Kazuhiko, Assigned: smontagu)

Tracking

({testcase, topembed-})

Trunk
Future
x86
Windows 2000
testcase, topembed-
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [adt3] [ETA Needed])

Attachments

(4 attachments, 13 obsolete attachments)

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
(Reporter)

Description

15 years ago
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

15 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.
Blocks: 154896
Severity: normal → major
Keywords: nsbeta1+, topembed+
Whiteboard: [adt2] [ETA Needed]
Target Milestone: --- → mozilla1.0.2

Comment 2

15 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

Updated

15 years ago
Keywords: intl
QA Contact: ruixu → ylong

Comment 3

15 years ago
reassign to shanjian
Assignee: yokoyama → shanjian
Status: ASSIGNED → NEW

Comment 4

15 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;
Bug 90884 and Bug 82265 are fixed on 20021100604-trunk/WinXP.
But, I can reproduce this problem with the build.

Comment 6

15 years ago
Created attachment 110437 [details]
testcase

Comment 7

15 years ago
Created attachment 110438 [details] [diff] [review]
patch

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

15 years ago
Created attachment 110439 [details]
screen shot

Updated

15 years ago
Attachment #110438 - Flags: review?(shanjian)

Comment 9

15 years ago
reassign to ftang.
Assignee: shanjian → ftang

Updated

15 years ago
Attachment #110438 - Flags: review?(shanjian) → review+

Comment 10

15 years ago
Created attachment 111157 [details] [diff] [review]
patch for textarea

A patch for the height of the TEXTAREA field.
ftang, review this patch also?

Comment 11

15 years ago
Created attachment 111186 [details]
testcase for textarea

Comment 12

15 years ago
Created attachment 111188 [details] [diff] [review]
patch for a vertical scroll bar

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

15 years ago
Created attachment 111189 [details]
screen shot for textarea

Comment 14

15 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

15 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

15 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

15 years ago
Status: NEW → ASSIGNED

Comment 17

15 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
URL: a¾»
Status: ASSIGNED → RESOLVED
Last Resolved: 15 years ago
Resolution: --- → WORKSFORME
No, I can still reproduce this bug with 2003021308-trunk/WinXP.
( http://bugzilla.mozilla.gr.jp/showattachment.cgi?attach_id=1015 )

Comment 19

15 years ago
I can see the problem in the attached testcases on 02-12 trunk build /WinXP
also, re-open.
Status: RESOLVED → VERIFIED

Comment 20

15 years ago
Re-open
Status: VERIFIED → REOPENED
Resolution: WORKSFORME → ---

Comment 21

15 years ago
i18n triage team: nsbeta1+/adt3
Whiteboard: [adt2] [ETA Needed] → [adt3] [ETA Needed]

Comment 22

15 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

15 years ago
Discussed in topembed bug triage.  Minusing.
Keywords: topembed+ → topembed-

Comment 24

14 years ago
retargeting
Target Milestone: mozilla1.0.2 → Future

Updated

14 years ago
Keywords: testcase
Created attachment 158414 [details] [diff] [review]
patch

I cannot reproduce this bug with this simply patch.
Does it have problem?
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)
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

13 years ago
Patch checked in.
URL: a¾»a��
Status: NEW → RESOLVED
Last Resolved: 15 years ago13 years ago
Resolution: --- → FIXED

Comment 28

13 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 → ---
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?
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

13 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
?
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
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?
Created attachment 158633 [details] [diff] [review]
patch for Mac OS X and Camino

For Mac OS X and Camino.
But I cannot test it.
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)
Created attachment 158634 [details] [diff] [review]
patch for Mac OS X and Camino

Sorry. my mistake.
Attachment #158633 - Attachment is obsolete: true
Attachment #158634 - Flags: review?(bryner)
Attachment #158633 - Flags: review?(bryner)
In Bugzilla-jp, attachment 158634 [details] [diff] [review] is works fine.

Please review and superreview it.
Attachment #158634 - Flags: superreview?(dbaron)
(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?
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 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.
Created attachment 158661 [details]
screenshot

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

13 years ago
FYI,  bug 218032 is about FONT_LEADING_APIS_V2 on non-Windows platforms.

Comment 48

13 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

13 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

13 years ago
Created attachment 159395 [details] [diff] [review]
patch for shifting a baseline
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

13 years ago
Created attachment 159399 [details]
testcase for patch
Attachment #158634 - Attachment is obsolete: true

Comment 52

13 years ago
Created attachment 159400 [details]
screenshot for patch

Comment 53

13 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

13 years ago
Created attachment 159448 [details] [diff] [review]
patch for increasing the line-height

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

13 years ago
Attachment #159448 - Flags: review?(ftang)

Updated

13 years ago
Attachment #159448 - Flags: review?(ftang)

Updated

13 years ago
Attachment #159448 - Flags: review?(smontagu)
(Assignee)

Comment 56

13 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

13 years ago
Created attachment 159951 [details] [diff] [review]
patch for increasing the line-height
Attachment #159448 - Attachment is obsolete: true

Updated

13 years ago
Attachment #159951 - Flags: superreview?(smontagu)
Attachment #159951 - Flags: review?(smontagu)

Updated

13 years ago
Attachment #159448 - Flags: review?(smontagu)
(Assignee)

Comment 58

13 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

13 years ago
Created attachment 160033 [details] [diff] [review]
 patch for using calculated line-height

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

13 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

13 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

13 years ago
Attachment #160033 - Flags: superreview?(dbaron)
Attachment #160033 - Flags: superreview?(dbaron) → superreview+

Comment 64

13 years ago
Thanks dbaron, please check into the trunk.

Comment 65

13 years ago
smontagu, could you check in attachment 160033 [details] [diff] [review], since dbaron's review and
super-review were done?
(Assignee)

Comment 66

13 years ago
Fix checked in.
Status: REOPENED → RESOLVED
Last Resolved: 13 years ago13 years ago
Resolution: --- → FIXED

Comment 67

13 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.