Skip to content

Conversation

@liamdebeasi
Copy link
Contributor

@liamdebeasi liamdebeasi commented Sep 13, 2023

Issue number: Part of #22940


What is the current behavior?

While working on a fix for #22940, I discovered another bug that impacted the reliability of my proposed fix for #22940. When we compute the scroll data (i.e. how much the input needs to be scrolled by), we subtract the keyboardHeight from the platformHeight (i.e. the viewport height):

const visibleAreaBottom = Math.min(contentRect.bottom, platformHeight - keyboardHeight);

Every time we tap between inputs (even if the keyboard is already open) we re-run scroll assist because the newly focused input could be partially obscured by the keyboard. However, in this case we scroll by too much because we effectively subtract the keyboard height twice. This is because by the time we compute platformHeight, the platform dimensions have already shrunk to account for the keyboard (when the webview resizes).

As a result, when we subtract keyboardHeight we get a visible area that is much smaller than the actual visible area.

Examples below with different resize modes. Notice that with the "Native" resize mode (entire webview resizes when keyboard is open) tapping into other inputs scrolls the content by much more than it needs to.

Body Native Ionic None
before-body.MP4
before-native.MP4
before-ionic.MP4
before-none.MP4

What is the new behavior?

  • We now compute the viewport height on load rather than on focus. This ensures that we always use the full viewport height when computing the visible area.
Body Native Ionic None
after-body.MP4
after-native.MP4
after-ionic.MP4
after-none.MP4

Does this introduce a breaking change?

  • Yes
  • No

Other information

Dev build: 7.3.4-dev.11694548895.1578981b

@github-actions github-actions bot added the package: core @ionic/core package label Sep 13, 2023
@liamdebeasi liamdebeasi changed the title fix(scroll-assist): improve input scroll accuracy fix(scroll-assist): improve input scroll accuracy with native resizing Sep 13, 2023
itemEl.getBoundingClientRect(),
contentEl.getBoundingClientRect(),
keyboardHeight,
(componentEl as any).ownerDocument.defaultView.innerHeight // TODO(FW-2832): type
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ownerDocument gives you the document for a node: https://developer.mozilla.org/en-US/docs/Web/API/Node/ownerDocument

defaultView gives you the window associated with the document if available: https://developer.mozilla.org/en-US/docs/Web/API/Document/defaultView

I opted to simplify this by using our win util since that accounts for SSR: https://github.com/ionic-team/ionic-framework/pull/28169/files#diff-ebd08eae5669c5c10420cdb5d258ee6db362b7796cc76ee15e34441a84d48888R51

@liamdebeasi liamdebeasi marked this pull request as ready for review September 13, 2023 20:32
@liamdebeasi liamdebeasi requested review from a team and brandyscarney and removed request for a team and brandyscarney September 13, 2023 20:33
@liamdebeasi liamdebeasi added this pull request to the merge queue Sep 19, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 19, 2023
@liamdebeasi liamdebeasi added this pull request to the merge queue Sep 19, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Sep 19, 2023
@liamdebeasi liamdebeasi added this pull request to the merge queue Sep 19, 2023
Merged via the queue into main with commit b5c736f Sep 19, 2023
@liamdebeasi liamdebeasi deleted the scroll-data-accuracy-fix branch September 19, 2023 16:31
liamdebeasi added a commit that referenced this pull request Sep 22, 2023
#28169)

Issue number: Part of #22940

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->

While working on a fix for #22940, I discovered another bug that
impacted the reliability of my proposed fix for #22940. When we compute
the scroll data (i.e. how much the input needs to be scrolled by), we
subtract the `keyboardHeight` from the `platformHeight` (i.e. the
viewport height):
https://github.com/ionic-team/ionic-framework/blob/1015c06cbef4ceb10d43e722157c04844d984509/core/src/utils/input-shims/hacks/scroll-data.ts#L34

Every time we tap between inputs (even if the keyboard is already open)
we re-run scroll assist because the newly focused input could be
partially obscured by the keyboard. However, in this case we scroll by
too much because we effectively subtract the keyboard height twice. This
is because by the time we compute `platformHeight`, the platform
dimensions have already shrunk to account for the keyboard (when the
webview resizes).

As a result, when we subtract `keyboardHeight` we get a visible area
that is much smaller than the actual visible area.

Examples below with different resize modes. Notice that with the
"Native" resize mode (entire webview resizes when keyboard is open)
tapping into other inputs scrolls the content by much more than it needs
to.

