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

Preserve vh/vw units in compatibility CSS #747

Merged
merged 3 commits into from
Sep 6, 2024

Conversation

gvtulder
Copy link
Contributor

@gvtulder gvtulder commented Sep 6, 2024

In the Apple Books app on iOS, full-page images are often spread over multiple pages:

Example from How the other half lives:
before 1 before 2 before 3

The full-page figures are styled with max-height: 100vh (see CSS from the book), but the compatibility code in se build converts the vh units to percentages. Apparently this doesn't work for full-page images in Apple Books. (Cantook on iOS doesn't have this problem: it show the images on a single page.)

This patch changes the compatibility code so that it still adds the percentages, but also keeps the vh and vm units for readers that support this (such as the Apple Books app):

	max-height: 100%;
	max-height: 100vh;

This fixes the problem, see this image from the Books app:
after


The change itself is quite small, but testing it is a bit more involved. This is the first test using se build, as far as I can see. The test actually runs two commands: se build to build the epub, followed by se extract-ebook to extract the files that can be compared against the golden version.

(I'd be happy to submit a patch without the test, if the test framework is out of scope for this PR.)


The third change (the middle commit) removes an unused compatibility rule that is supposed to replace margin-top: 20vh with margin-top: 5em. As far as I can see, this doesn't currently do anything, because the 20vh is changed to 20% before this rule is reached. For an input of:

	max-height: 20vh;

the current rules generate CSS:

	max-height: 20%;

However, now that the main change in this PR preserves the vh, it would generate this:

	max-height: 20%;
	max-height: 5em;

so I think it is better to remove the old rule. The result would then be:

	max-height: 20%;
	max-height: 20vh;

@acabal
Copy link
Member

acabal commented Sep 6, 2024

Great work, thanks!

@acabal acabal merged commit 8b402cf into standardebooks:master Sep 6, 2024
1 check passed
@gvtulder gvtulder deleted the f-css-vh-vw branch September 23, 2024 22:13
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.

2 participants