1830576 - Firefox sizes <textarea rows=N> as roughly N+1 rows tall, under an "always show scrollbars" configuration

Status: VERIFIED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Daniel Holbert [:dholbert]]

STR:

1. Be sure you have traditional (non-overlay) scrollbars enabled.
   (On Windows and Linux, enable "Always show scrollbars" in Firefox preferences -- enabled by default on Windows, I think. On macOS, use the System Preferences app and search "scroll bar behavior" and choose "Show scroll bars: always")

2. View https://bugzilla.mozilla.org/attachment.cgi?id=9330847

ACTUAL RESULTS:
The first textarea as being ~2 lines tall. (Technically it's tall enough for 1 line of text and a horizontal scrollbar, which is about the same height.)

EXPECTED RESULTS:
It should perhaps be ~1 line tall.

Reasoning:
(A) Other browsers render it as 1 line tall.
(B) Google.com happens to have a cosmetic dependency on it rendering as 1 line tall; see bug 1829588.
(C) The extra height here is due to us reserving some space for the horizontal scrollbar. However, by default, I think textareas shouldn't be able to overflow horizontally, since we give them this styling:

https://searchfox.org/mozilla-central/rev/1777bccfa2559e8c1331fa1541150c985328778b/layout/style/res/forms.css#105,119-120

textarea {
...
  white-space: pre-wrap;
  word-wrap: break-word;

...and I think that makes it impossible to overflow horizontally, essentially? So maybe it'd make sense to suppress this space if we have these default styles, and only leave space for the scrollbar if we have non-default values of these properties that could conceivably cause us to need the horizontal scrollbar? That seems to be what Chrome is doing, when you compare the first and second textareas in the linked testcase. (It's entirely possible there's an some edge case that makes this trickier than I'm describing it.)
[EDIT: yeah, it's not that simple; it's still possible to overflow horizontally; I'll post some examples in following comments]

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: testcase 1 where there's horizontal overflow from character being wider than textarea

Update: with extremely small textareas (on the order of 1em wide), we can in fact trigger horizontal scrollbars.

Here's a testcase with default font-size that does show horizontal scrollbars in Chrome (covering up the content, which is unfortunate for them).

Firefox doesn't paint scrollbars at all here since it's below our minimum size where we'll show scrollbars, but I'll post another testcase next, with larger text, where we do show scrollbars.

Comment 2

[Daniel Holbert [:dholbert]]

Attachment: testcase 2 (same as 1 but now with `font-size:100px`)

Here's the same testcase but with a larger font-size. Just as in testcase 1, scrollbars are needed for the first few textareas (and in this case we actually draw them since we have enough space in the orthogonal axis to draw a useful scrollbar).

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: testcase 3 (same as 1 but now with `word-wrap: normal`)

Here's testcase 1 again but now with word-wrap:normal.

This doesn't influence Firefox's sizing behavior (which makes sense), but it seems to prompt Chrome and Safari to increase the textarea's height to leave room for a horizontal scrollbar. (I can understand why they might make that choice -- horizontal overflow does become easier to trigger with word-wrap:normal -- but it's still possible even without it, as shown by testcase 1.)

Comment 5

[Daniel Holbert [:dholbert]]

Attachment: testcase 4 (same as 1 but with `font-size:100px` and `word-wrap:normal`)

...and here that is again both a large font and word-wrap:normal. As in testcase 3, Chrome now adds some height to accomodate the scrollbar.

(I added explicit newlines in the textarea here to make a vertical scrollbar continue to show up, just to show that that scrollbar is [or can be] still present as well, even when we don't forcibly linewrap abc via the default word-wrap.)

Comment 6

[Daniel Holbert [:dholbert]]

Hi IanK -- I know you've been looking at textarea layout in the near-recent past -- do you know if there's a good reason that Chromium only reserves some height for the scrollbars here when word-wrap:normal is present? (comparing my attached testcase 3 vs. testcase 1, or testcase 4 vs. testcase 2)

Superficially, it's quite odd[1] that that word-wrap influences the textarea height. But it'd be nice to be able to reach interop on this, if it's possible to describe the behavior in an interoperable & ideally speccable sort of way.

[1] (I can see how a horizontal scrollbar would be more likely to be needed with word-wrap:normal -- but it's still possible that it'll be needed even without that style, as shown by testcase 1 and 2; so it seems strange to unconditionally not-reserve vs. always-reserve height for the scrollbar depending on the computed word-wrap.)

Comment 7

[Ksenia Berezina [:ksenia]]

Given that this is potentially affecting the search field on google.com, setting webcompat priority as P1

Comment 8

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is set to S3. However, this bug has a P1 WebCompat priority.
:emilio, could you consider increasing the severity of this web compatibility bug?

For more information, please visit BugBot documentation.

Comment 9

[Emilio Cobos Álvarez [:emilio]]

Severity seems appropriate for a cosmetic bug tbh, but yeah we're actively investigating, as the fix on Google's side should be trivial while we sort out what the behavior here should be.

Comment 10

[Daniel Holbert [:dholbert]]

I suppose this is sort of a duplicate of bug 33654. For now, I'm going to mark that bug as a dependency of this one, with the intent that we'll use this bug to come up with & land a fix, modulo some further info about what other browsers actually do per comment 6.

(bug 33654 is already at nearly 200 comments (with most of them decades old) which makes it a hard-to-follow spot for new discussion/investigation.)

Comment 11

[Ian Kilpatrick [:iank]]

Hey - yeah scrollbars for textareas are very weird today.

Our logic is here:
https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/core/layout/layout_box.cc;l=215-218?q=textarea.*intrinsic&ss=chromium

We basically do what you describe today - in that if the <textarea> has "overflow-wrap: normal" we'll reserve the space for the scrollbar.

This was added pre-fork in 2006 here:
https://chromium.googlesource.com/chromium/src/+/4467e346a42c01e5ef04cdbf97c0015fcb2ed911%5E%21/#F1

(Note we also just received a bug for always reserving space for the scrollbar in the inline dimension - I'll likely update this to match you - e.g. only reserving space for auto/scroll overflow).

Ian

Comment 12

[Daniel Holbert [:dholbert]]

(In reply to Ian Kilpatrick [:iank] from comment #11)

Hey - yeah scrollbars for textareas are very weird today. [...]
We basically do what you describe today - in that if the <textarea> has "overflow-wrap: normal" we'll reserve the space for the scrollbar.

Thanks!

So, that is indeed very weird. The code linked there is documented with this code-comment: "We are able to have a horizontal scrollbar if the overflow style is [...] auto and there's no word wrap."

This is clearly wrong/incomplete. That comment and the associated code are basically asserting/expecting that overflow:auto textareas with the default word-wrap styling will never have a horizontal scrollbar. That's clearly incorrect, given e.g. testcase 2 here, where WebKit and Chromium both show a scrollbar on at least some of the textareas.

(Having said that: the cases where a scrollbar is needed are admittedly a bit edge-casey; they require the textarea to be on the order of 1em wide, which might (?) be pretty rare in practice. So maybe it's a net win to optimistically not-reserve-space for the scrollbar, and then take the hit and let the scrollbar cover up content if it ends up being needed?)

(Note we also just received a bug for always reserving space for the scrollbar in the inline dimension - I'll likely update this to match you - e.g. only reserving space for auto/scroll overflow).

Can you share a link to that bug?

(And just to clarify, by "space for scrollbar in the inline dimension", are you talking about a vertical scrollbar (which contributes width i.e. space in the inline dimension), or a horizontal scrollbar? I want to better understand what change you're tentatively planning on, so we know to what extent it influences whether there's anything to be done on our end here. :) )

Comment 13

[Ian Kilpatrick [:iank]]

Yeah this issue - I haven't got around to triaging yet - https://bugs.chromium.org/p/chromium/issues/detail?id=1441993&q=component%3ABlink%3ELayout&can=2

What do you think about the following behaviour:

"vertical scrollbar" - contributes to inline-size if overflow is "auto" or "scroll" (as scrolling is likely in this dimension).
"horizontal scrollbar" - contributes to block-size if overflow is "scroll" (as scrolling unlikely in this dimension - remove the overflow-wrap case).
(given this issue, and the other linked issue folks don't expect space reserved for the auto scrollbar typically).

Ian

Comment 14

[Daniel Holbert [:dholbert]]

I think that sounds reasonable, yeah.

(For this bug here, that'd mean that all of the textareas here would render as 1 row tall -- the same height as if we had overflow-x:hidden. That represents a behavior-change in Gecko for all of the attached testcases, and a behavior-change in Blink/WebKit for testcases 3 and 4 (the ones with overflow-wrap). Plus behavior-changes for vertical scrollbars in Blink/WebKit, for the case in the blink issue that IanK linked.)

Comment 15

[Daniel Holbert [:dholbert]]

I think our fix here would just be an adjustment to the spot where we add the scrollbar block-size to the GetRows()-based calculation here:
https://searchfox.org/mozilla-central/rev/445a5ab684b73eb56d807d0f3b2fabcc85a7c3dd/layout/forms/nsTextControlFrame.cpp#224-236

I'm tentatively planning on adding a // TODO: compat hack, fix if we can check for overflow-wrap there, to match the linked Chromium/WebKit code from comment 11 for compatibility reasons. If we make progress on interoperably proving that this check can be removed without breaking web content, then all the better, and we can land that as a followup.

Comment 16

[Luke Warlow]

As an fyi, I've submitted a WebKit PR to address the vertical scrollbar space being incorrectly reserved. https://github.com/WebKit/WebKit/pull/13832
That along with my chromium patch that does the same should bring some level of interop.

I'm happy to do a follow up WebKit PR if there's a change needed there (I believe there is) to address the horizontal scrollbar case.

Comment 17

[Ian Kilpatrick [:iank]]

As a FYI Luke submitted the changes in Blink as - https://chromium-review.googlesource.com/c/chromium/src/+/4529815

Ian

Comment 18

[Daniel Holbert [:dholbert]]

(In reply to Luke from comment #16)

As an fyi, I've submitted a WebKit PR to address the vertical scrollbar space being incorrectly reserved. https://github.com/WebKit/WebKit/pull/13832

Thanks. For reference, this was https://bugs.webkit.org/show_bug.cgi?id=256697

I'm happy to do a follow up WebKit PR if there's a change needed there (I believe there is) to address the horizontal scrollbar case.

This followup (more closely tied to the focus of this bug here) seems to be tracked in https://bugs.webkit.org/show_bug.cgi?id=256811 .

(In reply to Ian Kilpatrick [:iank] from comment #17)

As a FYI Luke submitted the changes in Blink as - https://chromium-review.googlesource.com/c/chromium/src/+/4529815

Thanks. Looking at those changes, focusing on the horizontal scrollbar overflow:auto aspects -- it looks like in Blink, the layout_box.cc check for overflow-wrap: normal (from comment 11) is now disabled-by-default (behind the flag RuntimeEnabledFeatures::LayoutNewTextAreaScrollbarEnabled).

This means that (by default, unless this flag is toggled) Blink will render testcases 1 and 3 with the same textarea-heights, and testcases 2 and 4 with the same textarea heights -- not reserving height for a horizontal scrollbar in any of those cases.

So, amending the plan from comment 15: I'll put my (to-be-added) overflow-wrap check behind an off-by-default pref for consistency with the behavior that now exists in Blink. Hopefully we never need the overflow-wrap check, but we can fall back on it via a pref flip if it ends up being necessary.

Comment 19

[Luke Warlow]

https://github.com/WebKit/WebKit/commit/8b48b8b9e8b826979b747f9c11166b3f03c850c3 this is fixed in WebKit now.

Comment 20

[Ian Kilpatrick [:iank]]

115 has ramped for us, and this hasn't received any bug reports.

Comment 21

[Luke Warlow]

We've had a bug raised about this change. https://bugs.chromium.org/p/chromium/issues/detail?id=1472254

<textarea rows="1" wrap="off">xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx</textarea> results in the scrollbar obscuring the contents. Will update this issue with what bug fix we decide to go with.

Comment 22

[Luke Warlow]

Ah Ian's marked as wont fix so not an issue it turns out.

Comment 23

[James Graham [:jgraham]]

AFAICT there's no known site breakage currently associated with this, so unsetting the Webcompat Priority flag.

Comment 24

[Luke Warlow]

I believe there's still an interop bug here with testcases 1 and 3 where Firefox still renders with an extra row.

Comment 25

[Luke Warlow]

Attachment: Bug 1830576 - Update textarea intrinsic height calculations r?dholbert

This change adds a new layout.forms.textarea-scrollbar-new-behavior.enabled pref that controls the height calculations for textareas.

With the pref enabled (the default) textareas only include the scrollbar size in the height calculations when overflow-x is scroll, this matches Chromium and WebKit.

Also adds a corresponding WPT for this.

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/12723265fe8c
Update textarea intrinsic height calculations r=dholbert

Comment 27

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/46364 for changes under testing/web-platform/tests

Comment 28

[Serban Stanca [:SerbanS]]

https://hg.mozilla.org/mozilla-central/rev/12723265fe8c

Comment 29

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot

Comment 31

[Ardelean Oana, Desktop Test Engineering [:oardelean]]

Issue is reproducible on a 2024-05-15 Nightly build on Windows 10.
Verified as fixed on Firefox 128.0b2 and Firefox Nightly 129.0a1 on Windows 10, Ubuntu 22, macOS 14.