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.

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.

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.

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

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

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.

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

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

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.

(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:22-ktomlinson@mozilla.com-try-66673d82f1c/

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?

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

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

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

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

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.

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

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

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

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.

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

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 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. ;)

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

Comment on attachment 378147 [details] [diff] [review] line-height patch (for check-in) Big patch, sadly, we'll get it next time.

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.

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

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