| Body | Native | Ionic | None |
| - | - | - | - |
| <video
src="https://github.com/ionic-team/ionic-framework/assets/2721089/06d1cd20-0349-4a59-ad85-c1c8a8a03caa"></video>
| <video
src="https://github.com/ionic-team/ionic-framework/assets/2721089/1d4e8363-a69b-45c4-931c-d6227e548ec9"></video>
| <video
src="https://github.com/ionic-team/ionic-framework/assets/2721089/7e4304c1-7d56-48c8-aed8-16fc7e51641a"></video>
| <video
src="https://github.com/ionic-team/ionic-framework/assets/2721089/7869c5e0-b202-46e1-af82-49e41b3b067e"></video>
|

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- We now compute the viewport height on load rather than on focus. This
ensures that we always use the full viewport height when computing the
visible area.

| Body | Native | Ionic | None |
| - | - | - | - |
| <video
src="https://github.com/ionic-team/ionic-framework/assets/2721089/c5a66287-0cad-42db-bece-da16edad60e3"></video>
| <video
src="https://github.com/ionic-team/ionic-framework/assets/2721089/372a45c8-e8bd-43d2-bf50-d87b7250e9b3"></video>
| <video
src="https://github.com/ionic-team/ionic-framework/assets/2721089/3d656467-8e2e-48cc-8d72-dc89a67ef8b1"></video>
| <video
src="https://github.com/ionic-team/ionic-framework/assets/2721089/19969535-7d06-404c-98e4-ae49957e0ffe"></video>
|



## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

Dev build: `7.3.4-dev.11694548895.1578981b`
liamdebeasi added a commit that referenced this pull request Sep 26, 2023
#28169)

Issue number: Part of #22940

---------

<!-- Please do not submit updates to dependencies unless it fixes an
issue. -->

<!-- Please try to limit your pull request to one type (bugfix, feature,
etc). Submit multiple pull requests if needed. -->

## What is the current behavior?
<!-- Please describe the current behavior that you are modifying. -->

While working on a fix for #22940, I discovered another bug that
impacted the reliability of my proposed fix for #22940. When we compute
the scroll data (i.e. how much the input needs to be scrolled by), we
subtract the `keyboardHeight` from the `platformHeight` (i.e. the
viewport height):
https://github.com/ionic-team/ionic-framework/blob/1015c06cbef4ceb10d43e722157c04844d984509/core/src/utils/input-shims/hacks/scroll-data.ts#L34

Every time we tap between inputs (even if the keyboard is already open)
we re-run scroll assist because the newly focused input could be
partially obscured by the keyboard. However, in this case we scroll by
too much because we effectively subtract the keyboard height twice. This
is because by the time we compute `platformHeight`, the platform
dimensions have already shrunk to account for the keyboard (when the
webview resizes).

As a result, when we subtract `keyboardHeight` we get a visible area
that is much smaller than the actual visible area.

Examples below with different resize modes. Notice that with the
"Native" resize mode (entire webview resizes when keyboard is open)
tapping into other inputs scrolls the content by much more than it needs
to.

| Body | Native | Ionic | None |
| - | - | - | - |
| <video
src="https://github.com/ionic-team/ionic-framework/assets/2721089/06d1cd20-0349-4a59-ad85-c1c8a8a03caa"></video>
| <video
src="https://github.com/ionic-team/ionic-framework/assets/2721089/1d4e8363-a69b-45c4-931c-d6227e548ec9"></video>
| <video
src="https://github.com/ionic-team/ionic-framework/assets/2721089/7e4304c1-7d56-48c8-aed8-16fc7e51641a"></video>
| <video
src="https://github.com/ionic-team/ionic-framework/assets/2721089/7869c5e0-b202-46e1-af82-49e41b3b067e"></video>
|

## What is the new behavior?
<!-- Please describe the behavior or changes that are being added by
this PR. -->

- We now compute the viewport height on load rather than on focus. This
ensures that we always use the full viewport height when computing the
visible area.

| Body | Native | Ionic | None |
| - | - | - | - |
| <video
src="https://github.com/ionic-team/ionic-framework/assets/2721089/c5a66287-0cad-42db-bece-da16edad60e3"></video>
| <video
src="https://github.com/ionic-team/ionic-framework/assets/2721089/372a45c8-e8bd-43d2-bf50-d87b7250e9b3"></video>
| <video
src="https://github.com/ionic-team/ionic-framework/assets/2721089/3d656467-8e2e-48cc-8d72-dc89a67ef8b1"></video>
| <video
src="https://github.com/ionic-team/ionic-framework/assets/2721089/19969535-7d06-404c-98e4-ae49957e0ffe"></video>
|



## Does this introduce a breaking change?

- [ ] Yes
- [x] No

<!-- If this introduces a breaking change, please describe the impact
and migration path for existing applications below. -->


## Other information

<!-- Any other information that is important to this PR such as
screenshots of how the component looks before and after the change. -->

Dev build: `7.3.4-dev.11694548895.1578981b`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

package: core @ionic/core package

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants