search

readium / readium-shared-js

Repository for the shared JavaScript libraries that are used in the SDK-Launchers and other applications developed on top of the SDK
BSD 3-Clause "New" or "Revised" License
78 stars 101 forks source link

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

Open danielweck opened 9 years ago

danielweck commented 9 years ago

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

johanpoirier commented 9 years ago

Any update on this issue ?

johanpoirier commented 9 years ago

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

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

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 commented 9 years ago

@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 https://github.com/readium/readium-shared-js/issues/144#issuecomment-66154039

danielweck commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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

danielweck commented 9 years ago

Related: https://github.com/readium/readium-shared-js/issues/138 and https://github.com/readium/readium-shared-js/issues/88

JayPanoz commented 7 years ago

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 commented 7 years ago

Useful update, thanks @JayPanoz