167236 - [FIXr]Text/password inputs should be aligned to baseline

Status: RESOLVED FIXED

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

Description

[Boris Zbarsky [:bzbarsky]]

Currently text/password inputs claim that their baseline is at the bottom of the
border.  As a result, we have to align them to text-bottom for things not to
look too crappy.  The problem, of course, is that this is exactly what
IE/Windows does.  NS4/Unix aligns the inputs with their baseline at the baseline
of the text inside.  This is a much more sensible approach, imo.

Attaching a patch that makes us do that.  Some issues I'd like to resolve, however:

1)  Do we want to do this?  (I think "yes"; I know mpt and tor agree)
2)  How do we want to align <textarea>?  (For now I left it exactly as it was
    without my changes
3)  <input type="text" style="height: 200px"> -- where should the baseline go?
    (with my changes it ends up near the type of the <input>, 
     just like it would for an inline-block; without them it's at the bottom)
4)  Should we put 
      input:not([type]), input[type="text"], input[type="password"] {
          height: auto ! important 
      } 
    in forms.css?  (This would get around issue #3, but I don't think we should
    really do this.)

See 

http://web.mit.edu/bzbarsky/www/testcases/form-baseline/textInputInRunningText.html
http://web.mit.edu/bzbarsky/www/testcases/form-baseline/textInputsShowingBaseline.html
http://web.mit.edu/bzbarsky/www/testcases/form-baseline/textareaInRunningText.html

for some testcases; the same directory also has -quirk versions of the same
files where I've just removed the doctype.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: patch

Comment 2

[Hixie (not reading bugmail)]

I'd want rbs to review this patch. Is this the right way to do baseline
alignment? (rbs implemented baseline alignment for table cells)

Comment 3

[Boris Zbarsky [:bzbarsky]]

ccing rbs, then.  ;)

Comment 4

[Boris Zbarsky [:bzbarsky]]

> "(with my changes it ends up near the type of the <input>"

I meant "near the _top_", of course.

Comment 5

[fantasai]

1)  Do we want to do this?  (I think "yes"; I know mpt and tor agree)

I agree, too. It makes the form input's contents look more like part of the 
text. Authors can override this behavior with vertical-align if they want.

2)  How do we want to align <textarea>?

I would personally find aligning the first line's baseline to the line's 
baseline very useful--one rarely places the label at the bottom of a multi-line 
input--but I think this might be too much of a change. Also, CSS3 is suggesting 
the last line for baseline alignment of inline blocks.

3)  <input type="text" style="height: 200px"> -- where should the baseline go?

I think the text should be either at the bottom or in the middle of the box.

Considering also
   <input type="text" style="height: 10px; font-size: 16px">,
as well as the fact that 'line-height', not 'height', affects the visible height 
of a non-replaced inline box and that leading is always split across the top and 
bottom, I suggest centering the text.

I think most people use font size to control input height anyway.

http://www.w3.org/TR/REC-CSS2/visudet.html#propdef-line-height

Comment 6

[rbs]

Attachment: effect of the patch

patch isn't ready yet (let me know when you have the final patch -- cannot
afford lot of early reviews).

When I was attaching this screenshot, also noted that the "Browse..." button
became a large mutant :-) Will attach another screenshot. See also the quote
from Erik in bug 76097 comment 36 regarding "horror stories" if the height of
form elements isn't retained up to the pixel.

Comment 7

[rbs]

Attachment: big Browse...

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: I guess we do need to fix buttons after all

Ah, great fun... I accidentally set button/sumit/reset inputs to not be
vertical-align: text-bottom in that page, which is why the bugzilla page was
looking odd.  And the file control was wacky for much the same reason (the
button was putting its bottom on the baseline of the <input>

Restoring that style helps somewhat, but the textfield and the button are
misaligned still... and the file upload looks less odd but somewhat odd.  The
baselines really need to match there.

I was wondering whether the button patch should be included (it's in my tree
too, hence my not seeing the problems) but it's evident that it should be. 
Uploading patch with that added in.  With it, both of those testcases look just
dandy.

This is all pretty raw so far; not much testing.  Just checking that people
actually want this (and good call on ccing rods; I forgot to do that).

The net effect of this change should be to make the line boxes holding text
inputs have a slightly smaller ascent (by about 3px + descent of font) and a
slightly larger descent (by about 3px).  The total line height will probably
decrease as a result, though for a typical font that would be a 1-2 px decrease
at most.  I suppose if we really care we could increase the vertical padding of
the inputs.

Oh, and any heavily-table-based page is not likely to suffer much from this,
since this does not change how an <input> behaves when it's the only thing in a
table cell.... And such pages tend to valign="center" all over in any case.

Comment 9

[John Keiser (jkeiser)]

Comment on attachment 98387 [details] [diff] [review]
I guess we do need to fix buttons after all

r=jkeiser

Comment 10

[John Keiser (jkeiser)]

Rod, it would be good to get your feedback too.  I think this is the right
thing, and the patch works as advertised.

Comment 11

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

Comment on attachment 98387 [details] [diff] [review]
I guess we do need to fix buttons after all

>+    // For a textarea or <editor>, just don't bother for now

Why not?  The baseline is relevant for blocks for the application of
'vertical-align: baseline' on table cells.  (And textareas aren't even blocks,
are they?)  Or does it not work?

Other than that, sr=dbaron.

Comment 12

[Boris Zbarsky [:bzbarsky]]

Mostly that was because the baseline ended up near the top of the <textarea>,
iirc... I can't check at the moment, but could do it once I have net access again...

Comment 13

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

The baseline of a block is *supposed* to be the baseline of its first line.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Right.  But the problem then is that it _radically_ changes the way we render
textareas (as far as vertical alignment goes).  We're talking difference of
height of the textarea (unlike the several pixels for text inputs)...  While I
have no problems with that per se, I'd like to carefully investigate that change
before we make it...

Comment 15

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

I wouldn't want baseline-alignment for textareas to be the default.  However, I
think that's what it should do.

Comment 16

[John Keiser (jkeiser)]

bz, you want I should check this in for you?  It's Checkin Night.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Ah, ok.  jkeiser, sure if dbaron's OK with it.  Once I have a chance to write
some testcases I'll make the change for <textarea> as well in a separate patch.

Comment 18

[Boris Zbarsky [:bzbarsky]]

dbaron says he's cool with a separate <textarea> patch.  land it, I say.

Comment 19

[rbs]

Seems to me that this patch was checked in prematurely. It caused regression
bug 171214 which jkeiser said was deliberate [known beforehand?], but which
seems to me important enough to have warranted holding the patch a little bit
(since there isn't a contingency plan yet).

Comment 20

[John Keiser (jkeiser)]

Strange, my "fix checked in" comment didn't take.  We will continue discussion
in the other bug.

Comment 21

[Boris Zbarsky [:bzbarsky]]

OK.  The "buttons misaligned" thing is fixed (unwanted interaction with another
patch).  There is another problem, however.... clicking buttons makes things
shift around a bit.  Filed bug 171465 on that.

Comment 22

[Boris Zbarsky [:bzbarsky]]

I feel we should just back this patch out for now, since bug 171465 does not
seem to have any obvious solution and is pretty serious in my view...

Comment 23

[John Keiser (jkeiser)]

OK, backed out.  Buttons and text inputs are back to their horrible norm :)

I tried doing vertical-align: middle for input type=button, which made buttons
look decent but for some reason made text inputs no longer align to baseline!

Comment 24

[Boris Zbarsky [:bzbarsky]]

Attachment: current wip patch

Note that the button frame changes were not really backed out, so they're
already in the trunk....  This patch should fix the "shifting" issue by making
the buttons move _right_ instead of down, which is a little controversial....

I also made the right/left padding not suck, took out the "4px" hack that we
had that screwed up small buttons, andtook out the 3/2 hack we had for "IE
compat" (I feel that the way we do things with my patch, which is very similar
to IE/Mac, is more reasonable).

Still remaining: <textarea>, <select> (maybe), lots of testing (testcases
welcome!)

Comment 25

[Boris Zbarsky [:bzbarsky]]

Attachment: More WIP

Fixed: <select>, <select size="n">, <textarea>, <button> now all report
baselines
correctly.

Remaining issues:  Test the bejeezus out of the scrollframe change.  Chances
are no one but <select> cares, but worth checking.  Investigate interaction of
the "focusPadding" that <button> adds in (this is why <button> sizes
differently from <input type="button">) with this stuff.  Just removing it
makes the focus ring shrink-wrap the text, which is no good.  Of course removal
of the "3/2" hack made the focus ring shrink-wrap the text.... perhaps the
right answer is to make <input type="button"> use that code too, and to reduce
some of the other paddings in html.css

Comment 26

[Boris Zbarsky [:bzbarsky]]

Attachment: Version 1.57

OK.  This passes all the tests I've managed to come up with (see the url for
this bug).  The focus outlines look good.  I've run into no problems with
scrollframe.

I think this baby is ready for review.	The only remaining issues I can think
of are:

1) Do we want one more px of vertical padding on buttons (this makes all
   controls 2px taller) outside of the focus outline?
2) Are people OK with controls moving right instead of down when clicked?  Yes,

   ideally we would do both... If people feel strongly about it, I can try to
   put in some sort of hack to not account for the focusPadding when
calculating
   ascents or something....  but it would be very hackish.
   in some sort of hack to get

Comment 27

[Boris Zbarsky [:bzbarsky]]

Oh, that patch fixes bug 70251, basically, and bug 79927.

It also makes bug 90884 and bug 155483 worse; looking into why that is.

I also need to look at what's going on with bug 128115.

It's tempting to just XBL-ify buttons at this juncture....

Comment 28

[Boris Zbarsky [:bzbarsky]]

OK... all three of the bugs this made worse are about line-height.  Since I
think that line-height should not affect the internal rendering of form controls
(except _maybe_ <button>), I think we can more or less ignore those... (or
rather not include the fixes in this patch).

Comment 29

[Boris Zbarsky [:bzbarsky]]

Attachment: version 1.58

OK.  As I looked at this code again, I realized that if <button> and <input
type="button"> now render the same, we should just get rid of the
NavQuirkReflow() stuff that <input type="button"> does.  So the really relevant
parts of this patch are the:

-#if 1
+#if 0

in nsGfxButtonControlFrame.cpp and the nsHTMLButtonControlFrame reflow/sizing
cleanup (mostly in forms.css).	I like this patch, and if I get some buy-in I
will go ahead and permanently rip out that navquirk stuff.  But I want some
buy-in from rods or jkeiser first....

Ignore the line-height parts of forms.css in this patch; they fix the three
bugs blocking this one.

By the way, I can post some screenshots if people care.  ;)

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 104240 [details] [diff] [review]
version 1.58

There is a subtle problem with disabled comboboxes -- since they show no text,
they get misaligned....  The same problem affects <select> with no options in
it as well.  See
http://web.mit.edu/bzbarsky/www/testcases/form-baseline/disabledCombobox.html

Comment 31

[Boris Zbarsky [:bzbarsky]]

Attachment: Version 1.582

Comment 32

[John Keiser (jkeiser)]

Comment on attachment 104715 [details] [diff] [review]
Version 1.582

r=jkeiser with just one thing:

Remove the #if 0'd code in nsGfxButtonControlFrame::Reflow, and have the last
thing in the function be return nsHTMLButtonControlFrame::Reflow(...); it does
all the same things as the end of that function.  Even the else case does not
need to be there.  Neither side of that if () needs to do all that extra stuff.

The caching seems suspect, particularly caching ascent for checkbox and radio,
but this patch doesn't have to deal with that.

Everything appears fine in testing and such.

Comment 33

[Boris Zbarsky [:bzbarsky]]

Attachment: version 1.582 III -- Banishment of DoNavQuirksReflow

Comment 34

[John Keiser (jkeiser)]

Comment on attachment 105431 [details] [diff] [review]
version 1.582 III -- Banishment of DoNavQuirksReflow

r=jkeiser
I'd like to specifically hear Rod's comment on the button reflow deal but I
don't see anything bad and I do see many good things.

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 105431 [details] [diff] [review]
version 1.582 III -- Banishment of DoNavQuirksReflow

Looks good. Nice cleanup. sr=roc+moz

Comment 36

[rods (gone)]

Nothing jumps out at me, so as long it is updates when scripts changes it.....
looks fine.

Comment 37

[Boris Zbarsky [:bzbarsky]]

Thanks, rod.  I tested that for a bit, and everything looks good.  Fix checked in.