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

[BUG] content area scroll does not work correctly after data is changed in another screen #104

Closed
ebhsgit opened this issue Dec 10, 2020 · 9 comments · Fixed by #107
Closed
Assignees
Labels
android bug Something isn't working

Comments

@ebhsgit
Copy link
Contributor

ebhsgit commented Dec 10, 2020

Describe the bug
My app is built on Ionic 3.
On the root page is a cupertino-pane.
The cupertino pane has a header and content area. The content shows a list bound to an array of items.

The user can push another view to edit any of the items.

When the user changes the a value of any of the items in the list, and pop back to the root page. The content area can not scroll to the bottom of the list.

To Reproduce
You will need a project that has the setup as described above

Example project
https://stackblitz.com/edit/ionic-boe2na

NOTE: Problem is not reproduced in the sample project, because the dependency is using older library - 1.1.652
Unable to use latest code in stackblitz.com

Expected behavior
The content area scrolls correctly

Smartphone (please complete the following information):

  • Device: S9+ and emulators

Additional context
The problem is due to setOverflowHeight() calculating incorrect value, due to this.overflowEl.offsetTop being 0.

Using my page as an example.

The height of the pane is 300px.

On first showing the pane, the heights are calculated correctly.

  • header area is 70px
  • content area is 215px.

When setOverflowHeight() calculates this first height, the this.overflowEl.offsetTop is 85.

When an item is being edited in another page. setOverflowHeight() is called, but the calculated height is set to 300px.
The problem is because this.overflowEl.offsetTop is 0.

@ebhsgit ebhsgit changed the title [BUG] content area scroll does not work correctly when data is editted in another screen [BUG] content area scroll does not work correctly when data is changed in another screen Dec 10, 2020
@ebhsgit ebhsgit changed the title [BUG] content area scroll does not work correctly when data is changed in another screen [BUG] content area scroll does not work correctly after data is changed in another screen Dec 10, 2020
@roman-rr roman-rr self-assigned this Dec 10, 2020
@roman-rr roman-rr added the bug Something isn't working label Dec 10, 2020
@roman-rr
Copy link
Collaborator

@ebhsgit Thank you for issue!
I can't reproduce with ionic 4 and latest package version from master branch.

What i have seen from your example, you are using ionViewDidLoad() and pane represented all times that you come into page.
First, try to use ngOnInit() instead of ionViewDidLoad(). Pane will not be re-calculated and re-presented.
If this is not help in your case, provide me any RELATED code sources with issue.

@ebhsgit
Copy link
Contributor Author

ebhsgit commented Dec 11, 2020

@roman-rr Thanks for getting back to me.

You are correct. On my root page, the pane is always presented, and only loaded on load.

I've reproduced the issue. Please use the source code from below
https://github.com/ebhsgit/cupertino-pane-test/tree/bug-104

Video of the issue
https://github.com/ebhsgit/cupertino-pane-test/blob/bug-104/bug.mp4

Steps to reproduce.

  1. Start app
  2. Scroll to bottom of the list in pane
  3. Click Change Data
  4. On Edit page, change the text
  5. Close the keyboard <---- Critical step
  6. Go back to home page
  7. Notice the list does not scroll to bottom any more.

Note
Step 5 is critical. If you just made changes in the text box, and tap the back button, then the bug is does not happen.

I think the problem is caused by the keyboard hide event, and the presented pane being in the background page.

My app has another page in between the root and the data edit page. So when the user change data in the edit page, and use the back button, it still causes the issue.

@ebhsgit
Copy link
Contributor Author

ebhsgit commented Dec 11, 2020

Added an intermediate page to cause the bug even without user manually closing the keyboard.

Steps

  1. Start app
  2. Scroll to bottom of the list in pane
  3. Click Change Data
  4. On Another page, click Continue button
  5. On Edit page, tap the textbox and make changes to the text
  6. Go back to home page
  7. Notice the list does not scroll to bottom any more.

@roman-rr
Copy link
Collaborator

@ebhsgit When you route to some page, previous page got display: none;.
When keyboard called, pane re-calc height for overflow element, and as for display:none element child, offsetTop calculated as 0.

Your solution must be additional re-calculating overflow height with method on home.ts

ionViewWillEnter() {
    if (this.btmPane) {
      this.btmPane.setOverflowHeight();
    }
}

Other way is to set offsetTop to static in package on first load. But dynamic cases must have for some users, and i prefer to await if more similar cases occurs.

@ebhsgit
Copy link
Contributor Author

ebhsgit commented Dec 13, 2020

Thank @roman-rr

I got caught up with other stuff and didn't get to test master branch earlier.


I can confirm adding

ionViewWillEnter() {
    if (this.btmPane) {
      this.btmPane.setOverflowHeight();
    }
}

Fixes the issue, even on the previous version (without using the changes from the latest master)

@ebhsgit
Copy link
Contributor Author

ebhsgit commented Dec 14, 2020

@roman-rr

I'm thinking it is better if the library itself does not call setOverflowHeight when the page/container is display:none.
Do you know if this change will cause other issue?

Reason for making the change in the library.

  • Users may not know they need to add the workaround code to fix this issue
  • If app has many pages that use the pane, then there is a lot of "duplicate" code
  • Also notice a minor issue, described below

Issue
I'm noticing that the visible position of the list in the pane is different when returning the page after the keyboard is open/closed.

This is due to setOverflowHeight() being called on the keyboard events. As you noticed, offsetTop is 0 because parent is display: none, this makes the size of the content bigger. Then in ionViewWillEnter, the size is recalculated to be smaller.

Change data Button is visible before leaving page
before

Button is no longer visible when returning to page
after

This is not critical issue, and is acceptable for me.


In terms of how to implement it, one way is to provide a property in the Settings class to accept page/container selector.
Then in the functions that call setOverflowHeight, check that page/container is not display:none. OR directly inside setOverflowHeight, check if page/container is display:none

If the property is not set, then use current behaviour, and it is upto the user to apply the workaround on every page.

@ebhsgit
Copy link
Contributor Author

ebhsgit commented Dec 14, 2020

Alternative implementation

Just check const isVisible = (this.overflowEl.offsetHeight == 0 && this.overflowEl.offsetWidth == 0) .

If not visible don't setOverflowHeight

@ebhsgit
Copy link
Contributor Author

ebhsgit commented Dec 14, 2020

@roman-rr
This is broken again in latest master.

I've updated my test project with reference to latest master
https://github.com/ebhsgit/cupertino-pane-test/tree/bug-104


Likely caused by "Fix on pull" commit.
The changeset removed the isHidden check from setOverflowHeight() but did not add check in the keyboard events (onKeyboardShow and onKeyboardHide).

roman-rr added a commit that referenced this issue Dec 15, 2020
@roman-rr
Copy link
Collaborator

@ebhsgit Should be fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
android bug Something isn't working
Projects
None yet
2 participants