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

Non-linear fragments and list item markers tweaks #462

Merged
merged 8 commits into from
Jan 21, 2022

Conversation

poire-z
Copy link
Contributor

@poire-z poire-z commented Jan 19, 2022

LVHashTable::resize(): behave as ::init()

This fixes a floating point error crash when reloading from cache a book with no content.
See koreader/koreader#8583 (comment) .

FB2 series: extract series number as string, not int

Just like it is done for EPUBs since ages.
Will allow closing koreader/koreader#8666 .

Non-linear fragments: more generic handling

Via -cr-hint: non-linear / non-linear-combining, set in stylesheets.
Also consider <section epub:type="footnotes"> and <aside epub:type="footnote"> as non-linear.
See koreader/koreader#8623 (comment) . Will allow closing koreader/koreader#8623 .
Pinging @Jellby for a quick look/review.

CSS: list-style-position: correct alignment for "outside"

Do as web browsers, which align list item markers near the followup content:

   9 some content
  10 some content

Also add private property list-style-position: -cr-outside to be able to get the previous distant alignment:

  9  some content
  10 some content

which is preferable when LI are used for in-page footnotes (like they are with Wikipedia EPUBs).

List items: fix marker not drawn with position:inside and no content

(Upstream) Ordered list markers: add a decimal point suffix

Will allow closing koreader/koreader#8668 .
Considered in #411, but not picked at the time because of other considerations, that should be gone with next commit.

List item markers: more pleasant rendering

  • Allow disc/circle/square markers to be drawn with a better specific font (FreeSans or FreeSerif) if available: as most fonts don't have all these glyphs, some of them could be picked from fallback fonts and they would have inconsistent look, vertical position, or spacing.
  • Allow decimal number markers to be drawn with a variant of the list item font, with the OpenType feature "tabular nums" enabled, so digits have a fixed width and vertically align above each others (only works with Harfbuzz kerning).
  • Keep the dot added by previous commit, and add a space after (regular or en-space depending on list type and position.)
  • Fix bad orderning of alpha/roman marker and the dot when in RTL direction.

Last 3 commits discussed in koreader/koreader#8668.
Descriptions and screenshots of the various issues with the previous implementation are discussed in upstream buggins/coolreader#217 and buggins/coolreader#226

renderListItemMarker(): cleanup, minor optimizations

Re-order arguments, use default values, to make the call clearer.
Also avoids using LFormattedText->Format() to get the height, as we have computed it in there, so allow it to be returned.


This change is Reviewable

Comment on lines +19 to +21
section[type=footnotes] {
-cr-hint: non-linear;
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should you then also do some others like div or p?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think some specs or guidlines say to use <section epub:type="footnotes"> for the container of <aside epub:type="footnote">

Among the 13 epubs I have with aside type=footnote, all those that have a type="footnotes" (plural) have it on SECTION. Only one has a container DIV type="footnote" (singular, missing the final s).
That's what I met in these 13 books:

   nonlinear, section type=footnotes, aside type=footnote
   nonlinear, section type=footnotes, aside type=footnote
                  div type=footnote (sans s), aside type=footnote
               bottom of content html, aside type=footnote
               bottom of content html, section type=footnotes, aside type=footnote
               bottom of content html, section type=footnotes, aside type=footnote
   nonlinear, section type=footnotes, aside type=footnote
               bottom of content html, section type=footnotes, aside type=footnote
   nonlinear, section type=footnotes, aside type=footnote
   nonlinear, section type=footnotes, aside type=footnote
               bottom of content html, aside type=footnote
   scrambled, aside type=footnote in single OEBPS/chapitre20.xhtml
   scrambled, aside type=footnote in single OEBPS/chapitre20.xhtml

So, feel safer to keep targeting these specific SECTION and ASIDE - people who would meet other combinations can always add it with styletweaks.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. type=footnote felt like an any (block) element kind of thing to me, at least when reading it like in these lines

@poire-z
Copy link
Contributor Author

poire-z commented Jan 20, 2022

Just noticed a few past and new issues with list items :/ that I need to fix before merging this.
Most bothering is that using bullets from FreeSans when the original font has smaller baseline/height can result in no longer steady line heights/interline space...

Before | After:
image

@poire-z
Copy link
Contributor Author

poire-z commented Jan 21, 2022

Cut and pasting analysis and screenshots from my gitter answer at https://gitter.im/koreader/koreader?at=61ea9fa89a3354540633a573, for reference:

Is anyone using the Charter font by any chance?
I think that it looks fantastic on my e-reader but compared to a bunch of other fonts that I've tried, it requires something like 145% line spacing to be comparable to other fonts at 100% or less
I'm not really understanding why lines overlap at 100% line height whereas this doesn't happen to other fonts
I'm using the files provided here: https://practicaltypography.com/charter.html
tried both .otf and .ttf with the same results

This Charter font seems to have really bad metrics - at least for FreeType, or FreeType is not able to pick what the designer has intended
It causes me new issues :( with what I thought I had made solid solving

image

size=21 orig font baseline=20 height=16 vs FreeSans baseline=18 height=23
size=14 orig font baseline=13 height=10 vs FreeSans baseline=12 height=15
size=39 orig font baseline=38 height=29 vs FreeSans baseline=35 height=43

With all sizes, we get a baseline greater than the height ! so glyphs are drawn below the line box height !

even our highlights are wrong (we highlight the line box):
image

Thought I would have to handle this extreme case for this bullet list item business, but given that it does not absolutely break things, and that the font rendering is bad elsewhere, I will just not bother :) and put it all on the font being really bad.

@poire-z
Copy link
Contributor Author

poire-z commented Jan 21, 2022

I now get proper and nice bullets with all my fonts, so hopefully, I'm done with this :)

poire-z and others added 8 commits January 21, 2022 15:04
(This fixes a floating point error crash when
reloading from cache a book with no content.)
Just like it is done for EPUBs since ages.
Via "-cr-hint: non-linear" / "non-linear-combining",
set in stylesheets.
Also consider <section epub:type="footnotes"> and
<aside epub:type="footnote"> as non-linear.
Do as web browsers, which align list item markers near the
followup content:
   9 some content
  10 some content
Also add private property "list-style-position: -cr-outside"
to be able to get the previous distant alignment:
  9  some content
  10 some content
which is preferable when LI are used for in-page footnotes
(like they are with Wikipedia EPUBs).
- Allow disc/circle/square markers to be drawn with a better
  specific font (FreeSans or FreeSerif) if available: as most
  fonts don't have all these glyphs, some of them could be
  picked from fallback fonts and they would have inconsistent
  look, vertical position, or spacing.
- Allow decimal number markers to be drawn with a variant
  of the list item font, with the OpenType feature "tabular
  nums" enabled, so digits have a fixed width and vertically
  align above each others (only works with Harfbuzz kerning).
- Keep the dot added by previous commit, and add a space after
  (regular or en-space depending on list type and position.)
- Fix bad orderning of alpha/roman marker and the dot when
  in RTL direction.
Re-order arguments, use default values, to make the
call clearer.
Also avoids using LFormattedText->Format() to get the
height, as we have computed it in there, so allow it
to be returned.
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.

None yet

3 participants