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.

Sign up for GitHub

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

Jump to bottom

Add extra padding for zoomed in web pages #887

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Add extra padding for zoomed in web pages #887

wants to merge 2 commits into from

Conversation

BPerlakiH
Copy link
Collaborator

Fixes: #885

Origin

I have found the zoom functionality is applying a JS callback to adjust the CSS values of the page:

kiwix-apple/Views/BuildingBlocks/WebView.swift

Lines 213 to 220 in c0539cc

extension WKWebView {
func adjustTextSize(pageZoom: Double? = nil) {
let pageZoom = pageZoom ?? Defaults[.webViewPageZoom]
let template = "document.getElementsByTagName('body')[0].style.webkitTextSizeAdjust='%.0f%%'"
let javascript = String(format: template, pageZoom * 100)
evaluateJavaScript(javascript, completionHandler: nil)
}
}

Proposed solution

I tried adjustments on the the swift view level, but that did not look good.

Therefore, I am proposing to adjust the webkit padding in a similar fashion as the text size increase is done for zooming in.

As of my testing it looks good on all devices (iPhone, iPad, macOS).

@BPerlakiH BPerlakiH linked an issue Jul 28, 2024 that may be closed by this pull request
Content margin a removed when no sidebar #885
Open
@kelson42
Copy link
Contributor

I don't think this is a good idea at all to modify the css or dom. Why do we do that?

@BPerlakiH
Copy link
Collaborator Author

This was the implementation of the "page zoom" functionality.

@kelson42
Copy link
Contributor

@BPerlakiH There should be some kind of zoom provided by the html widget itself

@BPerlakiH
Copy link
Collaborator Author

We have the default pinch gesture to zoom:

pinch_to_zoom.mov

@kelson42
Copy link
Contributor

pinch is not what we need or talk about here!

@BPerlakiH
Copy link
Collaborator Author

We have the "magnify" option for the webView, but that zooms in a similar fashion as the pinch zoom, making the letters relatively larger, but you need to scroll horizontally as well to read it.
With the current solution the margins remain nicely in place, and the text wraps at the side of the screen.

@BPerlakiH BPerlakiH force-pushed the 885-content-margin-a-removed-when-no-sidebar branch from db471aa to ec3de16 Compare August 2, 2024 08:58
@BPerlakiH BPerlakiH force-pushed the 885-content-margin-a-removed-when-no-sidebar branch from ec3de16 to 7ea2776 Compare August 4, 2024 11:30
@BPerlakiH BPerlakiH force-pushed the 885-content-margin-a-removed-when-no-sidebar branch from 7ea2776 to b6ac11d Compare August 5, 2024 17:44
@codecov-commenter
Copy link

codecov-commenter commented Aug 5, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 0% with 7 lines in your changes missing coverage. Please review.

Project coverage is 39.96%. Comparing base (99356e3) to head (b6ac11d).

Files Patch % Lines
Model/Utilities/Javascript.swift 0.00% 5 Missing ⚠️
Views/BuildingBlocks/WebView.swift 0.00% 2 Missing ⚠️
Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #887      +/-   ##
==========================================
- Coverage   40.00%   39.96%   -0.05%     
==========================================
  Files         108      109       +1     
  Lines        6186     6193       +7     
==========================================
  Hits         2475     2475              
- Misses       3711     3718       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@BPerlakiH BPerlakiH marked this pull request as draft September 21, 2024 13:48
@BPerlakiH
Copy link
Collaborator Author

Converted to draft, as it's a stale PR / issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rgaudin rgaudin Awaiting requested review from rgaudin

@kelson42 kelson42 Awaiting requested review from kelson42

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Content margin a removed when no sidebar
3 participants
@BPerlakiH @kelson42 @codecov-commenter