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 zoom behavior on mobile #935

Merged
merged 5 commits into from Apr 29, 2024
Merged

Fix zoom behavior on mobile #935

merged 5 commits into from Apr 29, 2024

Conversation

cowuake
Copy link
Contributor

@cowuake cowuake commented Apr 28, 2024

Change preliminarily discussed in #859.
The change fixes an erratic behavior when zooming in with a mobile web browser.
The terminal height was changed accordingly to a change in the visual viewport, but without considering the scale factor. This led, e.g., to a terminal area half the expected one for a 2X zoom level.
The fix does not alter the resizing behavior at constant scale factor for the appearing and disappearing of a virtual keyboard.

@coveralls
Copy link

coveralls commented Apr 28, 2024

Coverage Status

coverage: 82.704% (-0.03%) from 82.729%
when pulling e94c9c3 on cowuake:devel
into df3506a on jcubic:devel.

@cowuake
Copy link
Contributor Author

cowuake commented Apr 28, 2024

@jcubic, I had to revert the commit that implemented the refactoring you asked for in #859. The reason is that comparing the heights already multiplied by the scale and then rounded breaks again the behavior with the virtual keyboard - I tested the change on Android 13 with Firefox/Chrome/Edge.

@jcubic
Copy link
Owner

jcubic commented Apr 28, 2024

How it breaks if it's exact same code:

var height = window.visualViewport.height;
var scale = window.visualViewport.scale;
height = Math.round(height * scale);

and this is the same:

var scale = window.visualViewport.scale;
var height = Math.round(window.visualViewport.height * scale);

You just do this in two steps instead of one.

@jcubic
Copy link
Owner

jcubic commented Apr 28, 2024

Ok it seems you added this line for no reason:

var newHeight = Math.round(window.visualViewport.height * newScale);

this code was on in first commit.

@cowuake
Copy link
Contributor Author

cowuake commented Apr 29, 2024

How it breaks if it's exact same code:

var height = window.visualViewport.height;
var scale = window.visualViewport.scale;
height = Math.round(height * scale);

and this is the same:

var scale = window.visualViewport.scale;
var height = Math.round(window.visualViewport.height * scale);

You just do this in two steps instead of one.

OK, so you wanted only those lines changed, i.e.:

var scale = window.visualViewport.scale;
var height = Math.round(window.visualViewport.height * scale);
callback(height);
window.visualViewport.addEventListener('resize', function() {
    var newHeight = window.visualViewport.height;
    var newScale = window.visualViewport.scale;
    if (height !== newHeight) {
        height = Math.round(newHeight * newScale);
        callback(height);
    }
});

This way we are comparing a scaled height with a non-scaled height (unless I'm missing something!!) with height !== newHeight, and that's the reason why I also changed the code below - the error was not reassigning the value of newHeight to height before callback invocation, hence the problematic behavior!
If you're satisfied with this solution I will update the PR accordingly: I'd just move the newScale initialization/assignment down, since when we don't have to change the height we don't have to read the scaling factor either.
It would become:

var scale = window.visualViewport.scale;
var height = Math.round(window.visualViewport.height * scale);
callback(height);
window.visualViewport.addEventListener('resize', function() {
    var newHeight = window.visualViewport.height;
    if (height !== newHeight) {
        var newScale = window.visualViewport.scale;
        height = Math.round(newHeight * newScale);
        callback(height);
    }
});

Storing the scaling factors in the scale and newScale variables could be avoided tout-court, but I'll follow your advice.

@jcubic
Copy link
Owner

jcubic commented Apr 29, 2024

Yea, you're right, but your code also have the same bug you compare scaled with non-scaled. It is triggering every time.

@cowuake
Copy link
Contributor Author

cowuake commented Apr 29, 2024

Yea, you're right, but your code also have the same bug you compare scaled with non-scaled. It is triggering every time.

Updated the PR. Now it should all be OK. Tested again on Android 13.
I changed more than those three initial lines but this way we only compare scaled heights.
I fixed the issue that we had with my first (erroneous) refactoring.

@jcubic jcubic merged commit 8615ac7 into jcubic:devel Apr 29, 2024
3 of 4 checks passed
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.

None yet

3 participants