481751 - Positions of text input and textarea controls depend on their text contents

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Karl Tomlinson (:karlt)]

Current behaviour:

The positioning of nsTextControlFrames with vertical-align:baseline depends
on the text in the element because the baseline of the nsTextControlFrame
depends on the first baseline of the descendant scrolled block.

When the text is edited the baseline of the descendant scrolled block changes,
but not enough ancestor frames are reflowed to cause the nsTextControlFrame to
be repositioned.

However, if, after editing the text, the page is reloaded, then the
nsTextControlFrame is repositioned.


Expected behaviour:

Adjacent text input controls should look aligned.

Reflowing the nsTextControlFrame after editing may be more consistent, and may
give a better view of the text, particularly of the frame height was also
adjusted.  However, I doubt many people expect the control to change position
while text is being modified.  (We don't make the control longer as the text
becomes longer.)

I think the best thing to do is to use a "natural" baseline for the
nsTextControlFrame from the same strut as used for the default height of the
frame.

Comment 1

[Karl Tomlinson (:karlt)]

Attachment: testcase

This testcase uses URW fonts that come with ghostscript, and are likely to be
installed on most Linux systems.  The primary font is Dingbats, which is
chosen as it behaves similarly to the primary UI font on many systems with a
locale that uses a non-Latin script.

Comment 2

[Karl Tomlinson (:karlt)]

Attachment: screenshot of testcase

In the text input at the top right, fallback from the primary font occurs
(such as when there is Latin text on a non-Latin locale), and the baseline of
the descendant scrolled block changes, causing the position of the control to
differ from the other input control.  If the text is deleted and the page is
reloaded, the control aligns with the other control.  The behavior is similar
with the textarea element with vertical-align: baseline.


The last line here demonstrates the effect of choice of baseline for a
multiline textarea.  Each row in the textarea has a "natural" baseline from
the same strut as used for the row height.  The main question here is whether
to use the baseline of the first or last row.

The textarea element is in some ways similar to an inline-block.  The baseline
of an inline-block is the baseline of the last line in the inline-block.  Note
that if the inline-block descends beyond the last line, that does not affect
its baseline (wrt the top).

I don't think we want the positioning of the textarea to depend on the
position of the last line of its text content, as adding another line of text
should then mean that the textarea control should be moved up (when
vertical-align is baseline).

Our current implementation uses a kind of first baseline.  This is nice when
starting to input text input an empty textarea, as that text is most likely to
align with surrounding text.

If the textarea is likely to be full, then the position of text added to the
end will correspond to the last row of the textarea, and so the last row
baseline may be better.  But this isn't enough to convince me that we should
change from first line to last row behavior.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> Reflowing the nsTextControlFrame after editing may be more consistent, and may
> give a better view of the text, particularly of the frame height was also
> adjusted. 

It would also be a performance nightmare...  You can try this yourself: remove the code that marks text control frames' anonymous <div>s as reflow roots and see how performance looks.

I'm not so much worried about the textarea case, honestly, since we don't baseline-align textareas by default.

One option we could seriously consider is to fix the baseline of single-line text inputs at the baseline of a strut in that input's default font.  This should work the same way as what we have now for all cases without font fallback, and will keep fallback from affecting anything outside the text control....

Comment 4

[Karl Tomlinson (:karlt)]

Attachment: natural baseline and initial scroll position

This is an updated version of attachment 368197 [details] [diff] [review] with a fix for the
discontinuity in the function for scroll position in ScrollToInitialTarget()
wrt the height of the control when textareas have more than one line of text.

This may be an option for fixing this bug.  Things look good on initial load
of the page and after SetValue, but this feels like a band-aid fix.  Some
symptoms of this are that freshly entered text may not be baseline-aligned or
as visible as possible, and if contents are scrolled with the mouse then it
can be impossible to scroll back to that original position (though this is not
a regression from the current state).

Comment 5

[Karl Tomlinson (:karlt)]

I'm starting to think that a better approach for single-line text input
controls is to set the line-height of the child div to the height of the
client area of the control.

This would essentially let the line layout code do the vertical centering (and
baseline-fixing) for us, and this centering would happen even when the height
of the control is large and when new text is entered.

The line layout code centers a box between the top of the tallest glyph in the
primary font and the bottom of the lowest glyph in that font
(for line-height != normal).
For some fonts this isn't as good as the algorithm in attachment 369419 [details] [diff] [review]
when the control height is smaller than this max ascent/descent box.
But the line-layout code could probably be improved to considered the em box.

The scroll frame implementation cuts off overflow above the div but not below.
This means that the text can still be scrolled to see low descenders but not
tall ascenders like stacked accents.  Perhaps the overflow above the div could
be made viewable at a -ve scroll offset, but then other modifications would be
necessary to make it possible to scroll to the original position.

Comment 6

[Karl Tomlinson (:karlt)]

Attachment: range of text input heights testcase

In the top row are text input controls with different CSS heights applied.

The second row is similar to the first but the line-height is also specified
on the controls to be equal to the height.  line-height actually has no effect
on text input controls kind of because UA style sheets force line-height to
normal (see bug 349259).

The third row is a set of divs with varying heights and line-height specified
to the equal to height.  This mirrors what happens on the div within the text
control when line-height has effect (though the bottom of the text in controls
gets truncated in quirks mode unless the line-height is appropriately modified
for border/padding).

Comment 7

[Karl Tomlinson (:karlt)]

Attachment: screenshot with line-height effects on controls

This screenshot is taken with attachment 369414 [details] [diff] [review] from bug 349259 applied.  The
explicit non-normal line-height in the second row gives the controls centered text and consistent alignment (independent of their text contents).

Comment 8

[Karl Tomlinson (:karlt)]

Attachment: screenshot with natural baseline and initial scroll position patch

This is with both attachment 369414 [details] [diff] [review] and attachment 369419 [details] [diff] [review] applied.

The top row show the effects of attachment 369419 [details] [diff] [review]:
control positions do not depend on contents but baseline alignment is not
maintained (due to the fallback font), and large controls still grow down
rather than being centered.

Comment 9

[Karl Tomlinson (:karlt)]

Attachment: line-height patch

(In reply to comment #5)
> I'm starting to think that a better approach for single-line text input
> controls is to set the line-height of the child div to the height of the
> client area of the control.

This patch implements this for text input elements by using a special line-height keyword value.  This value indicates to use the height of the nearest block for the line-height.  (This is similar to containing blocks if you consider lines to be "contained by blocks", but the minimum line-height property on the block comes from the block itself not its containing block.  Getting the height of the containing block would have worked fine for text input elements, but using the normal containing block concept would have been a little quirky with line-heights for inline elements depending on their block's height but minimum line-heights depending on that block's containing block's height.)

With this patch, the first line in attachment 369422 [details] looks like the second line in the screenshots in attachments 369424 and 369426.

For textarea elements, this patch only changes the baseline of the nsTextControlFrame (so that it doesn't depend on its text).  The text contents within the div are rendered the same as without the patch.

Builds are currently available here:
https://build.mozilla.org/tryserver-builds/2009-04-14_21:[e-mail]-try-66673d82f1c/

Comment 10

[Karl Tomlinson (:karlt)]

Attachment: screenshot of initial testcase with line-height patch

Comment 11

[Karl Tomlinson (:karlt)]

Attachment: reftests for line-height patch

Comment 12

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

Comment on attachment 373038 [details] [diff] [review]
line-height patch

>diff --git a/layout/forms/nsTextControlFrame.cpp b/layout/forms/nsTextControlFrame.cpp
>-  if (!IsSingleLineTextControl()) {
>+  if (IsSingleLineTextControl()) {
>+    // Make the line-height equal to the available height.
>+    rv = mAnonymousDiv->
>+      SetAttr(kNameSpaceID_None, nsGkAtoms::style,
>+              NS_LITERAL_STRING("line-height: -moz-block-height;"),
>+              PR_FALSE);
>+  }
>+  else {

This should go in forms.css ("input > .anonymous-div" rule) instead.



You should also add -moz-block-height to layout/style/test/property_database.js and then make sure all the mochitests in layout/style pass.  I don't intend to look at the style system code much until after you do that.

 nsComputedDOMStyle::GetLineHeightCoord(nscoord& aCoord)
 {
-  aCoord = nsHTMLReflowState::CalcLineHeight(mStyleContextHolder);
+  nscoord blockHeight = NS_AUTOHEIGHT;
+  if (mOuterFrame &&
+      GetStyleText()->mLineHeight.GetUnit() == eStyleUnit_Enumerated) {
+    if (mOuterFrame->IsContainingBlock()) {
+      AssertFlushedPendingReflows();

This requires that you change COMPUTED_STYLE_MAP_ENTRY to COMPUTED_STYLE_MAP_ENTRY_LAYOUT for both line-height and vertical-align.  You should also just push the AssertFlushedPendingReflows up to the top of the function, I think.



However, I also need to think about the concept here a bit.  Why is it that we want a new value of line-height here rather than a new (or existing) value of vertical-align?

Comment 13

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

Could you explain the alignment behavior you're trying to achieve with this patch, and how it's different from the current behavior?

Comment 14

[Karl Tomlinson (:karlt)]

(In reply to comment #13)
> Could you explain the alignment behavior you're trying to achieve with this
> patch, and how it's different from the current behavior?

The difference is visible by comparing the first and second rows in the
screenshot in attachment 369424 [details].  Currently the first row (div) of attachment
369422 looks like the first row in attachment 369424 [details].  With the patch, the
first row of attachment 369422 [details] looks the same as the second row in attachment
369424.

There are two alignments being changed here:

1) The ascent of the text control element no longer depends on the contents of
   the text control (reported in comment 0).  The ascent of single-line text
   inputs is set to the baseline of a strut (in that input's default font)
   vertically centered in the client area of the text input element.

2) (Editable) text within a single-line text control is vertically centered
   (based on the elements default font) with the control's client area.
   The current top-align situation is part of the cause of bug 425004
   and others.

These two alignments should be changed together/consistently so that text
within a single-line text input aligns with text outside the text input.

(In reply to comment #12)
> Why is it that we want a new value of line-height here rather than a new (or
> existing) value of vertical-align?

I think we want all text (from all fonts) to be baseline vertical-aligned
(with the other text).

The top of the div is aligned with the top of the client area of the text
control.  Baseline alignment is achieved by making the line-box for the text
the same height as the client area of the text input.  line-height seemed more
appropriate for this than vertical-align.

Does it make sense to set vertical-align on the (block) anonymous div?
AIUI vertical-align would affect the relative alignment between inline-level elements within the line-box, but would not directly control the position or size of the line-box.

Comment 15

[Daniel Veditz [:dveditz]]

Nomating for blocking1.9.1 because this is apparently the patch for 1.9.1-blocking bug 453827

Comment 16

[Boris Zbarsky [:bzbarsky]]

Ugh.  This is a pretty scary change for this point in the cycle, imo.

Comment 17

[Karl Tomlinson (:karlt)]

Attachment: line-height patch (updated for comment 12)

Thanks for the comments, David.

(In reply to comment #12)
> You should also add -moz-block-height to layout/style/test/property_database.js

nsComputedDOMStyle::GetLineHeightCoord needs to return a (non-initial) value when the element with computed line-height == -moz-block-height has no frame.
This now returns the computed value "-moz-block-height" when there is no frame to calculate a value, which is similar to what we do with percentage GetMaxHeight/GetMinHeight values and auto height.

Comment 18

[Karl Tomlinson (:karlt)]

Attachment: tidied up reftests for line-height patch

(Two of these reftests fail with the earlier "natural baseline and initial scroll position" patch attachment 369419 [details] [diff] [review].  input-text-baseline-1.html fails because the editable content has downloaded fonts which are dynamic and ScrollToInitialTarget is not called after reflow of the anonymous div.  input-text-centering-1.xul fails because there was no attempt to make that pass.)

Comment 19

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

(In reply to comment #14)
> 1) The ascent of the text control element no longer depends on the contents of
>    the text control (reported in comment 0).  The ascent of single-line text
>    inputs is set to the baseline of a strut (in that input's default font)
>    vertically centered in the client area of the text input element.

This sounds good.

> 2) (Editable) text within a single-line text control is vertically centered
>    (based on the elements default font) with the control's client area.
>    The current top-align situation is part of the cause of bug 425004
>    and others.

I don't understand why this should be described in terms of vertical centering.  It seems to me that what you want to do here is align the baseline of the contents with the baseline of the control, and that doing so could be a lot simpler than what you're doing here.  (You also say below that they're equivalent, but I'm having trouble following that statement of equivalence.  Even if they are, wouldn't doing it directly be simpler?)

> These two alignments should be changed together/consistently so that text
> within a single-line text input aligns with text outside the text input.

Comment 20

[Karl Tomlinson (:karlt)]

(In reply to comment #19)
> (In reply to comment #14)
> > 1) The ascent of the text control element no longer depends on the
> >    contents of the text control (reported in comment 0).  The ascent of
> >    single-line text inputs is set to the baseline of a strut (in that
> >    input's default font) vertically centered in the client area of the
> >    text input element.
> 
> This sounds good.
> 
> > 2) (Editable) text within a single-line text control is vertically
> >    centered (based on the elements default font) with the control's client
> >    area.  The current top-align situation is part of the cause of bug
> >    425004 and others.
> 
> I don't understand why this should be described in terms of vertical
> centering.

With the implementation in the patch here, where the vertical position
depends only on the metrics of the default font, the text is not really
"vertically centered"; it is merely the baseline being positioned in such a
way that the text is expected to be close to vertically centered.  There would
be cases where considering the extents of the actual glyphs used would give a
better result, and one day we may wish to sacrifice baseline alignment (with
text outside the control) for the sake of visibility of the text (inside the
control).

Conceptually, I think of the ascent of the control (1) as chosen to be the
vertical position where text is expected to be rendered.  So (1) tries to
match (2), but doesn't know what the text is.

>  It seems to me that what you want to do here is align the baseline of the
> contents with the baseline of the control, and that doing so could be a lot
> simpler than what you're doing here. (You also say below that they're
> equivalent, but I'm having trouble following that statement of equivalence. 
> Even if they are, wouldn't doing it directly be simpler?)

Simpler would be good, and if aligning the contents with the control is
simpler, then I'm happy with that.

I'm wondering if you have anything in mind here.

Are you thinking of (for text input elements) making the anonymous div
overflow:visible, moving the reflow root from the div to the control frame,
and aligning the div within the control frame during reflow of the control
frame?  I guess the control frame wouldn't be a stack frame anymore?

I'm also wondering what Boris meant by "reflow callback" in
bug 425004 comment 18?

Comment 21

[Karl Tomlinson (:karlt)]

If we could tell the block where to put the baseline of its line-box, then that would seem appealing.  But that's a bit different to the existing line-layout that we have IIUC.

Comment 22

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

(In reply to comment #20)
> Are you thinking of (for text input elements) making the anonymous div
> overflow:visible, moving the reflow root from the div to the control frame,
> and aligning the div within the control frame during reflow of the control
> frame?  I guess the control frame wouldn't be a stack frame anymore?

I think of those points, only changing which frame is the reflow root would really be necessary.  I think it's probably doable while leaving the overflow:hidden and leaving the text control a stack, by simply scrolling to the appropriate vertical position during Reflow, or something like that.  (There might be other scrolling operations that might need to be changed to avoid changing the vertical position.)

Then again, changing which frame is the reflow root might be somewhat risky...

Comment 23

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

Comment on attachment 376533 [details] [diff] [review]
line-height patch (updated for comment 12)

OK, I guess it's ok to just go with this approach.

>+nsHTMLReflowState::CalcLineHeight() const
>+{
>+  nscoord blockHeight =
>+    frame->IsContainingBlock() ? mComputedHeight :
>+    mCBReflowState ? mCBReflowState->mComputedHeight :
>+    NS_AUTOHEIGHT;

Could you parenthesize the inner ?: for clarity?

sr=dbaron

Comment 24

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

We're going to put this on the wanted1.9.1 list rather than blocking, but we'd be likely to take it on branch if it's ready soon.

Comment 25

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 376533 [details] [diff] [review]
line-height patch (updated for comment 12)

This looks great; I'm sorry it took me so long to review...

Please document the aBlockHeight argument to nsHTMLReflowState::CalcLineHeight,
and fix dbaron's nit, and r=bzbarsky.  Arlan thinks it looks pretty good too, for what it's worth.  ;)

Comment 26

[Karl Tomlinson (:karlt)]

Attachment: line-height patch (for check-in)

Comment 27

[Karl Tomlinson (:karlt)]

http://hg.mozilla.org/mozilla-central/rev/a1b982e50678
http://hg.mozilla.org/mozilla-central/rev/a9deac739d2f

Comment 28

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 378147 [details] [diff] [review]
line-height patch (for check-in)

Big patch, sadly, we'll get it next time.

Comment 29

[Daniel Veditz [:dveditz]]

1.9.0.13 and 1.9.1.2 ought to be "next time". (1.9.1.1 is catch-up to 1.9.0.12 for bugs that couldn't land during final lock-down)

We're going to need the regressions fixed though.

Comment 30

[Samuel Sidler (old account; do not CC)]

Yeah, we need to get this for 1.9.1.2, but not 1.9.1.1.

Comment 31

[Daniel Veditz [:dveditz]]

Comment on attachment 376534 [details] [diff] [review]
tidied up reftests for line-height patch

Approved for 1.9.1.3, a=dveditz for release-drivers

Comment 32

[Daniel Veditz [:dveditz]]

Comment on attachment 378147 [details] [diff] [review]
line-height patch (for check-in)

Approved for 1.9.1.3, a=dveditz for release-drivers

Can we please have a 1.9.0-branch patch if this one doesn't apply? Thanks.