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

Suggested fix for Autoscroll not happening #62

Merged
merged 1 commit into from
Jun 21, 2022
Merged

Conversation

revaij83
Copy link
Contributor

As far as I could understand how the autoscroll and the modified section of the code works:
1.) We calculate a padding we add to the body to increase it's innerheight so that the keyboard does not cover anything from the contents.
2.) If autoscroll enabled we scroll to the input.

Why I changed modified the inputTreshold calculation:
The issue described in #61 was caused by the inputTreshold variable.
In this scenario:

window size: 1024x768
keyboardsize: 1024x540
theInput.getBoundingClientRect()
DOMRect {x: 239, y: 238, width: 546, height: 54, top: 238, …}

Both isPaddingTop and isPaddingBottom are false. If the inputTreshold variable hadn't decreased the theInputOffsetTop variable's value, the isPaddingBottom would be true.

If I understand the purpose of the inputTreshold variable, it ensures that when the scrolling happens, there is some space left between the keyboard and the input. I suggest to use this variable only when calculating the scroll position, and not when we calculate wether we need body padding or not.

Why I introduced the new inputVisibleEdge variable:
When deciding wether we need the a body padding we want to ensure that in case of a bottom keyboard the top of the input remains visible, in case of a top keyboard the bottom of the keyboard stays visible. When deciding where to scroll, I'd always want to scroll to the top of the input.

The original inputTop and theInputOffsetTop variables were initialized so:
var inputTop = (isPlacementTop ? theInput.getBoundingClientRect().bottom : theInput.getBoundingClientRect().top) || 0;
var inputThreshold = isPlacementTop ? (theInput.clientHeight + 20) : 50;
var theInputOffsetTop = Math.round(inputTop + docTop) - inputThreshold;

So in case of a top keyboard the code first set inputTop to the bottom of the input, then decreased this with the height of the input, which is the top of the input. So theInputOffsetTop was the top of the input in both cases increased by 20 or 50. This was ok for the scrolling, but not ok when inspecting wether we need a body padding or not.

Why I recalculated the boundingClientRect of the input before scrolling:
In case of a top keyboard, when padding top is added to the body the boundingClientRect will change, and has to be recalculated.

As far as I could understand how the autoscroll and the modified section of the code works:
1.) We calculate a padding we add to the body to increase it's innerheight so that the keyboard does not cover anything from the contents.
2.) If autoscroll enabled we scroll to the input.

Why I changed modified the inputTreshold calculation:
The issue described in furcan#61 was caused by the inputTreshold variable. 
In this scenario:

window size: 1024x768
keyboardsize: 1024x540
theInput.getBoundingClientRect()
DOMRect {x: 239, y: 238, width: 546, height: 54, top: 238, …}

Both isPaddingTop and isPaddingBottom are false. If the inputTreshold variable hadn't decreased the theInputOffsetTop variable's value, the isPaddingBottom would be true.

If I understand the purpose of the inputTreshold variable, it ensures that when the scrolling happens, there is some space left between the keyboard and the input. I suggest to use this variable only when calculating the scroll position, and not when we calculate wether we need body padding or not. 

Why I introduced the new inputVisibleEdge variable:
When deciding wether we need the a body padding we want to ensure that in case of a bottom keyboard the top of the input remains visible, in case of a top keyboard the bottom of the keyboard stays visible. When deciding where to scroll, I'd always want to scroll to the top of the input. 

The original inputTop and theInputOffsetTop variables were initialized so:
var inputTop = (isPlacementTop ? theInput.getBoundingClientRect().bottom : theInput.getBoundingClientRect().top) || 0;
var inputThreshold = isPlacementTop ? (theInput.clientHeight + 20) : 50;
var theInputOffsetTop = Math.round(inputTop + docTop) - inputThreshold;

So in case of a top keyboard the code first set inputTop to the bottom of the input, then decreased this with the height of the input, which is the top of the input. So theInputOffsetTop was the top of the input in both cases increased by 20 or 50. This was ok for the scrolling, but not ok when inspecting wether we need a body padding or not. 

Why I recalculated the boundingClientRect of the input before scrolling:
In case of a top keyboard, when padding top is added to the body the boundingClientRect will change, and has to be recalculated.
@revaij83 revaij83 marked this pull request as ready for review June 16, 2022 12:54
@revaij83 revaij83 marked this pull request as draft June 16, 2022 12:57
@revaij83 revaij83 marked this pull request as ready for review June 16, 2022 12:57
@revaij83
Copy link
Contributor Author

I'm very new to using github, and cannot figure out how to attach #61 to this pr.

@furcan
Copy link
Owner

furcan commented Jun 18, 2022

Hello @revaij83 ,
I will review it ASAP.
Thank you for your contribution.

@surexxx
Copy link
Contributor

surexxx commented Jun 20, 2022

Hi @furcan,

just to know: @revaij83 is a member of our team :)

@furcan furcan merged commit 2494e89 into furcan:main Jun 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants