-
Notifications
You must be signed in to change notification settings - Fork 20
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
image scaling (fit into viweport), cover pages (HTML), surrounding markup, authored CSS, etc. (causes for unnecessary scroll extent / overflow due to margins-paddings) #55
Comments
Insightful comments by @JayPanoz starting here: |
As regards “possibly related”.
So to sum up, this issue can be scoped to the additional margin we’re setting up in scroll mode. readium-css/css/src/modules/ReadiumCSS-scroll.css Lines 24 to 30 in 051e3d2
Thanks for opening this issue BTW. 🙏 |
:) ...well, unless the said "web view" is Electron's own interpretation/implementation (derived from Chromium webview) which is in fact an embedded object (custom element + shadow DOM) that emulates a sandboxed iframe (isolated in a separate process), so See: |
Right, I’ll follow up tomorrow as I’m quite frankly exhausted right now and some comments may appear passive-agressive – sorry about that, wasn’t my intention – but we’d better have a solid reason for changing how that works, especially as it’s in the 2017 CSS snapshot, on which EPUB 3.2 is now relying – and well, if I’m being honest and although I’m trying really hard to be neutral, it’s very difficult to not be on some kind of a mission as I personally saw the nicest and most talented people quit ebooks because ebooks break such standards and they couldn’t take it anymore. For At first I thought it would maybe impact |
That + some vendors advise
Source: https://help.apple.com/itc/booksassetguide/#/itca71ad3c33 |
But then again, note the primary issue is about adding a Works for text but not for files consisting of images only. So I’d really like to scope this issue to that and see what our options are – not so much using CSS only unfortunately, but it’s worth discussing imho. |
Alright, here’s an illustrated recap on scroll styling. So right now, we have the following box model for Don’t pay attention to horizontal margins (
But why did we set vertical margins? It was all about text. Note: In the following screenshots, orange (outer) is For instance, here’s a chapter without any I can recall a heading in our test files that was somehow partly hidden at the top of the chapter because we didn’t apply any margin. I can’t remember why exactly (maybe negative And same for the bottom of chapters. Without a margin With a margin So the idea was to give some extra vertical spacing for content to breathe. Let’s now look at images (e.g. cover). Without a margin (note we constrain the height to With a margin I can confirm there’s overflow in this last screenshot, although you can’t see the scrollbar (because MacOS is buggy). But now the height of the document is the height of the image ( This is obviously something we don’t want, we want no extra vertical margins there. What the issue is In CSS, you can’t conditionally apply margins depending on the document’s content (text vs. only one image). I indeed can’t use such logic:
Because when checking this image, I’m already getting out of All I can do is expecting some kind of flag, so in ReadiumCSS by default, a custom property such as
So all we can do with vanilla CSS is either applying vertical margins to all documents or not applying them. Hypothetical fixes Such logic is JS territory, either in the form of:
|
Related: |
After further reflection, I’m wondering whether rolling back to no vertical margins and letting the app handle those, as in the paged view, wouldn’t be the most reasonable call. That would enforce consistency for implementers, which in my opinion is most important. |
I have observed a similar stretching of cover image issue with ebook. The cover image is rendered from start.html and since it is provided by the publisher it can have different html and css properties. For example i am showing below the html of two ebooks where in first book the cover image rendered perfectly but in other it gets stretched to multiple pages. HTML in start.html where cover image does not stretch.
Cover image is stretch in this book
Apparently we have observed this issue when migrated to recent readium-sdk along with LCP. In the previous setup which was using 2-3 yr old readium this problem was not so much evident. We are using readium on android & IOS platform. Is there any easy fix for this ? Thanks |
Just to be clear: you mean Readium2, where ReadiumCSS is used to apply document formatting, right? |
We are using Readium1 from this branch : https://github.com/readium/readium-sdk/tree/feature/latest-working-build-config/Platform/Android |
Oh, so you are not using ReadiumCSS, right? |
For Readium1, please report in (the image handling code in Readium1 SDK has not changed for months/years, but your migration from a very old revision of Readium1 may indeed have introduced different behaviours / regression bugs) |
Yes we are not using ReadiumCSS in the current setup. Also we were not using it earlier. In asset folder there are some css file present such as SDK.css etc.. I believe readiumCSS is applicable only to readium 2 and onward, right? Thanks i will report in readium-shared-js project. |
So yeah, readiumCSS is applicable to r2 onwards. Note we have a list of CSS-related issues from r1 that this project is attempting to fix in the wiki: https://github.com/readium/readium-css/wiki/CSS-related-issues-in-Readium-1 – uncrossed items don’t necessarily mean we haven’t fixed it, maybe it doesn’t apply, or we haven’t had the opportunity to address it yet, etc. |
cc @danielweck Removed those vertical margins in |
Thanks Jiminy, I've merged your updates into the ReadiumCSS copy of Test with Metropolis (which I found in your screenshots readium/r2-navigator-js#18 (comment) ), from Feedbooks: Load the remote EPUB directly in Cover and text in paginated mode (CSS columns): Cover and text in scroll mode: As you can see, no extraneous scroll extent, thanks to the removed top/bottom margins in ReadiumCSS. |
No problem, and thanks for the confirmation. Will resolve this issue when merging this change. |
Fixed via #63 |
Issue originally discussed at:
readium/r2-navigator-js#18
The text was updated successfully, but these errors were encountered: