Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 9 commits into from Apr 5, 2019

Conversation

Istador
Copy link
Contributor

@Istador Istador commented Apr 4, 2019

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)

  • Bugfix
  • Feature
  • Enhancement to an existing feature
  • ARIA accessibility
  • Documentation update
  • Other, please describe:

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 Outdated Show resolved Hide resolved
src/components/form-textarea/form-textarea.js Outdated Show resolved Hide resolved
src/components/form-textarea/form-textarea.js Outdated Show resolved Hide resolved
src/components/form-textarea/form-textarea.js Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Apr 4, 2019

Codecov Report

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

Impacted file tree graph

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

@jacobmllr95 jacobmllr95 self-requested a review April 4, 2019 23:45
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'
Copy link
Member

@tmorehouse tmorehouse Apr 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Member

tmorehouse commented Apr 5, 2019

@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
Copy link
Member

tmorehouse commented Apr 5, 2019

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

image

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

image

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.

@tmorehouse 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
Copy link
Member

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

@tmorehouse 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 tmorehouse merged commit 0043b92 into bootstrap-vue:dev Apr 5, 2019
@Istador
Copy link
Contributor Author

Istador commented Apr 5, 2019

@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/
1

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

#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
3

@tmorehouse
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants