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

bump crengine: tables rendering improvements and others #4378

Merged
merged 2 commits into from Dec 7, 2018

Conversation

Projects
None yet
3 participants
@poire-z
Copy link
Contributor

poire-z commented Dec 5, 2018

bump crengine, which includes koreader/crengine#243 and koreader/crengine#245 :

  • Adds support for symbol fonts (local or embedded) (See #4196 (comment) Closes #4196.)
  • Fix some issues when rendering text in constrained width
  • Page splitting: fix possible missing blocks
  • CSS: adds support for 'auto', ignore % for borders
  • Fix right border drawing position
  • Fix: adds missing properties in copystyle()
  • Adds comments, erm_killed rendering method
  • Adds getRenderedWidths(): get node min/max node content width
  • Tables rendering: fixes and improvements (Closes #2846, vertical-align now supported)
  • getRenderedWidths: enable min_width to be a single CJK char
  • Fix wrong text wrap avoid in some case
  • epub.css: add style for 'blockquote'
  • Fix rendering issue when line ends with an image

Adds a few style tweaks related to tables (corrections or alternate english wording welcome :)

Enforce table width: 100% in Wikipedia EPUBs to keep previous look, which feels better with the various kinds of tables in Wikipedia pages.

Also includes: Bump FBInk to 1.9.2 (koreader/koreader-base#770) Bump libpng to 1.6.36 koreader/koreader-base#771 and some android build tweaks koreader/koreader-base#750

Also includes a fix for diagonal swipe to refresh full screen not working in TOC and History (see #4240 (comment)).

Fix diagonal refresh not working in TOC and History
By delegating diagonal swipe handling to GestureManager only
when the Menu or FileChooser instance is the FileManager one.

@poire-z poire-z force-pushed the poire-z:bump_crengine branch from ffe5286 to 2863bf4 Dec 5, 2018

@Frenzie Frenzie referenced this pull request Dec 5, 2018

Merged

Bump libpng to 1.6.36 #771

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Dec 5, 2018

Oh, joy, unit test failures... :(

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Dec 6, 2018

Oh my, juliet.epub is full of <blockquote> ... so it goes from 189 pages to 262 pages...

Old rendering:
image

New rendering:
image

Do we agree the new is better than the old and worth the 40% raise in pages count?

(I'd rather drop the blockquote style than have to adapt all these unit tests :) ... but well, if I have to...)

An other small thing is that the cover page does and did overflow the page height. But in the page splitting code, a SplitLineIfOverflowPage(line) in the first page handling code was missing, that I added.
So, on my 578px page height emulator, the first page is 583px, and the 5px were previously just dropped. Now, they are pushed to next page, which is now a blank page (the cover image is rightly 578px, but because of the "strut", 5px below the baseline (from the DIV font size) is added below the image (and it is the right behaviour that if there are these 5px, they have to not be dropped and pushed to next page):
image

Now, there is another small thing: I didn't read it in the spec, but Firefox seems to not enforce this strut when there is no text (or no non-space-only text) alongside an image:

<div style="background-color: #ff0000">
<img src="Lac_sevan_armenie.png">
</div>

image
Add a single letter (e, or " here) in the DIV, and the strut is enforced:
image

image
with:

<div style="background-color: #ff0000; font-size: 400%">
<img src="Lac_sevan_armenie.png"> "
</div>

crengine currently enforces it even when there is no text:
image
(i see no quick and easy solution to have it does as Firefox - so we'd have to do with a blank page following an inline image with height 100%)

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Dec 6, 2018

Additional note: Firefox does as crengine does when I add at the top of the HTML

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN"
  "http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">

image

When the above "html header/doctype" is missing, all the strut rules stated by the specs (that I added in the vertical-align PR ) are not enforced by Firefox...

So, essential question: is it ok/fine/best that crengine now does what Firefox seems to do only when ...DTD/xhtml11 ??

It's really by chance I had this HTML "header" in my test file when I did the vertical-align PR...

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Dec 6, 2018

Checked firefox with various doctypes mentionned in https://www.w3schools.com/tags/tag_doctype.asp.

strut is enforced with

<!DOCTYPE html>
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Strict//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-strict.dtd">
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN" "http://www.w3.org/TR/xhtml11/DTD/xhtml11.dtd">

strut is NOT enforced with

<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Transitional//EN" "http://www.w3.org/TR/html4/loose.dtd">
<!DOCTYPE HTML PUBLIC "-//W3C//DTD HTML 4.01 Frameset//EN" "http://www.w3.org/TR/html4/frameset.dtd">
<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.0 Transitional//EN" "http://www.w3.org/TR/xhtml1/DTD/xhtml1-transitional.dtd">

(We won't implement different behaviousr based on the doctype in crengine :) we should just have a single most adequate one.)

@poire-z poire-z force-pushed the poire-z:bump_crengine branch from bba38d7 to 822be99 Dec 6, 2018

@NiLuJe

This comment has been minimized.

Copy link
Member

NiLuJe commented Dec 6, 2018

I kinda prefer the new bq rendering, yeah ;). (At least in that context ;)).

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Dec 6, 2018

I agree that the new rendering is more like how one would expect a blockquote to render. Otherwise you'd barely know the difference with a paragraph.

calibre for comparison:

screenshot_2018-12-06_20-28-48

Regarding the strut, you just found out that in between quirks mode and standards mode, there's almost standards mode. ;-)

https://developer.mozilla.org/en-US/docs/Mozilla/Gecko_Almost_Standards_Mode

Particularly here.

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Dec 6, 2018

(We won't implement different behaviousr based on the doctype in crengine :) we should just have a single most adequate one.)

The current behavior is excellent.

@poire-z

This comment has been minimized.

Copy link
Contributor

poire-z commented Dec 6, 2018

Thanks, that explains it all. And thanks for the positives.
Any comment about the style tweaks wording, before I merge this? (I'll look in the mathml/image degradation soon, hoping it does not degrade anything else more classic).

@Frenzie

This comment has been minimized.

Copy link
Member

Frenzie commented Dec 6, 2018

Oh, my bad. I thought it was just the crengine bump. I'll have a look in an hour.

Show resolved Hide resolved frontend/ui/data/css_tweaks.lua Outdated
Show resolved Hide resolved frontend/ui/data/css_tweaks.lua Outdated
Show resolved Hide resolved frontend/ui/data/css_tweaks.lua Outdated
Show resolved Hide resolved frontend/ui/data/css_tweaks.lua Outdated
Show resolved Hide resolved frontend/ui/data/css_tweaks.lua Outdated
Show resolved Hide resolved frontend/ui/data/css_tweaks.lua Outdated
title = _("Make images full width"),
description = _("Useful for books containing only images, when they are smaller than your screen. May stretch images in some cases."),
-- This helped me once with a book. Will mess with aspect ratio
-- when images have a style="width: NNpx; heigh: NNpx"

This comment has been minimized.

@Frenzie

Frenzie Dec 6, 2018

Member

In your average browser you could just set height:auto !important.

bump crengine: tables rendering improvements and others
bump crengine, which includes:
- Adds support for symbol fonts (local or embedded)
- Fix some issues when rendering text in constrained width
- Page splitting: fix possible missing blocks
- CSS: adds support for 'auto', ignore % for borders
- Fix right border drawing position
- Fix: adds missing properties in copystyle()
- Adds comments, erm_killed rendering method
- Adds getRenderedWidths(): get node min/max node content width
- Tables rendering: fixes and improvements
- getRenderedWidths: enable min_width to be a single CJK char
- Fix wrong text wrap avoid in some case
- epub.css: add style for 'blockquote'
- Fix rendering issue when line ends with an image

Adds a few style tweaks related to tables.

Enforce table width: 100% in Wikipedia EPUBs to keep
previous look, which feels better with the various kinds
of tables in Wikipedia pages.

Fix unit tests as juliet.epub (full of blockquotes), grew quite
a few pages with the epub.css update.

@poire-z poire-z force-pushed the poire-z:bump_crengine branch from 61800de to 84fd6eb Dec 7, 2018

@poire-z poire-z merged commit ea946d5 into koreader:master Dec 7, 2018

1 check passed

ci/circleci Your tests passed on CircleCI!
Details

@poire-z poire-z deleted the poire-z:bump_crengine branch Dec 7, 2018

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