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

Chrome/Opera/Chromium-only bug (not Safari WebKit): cover image "fit" overflows in next column #144

Open
danielweck opened this issue Nov 23, 2014 · 10 comments

Comments

@danielweck
Copy link
Member

EPUB:
https://readiumfoundation.box.com/s/eruhof2djmz3mmj1qg3a

@danielweck danielweck changed the title Chrome-only bug (not Safari): image "fit" Chrome-only bug (not Safari): cover image "fit" overflows in next column Nov 23, 2014
@danielweck danielweck changed the title Chrome-only bug (not Safari): cover image "fit" overflows in next column Chrome/Opera/Chromium-only bug (not Safari WebKit): cover image "fit" overflows in next column Nov 23, 2014
@johanpoirier
Copy link

Any update on this issue ?

@johanpoirier
Copy link

I've just spend the 3 last hours on this issue :

The cover image doesn't scale up/down correctly because :

  • the cover image has an height of 100%
  • and its parents have no height

For my project, I came up with a quick fix in both readium-shared-js and readium-js :

I think this solution may break things but I had no time to test it thoroughly. I know we can do better but this is a start :-)

@danielweck
Copy link
Member Author

@johanpoirier Thanks!
Regarding height vs. min/max-height: the rationale was probably to avoid interfering with the default dimensions of a "small" amount of HTML content (i.e. preserve whatever the computed height is, when under the MAX value). However, I have just tested Right-To-Left + Vertical-Writing-Mode EPUBs and setting the height explicitly does not seem to interfere with any of the CSS column computations, so I would be happy to use this fix given the benefit of properly-resized "cover" images, across all web browsers. EDIT: ah, there's a regression bug, see comment below #144 (comment)

@danielweck
Copy link
Member Author

Regarding the changes in content_document_fetcher.js: this code is only used by the "cloud reader" configuration (not the "chrome extension" or native ReadiumSDK "launcher apps"), so I would be reluctant to implement it that way. Instead, a solution in readium-shared-js would be more appropriate, as it would benefit all platforms at once.

@danielweck
Copy link
Member Author

Unfortunately, explicitly setting the height (either 100% or fixed pixel value) breaks single-page reflow pagination, because an additional CSS column incorrectly gets inserted after the correct page. I can reproduce this behaviour with Firefox, Chrome, Safari on OSX, with the "Accessible EPUB3" ebook that's in the readium-js-viewer repository (under the epub_content/ folder).

The reason is that the html element's computed CSS style reports a scrollWidth that is larger than the actual width of its body.

@johanpoirier
Copy link

Thanks for your feedback @danielweck. As I was saying in my first comment, this is not the perfect solution.

By the way, I will probably have more time to work on the readium cloud reader in the beginning of next year.

@danielweck
Copy link
Member Author

Thanks a lot @johanpoirier your contributions and/or experimentations are much welcome! :)

@danielweck
Copy link
Member Author

Related: #138 and #88

@JayPanoz
Copy link

The reason is that the html element's computed CSS style reports a scrollWidth that is larger than the actual width of its body.

We had this bug in an early version of Readium CSS since we had to find a way to support images with position: absolute; bottom: 0 on a title page in one of our samples—it turns out some authors don’t bother setting an explicit height and position: relative for the parent and think position: absolute for the img will automagically work.

TL;DR: because of margin collapse, body’s margin was the first element’s (h1) so although the height was 100%, there was this margin creating another column in single-page view.

As mentioned, setting an explicit height to the body element wouldn’t solve all issues though, since % values depends on the parent element so if there is an extra section, figure or div in the markup, the image would still overflow if the author is using height: 100% for instance. The rendering engine would not be able to compute a value for 100% since some of its parent elements would not have an explicit height.

Consequently, we ended up creating safeguards for media elements, with a max-height set in vh. It’s like % but it depends on the viewport height, not the parent element’s. Support is excellent.

Please note it’s 95vh to account for an hypothetical figcaption or margin-top, some authors nesting img in paragraphs….

You might want to get rid of this safeguard in scroll view though. I know I had some weird issues in EPUB files when using vh for images.

We’re also using object-fit: contain; to keep the correct aspect-ratio but this one won’t work in IE11 and Edge < 16.

There are polyfills available though:

@danielweck
Copy link
Member Author

Useful update, thanks @JayPanoz

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants