fix(form-textarea): improved computedHeight calculation when in auto resize mode #3012

Istador · 2019-04-04T22:27:43Z

Description of Pull Request:

computedHeight is used, when max-rows is given to an b-form-textarea to calculate the auto height.

1. fix: not subtracting border width from scrollHeight
scrollHeight contains only the padding and not the border.
This resulted in an incorrect calculation of contentRows (float instead of int) and lead to an visible vertical scroll bar, even when max-rows haven't been reached yet.

~~2. fix: respecting boxSizing in height calculation~~
~~Depending on boxSizing, the height of the element contains the padding and border (box-sizing: border-box) or not (box-sizing: content-box).~~ EDIT (by TM): Bootstrap CSS always sets every element to border-box, so this extra test would only generate overhead that is not needed.

~~3. fix: setting height temporary to minHeight instead of 'auto'~~
Setting it to auto might result in a height that is bigger than 2 content rows.
This is bad because when the content height is lower than this height, then scrollHeight is affected by this. It effectively results in the auto-height being the minHeight, therefore the textarea is bigger than it should be.

I only tested chrome, firefox and waterfox on desktop. Because one of the lines I changed was marked as "browser dependant" it should better be tested thoroughly - especially on mobile browsers.

PR checklist:

What kind of change does this PR introduce? (check at least one)

Does this PR introduce a breaking change? (check one)

Yes
No

If yes, please describe the impact:

The PR fulfills these requirements:

It's submitted to the dev branch, not the master branch
When resolving a specific issue, it's referenced in the PR's title (i.e.
fixes #xxxx[,#xxxx], where "xxxx" is the issue number)
The PR should address only one issue or feature. If adding multiple features or fixing a bug
and adding a new feature, break them into separate PRs if at all possible.
PR titles should following the
Conventional Commits naming convention (i.e.
"fix(alert): not alerting during SSR render", "docs(badge): Updated pill examples, fix typos",
"chore: fix typo in docs", etc). This is very important, as the CHANGELOG is generated
from these messages.

If new features/enhancement/fixes are added or changed:

Includes documentation updates (including updating the component's package.json for slot and
event changes)
New/updated tests are included and passing (if required)
Existing test suites are passing
The changes have not impacted the functionality of other components or directives
ARIA Accessibility has been taken into consideration (does it affect screen reader users or
keyboard only users? clickable items should be in the tab index, etc)

If adding a new feature, or changing the functionality of an existing feature, the PR's
description above includes:

A convincing reason for adding this feature (to avoid wasting your time, it's best to open a
suggestion issue first and wait for approval before working on it)

src/components/form-textarea/form-textarea.js

codecov · 2019-04-04T23:41:03Z

Codecov Report

Merging #3012 into dev will increase coverage by <.01%.
The diff coverage is 100%.

@@           Coverage Diff            @@
##             dev   #3012      +/-   ##
========================================
+ Coverage   98.2%   98.2%   +<.01%     
========================================
  Files        205     205              
  Lines       3685    3686       +1     
  Branches    1105    1105              
========================================
+ Hits        3619    3620       +1     
  Misses        46      46              
  Partials      20      20

Impacted Files	Coverage Δ
src/components/form-textarea/form-textarea.js	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 436e8c1...62be647. Read the comment docs.

tmorehouse · 2019-04-04T23:49:38Z

src/components/form-textarea/form-textarea.js

      if (!isVisible(el)) {
        return null
      }

-      // Remember old height (includes `px` units) and reset it temporarily to `auto`
-      const oldHeight = el.style.height
-      el.style.height = 'auto'


Part of the reason for using auto was that it would trigger an immediate content reflow of the <textarea>, as setting it to any other value may not trigger a reflow/recalculation until the next animation frame (which would generally happen after the rest of the computed property has completed executing) so the scrollHeight may not be actually be updated until well after the value is used.

tmorehouse · 2019-04-05T02:00:38Z

@Istador do you have an example of where #3, in your original post above, is occurring? What browser, and can you provide a minimum reproduction of that issue?

auto was used because of it automatically triggering a reflow calculation, where as setting it to a specific height may wait until the next animation frame occurs to update the new scrollHeight. The next animation frame may happen well after the routine pulls the (possibly incorrect) scrollHeight from the <textarea>.

tmorehouse · 2019-04-05T02:10:13Z

Here is an example using the new code from this PR, with rows set to 2 and max-rows set to 6 (in Chrome 72):

And the old code with the same text typed in (Also on chrome 72):

Note the extra blanks line in the first example using the code from this PR that sets to minHeight, where as the second example is the original code which is using auto as the temporary height.

So my suggestion would be to veto the change using the calculated minHeight and revert that part back to auto.

Needed for proper reflow recalculation of the `scrollHeight`

tmorehouse · 2019-04-05T03:55:24Z

The real issue with the extra line (or lack of in some situations) is due to the effect the scroll bar has on the maximum width of the content. i.e. when calculating the required height, there is no scrollbar taken into account.

By adding overflow-y: scroll (rather than the default of overflow-y: auto), the effect the scrollbar has on the content is always consistent (because the inner width does not change).

resolved

Istador · 2019-04-05T06:42:51Z

@Istador do you have an example of where #3, in your original post above, is occurring? What browser, and can you provide a minimum reproduction of that issue?

auto was used because of it automatically triggering a reflow calculation, where as setting it to a specific height may wait until the next animation frame occurs to update the new scrollHeight. The next animation frame may happen well after the routine pulls the (possibly incorrect) scrollHeight from the <textarea>.

#1: Scrollbar present when not needed
https://jsfiddle.net/vthwn6gq/

#2: Too big at the end (the user might overwrite box-sizing to content-box for whatever reason):
https://jsfiddle.net/g7k6txoj/

#3: Too big if underfull:
https://jsfiddle.net/byjdxr8f/
Works in Chrome 73.0.3683.103 but not in Firefox 66.0.2 or 60.6.1esr

tmorehouse · 2019-04-05T15:35:19Z

For #2, the user should never overwrite box-sizing: border-box as that would completely break bootstrap V4 layout which depends on sizes including the border, since form-controls rely on this accurate sizing (for widths of elements).

fix(form-textarea): fixed computedHeight calculation

49949e6

tmorehouse reviewed Apr 4, 2019

View reviewed changes

src/components/form-textarea/form-textarea.js Outdated Show resolved Hide resolved

tmorehouse added Type: Enhancement Type: Fix labels Apr 4, 2019

jacobmllr95 previously requested changes Apr 4, 2019

View reviewed changes

Update form-textarea.js

43249f0

jacobmllr95 added 2 commits April 5, 2019 01:41

Update form-textarea.js

9de9f43

Update form-textarea.js

cb7dc30

jacobmllr95 self-requested a review April 4, 2019 23:45

tmorehouse reviewed Apr 4, 2019

View reviewed changes

Update form-textarea.js

8011545

tmorehouse added 4 commits April 4, 2019 23:31

revert to auto for temporary height

deddc09

Needed for proper reflow recalculation of the `scrollHeight`

Update form-textarea.js

f5123c6

Update form-textarea.js

08e2a88

lint

62be647

tmorehouse changed the title ~~fix(form-textarea): fixed computedHeight calculation~~ fix(form-textarea): fixed computedHeight calculation when in auto resize mode Apr 5, 2019

tmorehouse changed the title ~~fix(form-textarea): fixed computedHeight calculation when in auto resize mode~~ fix(form-textarea): improved computedHeight calculation when in auto resize mode Apr 5, 2019

tmorehouse approved these changes Apr 5, 2019

View reviewed changes

tmorehouse merged commit 0043b92 into bootstrap-vue:dev Apr 5, 2019

tmorehouse mentioned this pull request Apr 10, 2019

<b-textarea> with max-rows property adds overflow-y: scroll #3077

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

fix(form-textarea): improved computedHeight calculation when in auto resize mode #3012

fix(form-textarea): improved computedHeight calculation when in auto resize mode #3012

Uh oh!

Istador commented Apr 4, 2019 •

edited by tmorehouse

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Apr 4, 2019 •

edited

Loading

Uh oh!

tmorehouse Apr 4, 2019 •

edited

Loading

Uh oh!

tmorehouse commented Apr 5, 2019 •

edited

Loading

Uh oh!

tmorehouse commented Apr 5, 2019 •

edited

Loading

Uh oh!

tmorehouse commented Apr 5, 2019

Uh oh!

Istador commented Apr 5, 2019

Uh oh!

tmorehouse commented Apr 5, 2019

Uh oh!

Uh oh!

Uh oh!

fix(form-textarea): improved computedHeight calculation when in auto resize mode #3012

fix(form-textarea): improved computedHeight calculation when in auto resize mode #3012

Uh oh!

Conversation

Istador commented Apr 4, 2019 • edited by tmorehouse Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of Pull Request:

PR checklist:

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Apr 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

tmorehouse Apr 4, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tmorehouse commented Apr 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmorehouse commented Apr 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tmorehouse commented Apr 5, 2019

Uh oh!

Istador commented Apr 5, 2019

Uh oh!

tmorehouse commented Apr 5, 2019

Uh oh!

Uh oh!

Istador commented Apr 4, 2019 •

edited by tmorehouse

Loading

codecov bot commented Apr 4, 2019 •

edited

Loading

tmorehouse Apr 4, 2019 •

edited

Loading

tmorehouse commented Apr 5, 2019 •

edited

Loading

tmorehouse commented Apr 5, 2019 •

edited

Loading