167001 - Anything below the baseline goes out of input field

Status: RESOLVED FIXED

(Core :: Layout: Text and Fonts, defect)

Description

[Koike Kazuhiko]

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

[Jaime Rodriguez, Jr.]

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

[Shanjian Li]

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

Comment 3

[Frank Tang]

reassign to shanjian

Comment 4

[Hideo Saito]

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

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Bug 90884 and Bug 82265 are fixed on 20021100604-trunk/WinXP.
But, I can reproduce this problem with the build.

Comment 6

[Hideo Saito]

Attachment: testcase

Comment 7

[Hideo Saito]

Attachment: 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

[Hideo Saito]

Attachment: screen shot

Comment 9

[Shanjian Li]

reassign to ftang.

Comment 10

[Hideo Saito]

Attachment: patch for textarea

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

Comment 11

[Hideo Saito]

Attachment: testcase for textarea

Comment 12

[Hideo Saito]

Attachment: 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

[Hideo Saito]

Attachment: screen shot for textarea

Comment 14

[Hideo Saito]

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.

Comment 15

[Frank Tang]

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

[Hideo Saito]

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.

Comment 17

[Frank Tang]

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

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

No, I can still reproduce this bug with 2003021308-trunk/WinXP.
( http://bugzilla.mozilla.gr.jp/showattachment.cgi?attach_id=1015 )

Comment 19

[Yuying Long]

I can see the problem in the attached testcases on 02-12 trunk build /WinXP
also, re-open.

Comment 20

[Yuying Long]

Re-open

Comment 21

[Samir Gehani]

i18n triage team: nsbeta1+/adt3

Comment 22

[Samir Gehani]

i18n triage team: over to Simon for a look.  

Comment 23

[Michael Buckland]

Discussed in topembed bug triage.  Minusing.

Comment 24

[Vedran Miletic]

retargeting

Comment 25

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: patch

I cannot reproduce this bug with this simply patch.
Does it have problem?

Comment 26

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 27

[Simon Montagu :smontagu]

Patch checked in.

Comment 28

[HARUNAGA Hirotoshi]

This patch broke Mac build.
http://bugzilla.mozilla.gr.jp/attachment.cgi?id=2377&action=view

reopening.
please back it out.

Comment 29

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

[HARUNAGA Hirotoshi]

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

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: patch for Mac OS X and Camino

For Mac OS X and Camino.
But I cannot test it.

Comment 35

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 36

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: patch for Mac OS X and Camino

Sorry. my mistake.

Comment 37

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

In Bugzilla-jp, attachment 158634 [details] [diff] [review] is works fine.

Please review and superreview it.

Comment 38

[Frank Wein [:mcsmurf]]

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

Comment 39

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

I think bug 258889 is not invalid.

The new bug is new alignment problem.
I think the line-height being '1' is correctly.

Comment 41

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.

Comment 42

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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.

Comment 43

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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)?

Comment 44

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

Original patch backed out.

Comment 45

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: screenshot

See this screenshot.

If the patch is backedout, we cannot understand j and y.
Because decent is cutted.

Comment 46

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.

Comment 47

[Jungshik Shin]

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

Comment 48

[Hideo Saito]

(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

[Hideo Saito]

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

[Hideo Saito]

Attachment: patch for shifting a baseline

Comment 51

[Hideo Saito]

Attachment: testcase for patch

Comment 52

[Hideo Saito]

Attachment: screenshot for patch

Comment 53

[Hideo Saito]

I think that this bug does not restrict with the problem which occurs only with
an input box.

Comment 54

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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?)

Comment 55

[Hideo Saito]

Attachment: patch for increasing the line-height

This patch increases the line-height of the input box (although the height of
the textarea also increases).

Comment 56

[Simon Montagu :smontagu]

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

[Hideo Saito]

Attachment: patch for increasing the line-height

Comment 58

[Simon Montagu :smontagu]

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.

Comment 59

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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

[Hideo Saito]

Attachment: 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

[Hideo Saito]

(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

[Hideo Saito]

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.

Comment 63

[David Baron :dbaron: (⌚️UTC-5, no longer working on Mozilla)]

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.

Comment 64

[Hideo Saito]

Thanks dbaron, please check into the trunk.

Comment 65

[Hideo Saito]

smontagu, could you check in attachment 160033 [details] [diff] [review], since dbaron's review and
super-review were done?

Comment 66

[Simon Montagu :smontagu]

Fix checked in.

Comment 67

[HARUNAGA Hirotoshi]

This fix caused Bug 263806 on Mac.