koreader / koreader

An ebook reader application supporting PDF, DjVu, EPUB, FB2 and many more formats, running on Cervantes, Kindle, Kobo, PocketBook and Android devices
http://koreader.rocks/
GNU Affero General Public License v3.0
16.8k stars 1.26k forks source link

Question: fb2 in-page footnotes justified #6344

Closed hius07 closed 4 years ago

hius07 commented 4 years ago

https://github.com/poire-z, can you please help me to have fb2 in-page footnotes justified, same as popup footnote. I've played with User style tweaks, cr3.ini, footnotewidget.lua with no success. Reader_2020-07-03_062817 Reader_2020-07-03_062826

poire-z commented 4 years ago

A user style tweak with this should work:

body[name="notes"] section,
body[name="comments"] section {
   text-align: justify !important;
}

Does that work ? If not, can you try to see by inspecting the HTML where a text-align: left comes from.

I added recently text-align: justify; to the "book style tweak" sample: https://github.com/koreader/koreader/blob/8a61e70fadb74189b1196ac9a7d07e49f6c0111a/frontend/apps/reader/modules/readerstyletweak.lua#L591-L602 Because I sometimes (but rarely) got them non-justified, and I figured it would not hurt there (a user can just erase that sample line). Dunno if we should switch all in-page footnotes tweaks to text-align: justify.

poire-z commented 4 years ago

Even if it works, can you try to see where a text-align: left comes from. Obviously, it's not you who set it via another style tweak.

hius07 commented 4 years ago

The user style tweak doesn't work. I've checked cr3.ini, fb2.css, cr3.css, the book itself - no left-aligned instructions. The text of the footnote is left-aligned both in the in-page footnote and in the end of the book.

hius07 commented 4 years ago

I've applied the sample of Book-specific tweak - doesn't work.

hius07 commented 4 years ago

I cannot trace the source of the text-align:left instruction, the html code shows just [body name="notes"], [section] and [p].

poire-z commented 4 years ago

Try this, forcing it on all descendants:

body[name="notes"] section *,
body[name="comments"] section * {
   text-align: justify !important;
}

It's only on this specific FB2 book (can you share it?), or does it happen with all FB2 books?

hius07 commented 4 years ago

Doesn't work. All fb2 books.

poire-z commented 4 years ago

So, please share one (that you can share), so we can see if it's you only - and if I can reproduce I'll investigate.

Btw, on your first post, it seems fine on the 2nd screenshot (?) - or did I not undestand the issue?

hius07 commented 4 years ago

The second screenshot shows popup footnote which is Okay. Лондон - Мартин Иден.fb2.zip

poire-z commented 4 years ago

The second screenshot shows popup footnote

Oh, right :)

OK, I can reproduce it - and it does not happen with a koreader 2020.04.1-12 I had around. So, probably a bug caused by some of my recent changes... :( I'll look into it - you can stop fighting it with style tweaks.

poire-z commented 4 years ago

Caused by https://github.com/koreader/crengine/commit/28ae22c8e311d811be2b1923d292619cc51faf8f and probably related to the specific display: run-in only used by FB2 footnotes

hius07 commented 4 years ago

I've changed in fb2.css run-in to auto and got justified footnote, the text starting the next line of the footnote number. User style tweak works.

poire-z commented 4 years ago

Should be fixed with https://github.com/koreader/crengine/pull/353. (You'll find there an alternative way of handling FB2 footnotes with floats, to replace the similar stuff in fb2.css with - if you need a quick temporary fix :)

hius07 commented 4 years ago

Thank you very much, I'll wait for the build to test.

hius07 commented 4 years ago

Dunno if we should switch all in-page footnotes tweaks to text-align: justify.

Can in-page footnotes follow the alignment settings of the main text?

poire-z commented 4 years ago

For FB2, they did, and they will with the fix. They are a <p> , so are affected by the Style tweaks > Text > Alignment > Left/justify most text. And FB2 do not have class= I think, and aren't really styled by publishers (is this right ?)

With EPUBs, it's more wild, as they can be in a DIV, and some publisher can explicitely set them to text-align: left. But some people may like them like that. I just wondered how many people :) It's quite rare - so I guess we can let them as-is, not overriding publishers and using our default of justify as a fallback.

hius07 commented 4 years ago

Nice, in fb2 books by default we will have both main text and in-page footnotes justified (as per fb2.css setting for <p>). Yes, fb2 books are not really styled by publishers, just marked up.

hius07 commented 4 years ago

In-page footnotes are now perfectly justified, thanks a lot!

I'm not sure if it is worth your attention, anyway: the space between the number and the text in in-page footnotes is doubled comparing to popup footnotes.

5

poire-z commented 4 years ago

That is (and has been for a long time - it's original upstream code) explicitely done by adding 2 UNICODE_NO_BREAK_SPACE between the number and the text: https://github.com/koreader/crengine/blob/59483446bc777b211cb23181bc8f6c60b94a1c1b/crengine/src/lvrend.cpp#L3175-L3184 and some checks to avoid having these spaces expanded or compressed by text justification: https://github.com/koreader/crengine/blob/59483446bc777b211cb23181bc8f6c60b94a1c1b/crengine/src/lvtextfm.cpp#L3052-L3062

So, I guess someone prefered them that way, unless it's just some technical trick, not sure. About that last bit, I added this as a comment in some current work on cleaning some stuff:

// For FB2 footnotes, renderFinalBlock() has added between the
// footnote number (set display: run-in) and the following text
// these 2 UNICODE_NO_BREAK_SPACE (flagged LTEXT_FLAG_PREFORMATTED).
// This condition and the following one might handle keeping their
// widths constant, or might handle leading real space in the
// footnote number or footnote text...
// But as the are LTEXT_FLAG_PREFORMATTED, they might not yet
// collapse.
// This might need some rethinking.

Btw, I dropped the idea of using float: left I mentionned above mainly because with it, I couldn't ensure a space between the number and text.

poire-z commented 4 years ago

the space between the number and the text in in-page footnotes is doubled comparing to popup footnotes

And which do you prefer ?

hius07 commented 4 years ago

One space as in popup.

hius07 commented 4 years ago

Good day,

This book shows no space after number in popup footnotes. Besides, popup footnotes don't render the [emphasis] tag. Голсуорси Джон - В петле.fb2.zip

Reader_2020-07-11_071545

This book has [stylesheet] section in the beginning (which is rare for fb2 books from Flibusta).

poire-z commented 4 years ago

Unlike most of the others FB2, this one have the footnotes part stuck to each other in the source:

<section id="n_6"><title><p>[6]</p></title><p><emphasis>¿¿¿¿¿¿¿¿¿ ¿¿¿¿¿¿</emphasis> (1836¿1914) ¿ ¿¿¿¿¿¿¿¿¿¿ ¿¿¿¿¿¿¿¿¿¿¿¿ ¿¿¿¿¿¿¿, ¿¿¿¿¿¿¿¿¿ ¿¿¿¿¿¿ ¿¿¿¿ ¿ ¿¿¿¿ ¿¿¿¿¿-¿¿¿¿¿¿¿ ¿¿¿¿¿; ¿ 1895¿1903 . ¿¿¿¿¿¿¿ ¿¿¿¿ ¿¿¿¿¿¿¿¿ ¿¿¿¿¿¿¿.</p>
</section>

(pardon my latin1 not showing cyrillic :) Most others have their source a bit formatted:

  <section id="n_10">
   <title>
    <p>10</p>
   </title>
   <p>¿¿¿¿¿¿¿¿ ¿ ¿¿. ¿¿¿¿¿¿. ¿¿¿¿¿.</p>
  </section>

So, when giving all that as-is to MuPDF, it gets some space between nodes and can show, once collapsed, at least a space between the number and the text - that it doesn't get in the first case.

I guess I could force adding one space after a node with display:run-in when crengine gives the HTML to our frontend code that will give it to MuPDF.

For the italic missing, FB2 has some non-standard HTML tags (like <emphasis> that MuPDF does not know about, so it displays them unformatted. So, we'd need to add formatting for these non-standard tags in: https://github.com/koreader/koreader/blob/f4dad2fae8548b44a59a4ae6de9361673717ae77/frontend/ui/widget/footnotewidget.lua#L57-L111

One would need to find out the ones that need to be formatted differently. https://github.com/koreader/crengine/blob/master/cr3gui/data/fb2.css https://github.com/koreader/crengine/blob/59483446bc777b211cb23181bc8f6c60b94a1c1b/crengine/include/fb2def.h#L137-L231 https://github.com/koreader/crengine/blob/master/fb2props/FictionBook2.2.xsd

Quick glance and I see emphasis and strikethrough might need it. Mind having a more serious look ?

hius07 commented 4 years ago

So, we'd need to add formatting for these non-standard tags in:

Will it be hardcoded that, say, [emphasis] is italic? Actually, it can be bold if one prefers to set it in fb2.css.

poire-z commented 4 years ago

It would be hardcoded (in footnotewidget.lua) in what we give to MuPDF for rendering popup footnotes. So, it should probably be the same as what we decided to be the default formatting for these tags in fb2.css.

hius07 commented 4 years ago

what we decided to be the default formatting for these tags in fb2.css

Can you please show me the thread discussing default fb2.css. May I say, it is not ideal.

poire-z commented 4 years ago

I don't think we ever discussed (or cared about :) fb2.css on the KOReader side. cf https://github.com/koreader/crengine/pull/194#issuecomment-392834289 . It's more a russian thing that we inherited from CoolReader.

The best you can get from us is the history of changes to this file: https://github.com/koreader/crengine/commits/master/cr3gui/data/fb2.css and upstream has not changed it since we picked it: https://github.com/buggins/coolreader/commits/master/cr3gui/data/fb2.css There are some issues opened there about fb2.css, like https://github.com/buggins/coolreader/issues/137. But if anything has ever been discussed, it's probably on the Coolreader russian forum https://4pda.ru/forum/index.php?showtopic=191747

Anyway, as you're probably the person who cares the most about fb2.css, I think we can accept any change you'd like to make :) Or open an issue here for discussion if you want feedback, and invite upstream russian FB2/FB3 lovers & devs.

poire-z commented 4 years ago

And for context, I guessed over the years that the history of all these CSS files might have been:

I mainly tried to make epub.css fixed and cleaner, and make obsolete all the others - except fb2.css that really needs something specific for its own non-HTML-standard element names like <emphasis> and its specific <section> stuff and footnotes.

hius07 commented 4 years ago

Thank you very much, I'll examine all of that.

hius07 commented 4 years ago

Seems to be one more issue with fb2. fb2.css has instruction for [code] to be left-aligned. KOReader tries to justify it. The second screenshot is from Coolreader for Win. 1.fb2.zip

1 2 I understand that this test book isn't usual (maybe incorrect) with that extra CR-LF's, anyway can you please look at it.

poire-z commented 4 years ago

Dunno how it should render, but telling you why it is that way:

In fb2.css, we have (stripped out some other element names from that):

code, pre { font-family: "Courier New", "Courier", monospace; }
code, pre {
   white-space: pre;
   text-align: left;
   font-family: "Droid Sans Mono", "Liberation Mono", "DeJaVu Sans Mono", monospace;
   }
code {
   display: inline;
}

In your sample fb2 html:

<body>
      <section>
          <p><code>one</code></p>
          <p><code>two two    <!-- there's a \n here -->
</code>
</p>
      </section>
</body>

CODE being white-space: pre, the \n is not ignored. CODE being display: inline, it's just a part (the single part here) of the inline flow, and text-align does not apply to individual parts of the inline flow. So, it's the text-align of the real inline flow container, the P, that will apply: justify. As two two is followed by a \n, two two is not the last line and so, the requested justification is done (while it is not done on the last line of a paragraph).

CoolReader does not have the code { display: inline; } , so it must handle it as block. code { display: inline; } was added on our side, may be wrongly while fixing other xxx.css, with https://github.com/koreader/crengine/commit/97e2ea7e

Note that on our side (and probably picked by some dev code in upstream coolreader), we defaulted code to be inline in fb2def.h (because it is inline in HTML/EPUB). So, if you want it block, you might have to explicitely set it code { display: block; } in fb2.css.

hius07 commented 4 years ago

Thank you. got it.

hius07 commented 4 years ago

Oh, sorry, I've changed code { display: inline; } to block in fb2.css, but still have it justified.

hius07 commented 4 years ago

Well, I've added the same to User style tweaks and it is left-aligned finally. fb2.css remains default (inline).

poire-z commented 4 years ago

There's an other instance of it, below the one you changed (so it overrides your change), in the messy fb2.css, at the end of:

b,strong,i,em,dfn,var,q,u,underline,del,s,strike,spacing,small,big,sub,sup,acronym,tt,sa mp,kbd,code {
   display: inline;
}
poire-z commented 4 years ago

Well, I've added the same to User style tweaks and it is left-aligned finally.

I hope you're not giving up on cleaning up fb2.css :) Now that we only use it with FB2 and FB3 (as we have a proper epub.css for other formats), you could really clean it up to only the element specified in the FB2 & FB3 specs.

hius07 commented 4 years ago

There's an other instance of it, below the one you changed

It's a trap :) Thanks!

hius07 commented 4 years ago

I now refer to the <v> tag which is formatted in fb2.css as v { text-align: left; text-align-last: right; text-indent: 1em hanging } The screenshots are self explained. 1.fb2.zip

Reader_2020-07-13_064716

This is from vlasovsoft CoolReader 20200713064916

The first and only short v-line should not be treated as last.

poire-z commented 4 years ago

The first and only short v-line should not be treated as last.

May be it shouldn't for you styling FB2, but according to the specs and Firefox, text-align-last applies to a single line (which is the first, but also the last). I "fixed" this recently in https://github.com/koreader/crengine/commit/5b28e2bd38a94e5b67aaf6c569be1e843072aedd - not yet picked by CoolReader.

I reckon it's strange and not what I would expect. But it's what Firefox & Chromium do for this:

<html>
<body>
<p style="text-align: left; text-align-last: right">abcdefghij klmnopqrstuvwxyz</p>
<p style="text-align: left; text-align-last: right">abcdefghij klmnopqrstuvwxyz abcdefghij klmnopqrstuvwxyz abcdefghij klmnopqrstuvwxyz abcdefghij klmnopqrstuvwxyz abcdefghij klmnopqrstuvwxyz abcdefghij klmnopqrstuvwxyz abcdefghij klmnopqrstuvwxyz abcdefghij klmnopqrstuvwxyz abcdefghij klmnopqrstuvwxyz abcdefghij klmnopqrstuvwxyz abcdefghij klmnopqrstuvwxyz abcdefghij klmnopqrstuvwxyz abcdefghij klmnopqrstuvwxyz</p>
</body>
</html>

image

So, I dunno how you could get what you want with CSS, no solution comes to my mind right now :( @Frenzie : any idea? If really needed, we could add a private property value: text-align-last: -cr-right-if-not-single ...

hius07 commented 4 years ago

The <v> tag is for a single line in poems, so now I have all poems with short lines right-aligned, which is not nice.

Frenzie commented 4 years ago

Since you're dealing in elements a v:last-child {blahblah} should do what you want?

poire-z commented 4 years ago

Nope, what we have here is a single element: some long text in a single <v> (or <p>) that will be rendered as multiple lines. :last-child won't catch on text/line boxes.

Frenzie commented 4 years ago

Yes, last or only line is per spec afaict.

Here's a thought I had:

<!DOCTYPE html>
<html>
<head>
<title>boo</title>
<style>
p {text-align-last:right}
p::first-line {text-align-last:left}
</style>
</head>
<body>
<p>
badger badger badger mushroom mushroom
badger badger badger mushroom mushroom
badger badger badger mushroom mushroom
badger badger badger mushroom mushroom
badger badger badger mushroom mushroom
badger badger badger mushroom mushroom
badger badger badger mushroom mushroom
badger badger badger mushroom mushroom
badger badger badger mushroom mushroom
</p>
<p>
badger badger badger mushroom mushroom
</p>
</body>
</html>

Works in Chromium 83 (Vivaldi 3.1), not in Firefox 78.

Screenshot_20200713_115744

poire-z commented 4 years ago

That's a clever idea ! :) Unfortunatly, we don't support ::first-line - and it's harder to implement than the other pseudo elements/classes - wouldn't be that hard for your sample case, but for p:first-line { font-weight: bold } we wouldn't be able to use anymore the single full paragraph measuring we have (we would need to measure with bold to know where to cut, and re-measure the rest with the non-bold font).

Anyway, private named value - or go against the specs and get the previous behaviour (wouldn't mind that too much, because, to me, it felt more natural and expected :) and I have no idea in which context one would want text-align-last to behave as defined in the specs... and if anyone is really using it :)

hius07 commented 4 years ago

v::first-line {text-align-last:left} doesn't work nor in fb2.fss neither in User style tweaks.

Oh, didn't see the comment above.

Frenzie commented 4 years ago

@hius07 Implicit in my thought above was that if you're implementing something new, it might make more sense to implement another part of the spec. ;-)

@poire-z Going against the spec is a potential option, but a bit dangerous. Perhaps something like text-align-last: -cr-last-but-not-only?

Frenzie commented 4 years ago

@poire-z Never mind, that's exactly what you already suggested earlier in https://github.com/koreader/koreader/issues/6344#issuecomment-657387318. ;-)

Frenzie commented 4 years ago

But I wrote it wrong. What I was thinking was clearly more along the lines of -cr-test-align-last-but-not-only: right. Either way, same principle.

hius07 commented 4 years ago

Can <v> be a specific case, not equivalent to <p>?

poire-z commented 4 years ago

What I was thinking was clearly more along the lines of -cr-text-align-last-but-not-only: right.

Would waste a CSS property slot :) Cheaper to go with a named value for the existing one.

Can <v> be a specific case

I'd rather not hardcode stuff, things should be driven by CSS. So, if we have one and you can use it in fb2.css, other people can use it for fixing other stuff in a user style tweaks.

We'll find some solution, let these <v> sit strangely in the meantime :)