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

Add more granularity in line-height settings with CRe #4785

Merged
merged 6 commits into from Mar 14, 2019

Conversation

Projects
None yet
4 participants
@NiLuJe
Copy link
Member

commented Mar 13, 2019

Much like #4691 did for margins ;).

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

Might be tad quirkier for people who were using custom defaults (and I'm guessing that's a lot, because the three previous settings were of dubious utility on >167dpi devices).

(i.e., I had LARGE set to 112, which made it smaller than it's new XXL_MEDIUM neighbor. I basically just moved my custom value to the end of the scale, because I know I'm never ever going to be using such a large value anywhere ;)).

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

I'll be happy to just get 0.95 - to reduce it with some font with large interline.

But dunno, shouldn't we have 100 in the middle, and the same nb of items on each side? Or is tthat setting more useful for people who need more interline, so they have more granularity on that increase side?

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

In practice, stuff rarely actually ever uses "1.0" as a default line-height, so it's not a great middle ground, AFAIK.

And yeah, I find granularity more useful on the > 1 side of the scale, but that's mainly because I'm using a severely mangled Bookerly with no extra vertical space ;).

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

I'm definitely not against adding 80 & 85 to push 100 closer to the middle, though, because I figure Fonts with extra vertical spacing are far more common than those without (.f.g, Noto Serif, which stops being ridiculous @ 0.85). ;).

NiLuJe added some commits Mar 13, 2019

Even more granularity
Effectively pushes 1.0 closer to the middle of the bar.
@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

I figure the toast shown when switching to another value should be enough for people to grok the "scale" ;).

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

In practice, stuff rarely actually ever uses "1.0" as a default line-height, so it's not a great middle ground, AFAIK.

Well, dunno what stuff you're talking about :) but crengine would use 1.0 as the default, and so do I, just to get the default font interline as decided by the designer, which is just right for my prefered font.

I figure Fonts with extra vertical spacing are far more common than those without

I agree.
(What's the L for in _L_SMALL or _L_MEDIUM ?)

I figure the toast shown when switching to another value should be enough for people to grok the "scale" ;).

I think that now that you switched to progressbutton, you have more chances to see what I described at #4782 (comment) (2nd point). I'll have to figure that out...

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

In practice, stuff rarely actually ever uses "1.0" as a default line-height, so it's not a great middle ground, AFAIK.

Well, dunno what stuff you're talking about :) but crengine would use 1.0 as the default, and so do I, just to get the default font interline as decided by the designer, which is just right for my prefered font.

Yeah, that was a shoddy argument, and badly worded to boot ;).

(What's the L for in _L_SMALL or _L_MEDIUM ?)

"Large" or "Larger", because that's the best I could come up with without rewording everything and breaking persistent defaults ^^.
Which begs a valid question, since breaking persistent defaults might be a good thing here... ;).

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

So, to recap:

  • I'd like to keep the linear progression in increments of 5
  • I'd like to keep 130 as the max
  • Having 100 in the middle would be logical, but that implies adding two steps (75 & 70), and I'm afraid that'd lead to itty-bitty squares on 600x800 screens...
@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

And, yeah, there's quite possibly a double re-paint/re-layout involved with some of the stuff in the bottom menu, I'm with you there ;).

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

I'd like to keep the linear progression in increments of 5 [...] implies adding two steps (75 & 70),

Yep. So may be just change the linear increment in the decrease side, 3 or 4? Althought we wouldn't then get the classic 0.95 and 0.90... So, I dunno :)
(0.975 0.95 0.925 0.9 0.875 0.85 could be nice, but we can only pass integer so no 0.005)

@Frenzie

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

What does this 130 value correspond to exactly? A line height of 1.5 doesn't seem particularly excessive, so I'm guessing it's not 1.3?

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

It's a %, 100 here = line-height: 1 - 120 here = line-height: 1.2 or line-height: 120% (same thing in crengine).
120 looks really big already to me. 1.5 may not sound excessive in contexts like presentation, display - but for book reading, it looks excessive to me :) althouhg it may depend on the font.

@Frenzie

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

Optimal line-spacing is generally said to be between 120 % and 145-150 % of the point size.

The line-spacing setting of 90 % (current "small") actually seems to look more like 120 %.

But I'm guessing there must be some line-height property built into the font that makes it so that what's called "100 %" here is actually something like 130-140 %?

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

It's also quite possible crengine does not handle that totally correctly. I've seen some differences between browsers and crengine. Some stuff with freetype glyphs, baseline, height might be not correct, or the way it adresses that with freetype metrics...

In one case I remember, the publisher was using line-height: .99em, and I needed to change it to line-height: .8em to have crengine shows it as browsers do with .99em...

@Frenzie

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

Note that line-height in browsers is basically a minimum depending on the pixel grid. On our readers or phones (nor my UHD display) that's not too bad, but on a more old fashioned monitor the end result may be typographically deceptive unless you're looking at a larger font size.

Incidentally, you should "never" specify CSS line-height with a unit, or at least not that unit. em and % have inheritance issues. rem and px would work but that's more of a "why would you" situation.

@Frenzie Frenzie added this to the 2019.04 milestone Mar 13, 2019

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Mar 13, 2019

Yeah, it really depends on the font here. My two usual extremes are Noto Serif, which has massive vertical spacing, and any fonts tweaked for eReaders where we usually squish the vertical space to the minimum.

In this context, 0.80 is viable w/ Noto, and 1.20 w/ Bookerly, while the reverse make 'em look stupid ;).

TL;DR: Add 70 & 75 and see what happens?

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Mar 13, 2019

Add 70 & 75 and see what happens?

Fine with me. Let's have that and see with our fonts.

@Frenzie

This comment has been minimized.

Copy link
Member

commented Mar 13, 2019

@NiLuJe

My two usual extremes are Noto Serif, which has massive vertical spacing

That's the one I was specifically looking at when saying that 90 % looks like 120 % btw.

Even more granularity on the lower end
Putting the default, 100, smack in the middle

@NiLuJe NiLuJe force-pushed the NiLuJe:more-line-heights branch from 54b1af9 to cd4533d Mar 14, 2019

@ptrm

This comment has been minimized.

Copy link

commented Mar 14, 2019

Yay, you're quicker than my thoughts. Should have checked the PRs here before pushing to my fork today ;)

@NiLuJe NiLuJe merged commit 645d41e into koreader:master Mar 14, 2019

1 check passed

ci/circleci Your tests passed on CircleCI!
Details
@poire-z

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

Noticing that 75% = 80%, and 130% = 125%, so some slots seem to have no effect. Is it just me or my fonts?

I remember that crengine may use these % to scale only a small integer, 16=100%, 32=200%, so, dunno if the maths confirm that then some of our % has logically no effect :)

https://github.com/koreader/crengine/blob/535c0afac77af638c35974989663bcd376668cae/crengine/src/lvrend.cpp#L1903-L1952
https://github.com/koreader/crengine/blob/535c0afac77af638c35974989663bcd376668cae/crengine/src/lvtextfm.cpp#L1167-L1173

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2019

@poire-z : Where are those percentages from?

(Not seeing anything out of the ordinary here, FWIW).

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2019

Oh, okay, yeah, I think I get what you mean. Switching between 75 & 80 doesn't necessarily yield an actual change of visible line-height.

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

Same for 105 = 100.
I guess with the current crengine implementation, we should use different values (and kill 3 of them).

Our numbers will be rounded down to the value on the left, so we may want to use the value in the middle (or ceil'ed to a prettier number) from frontend (our current values on the right that get mapped to these):

>>> for i in range(8,25): print 100*i/16.0, int(math.ceil(100*i/16.0))          ...
50.0      50    (<  55)
56.25     57    (<  60)
62.5      63    (<  65)
68.75     69    (<  70)
75.0      75    (<  75 & 80)
81.25     82    (<  85)
87.5      88    (<  90)
93.75     94    (<  95)
100.0    100    (< 100 & 105)
106.25   107    (< 110)
112.5    113    (< 115)
118.75   119    (< 120)
125.0    125    (< 125 & 130)
131.25   132    (< 135)
137.5    138    (< 140)
143.75   144
150.0    150

(And that way, we would decrease a bit the number of slots in the progress bar, it looks a bit odd that it is different from the margins ones, with smaller squares.)

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Mar 15, 2019

Or we can tweak crengine to have more granularity.

Just had a try with changes like:

-   lUInt8          interval, /* interline space, *16 (16=single, 32=double) */
+   lUInt16         interval, /* interline space, *256 (256=single, 512=double) */
    lInt16          valign_dy,/* drift y from baseline */
[...]
-                //   *16 (16=normal, 32=double)
-                // so both % and em should be related to the value '16'
+                //   *256 (256=normal, 512=double)
+                // so both % and em should be related to the value '256'
                 // line_height can be a number without unit, and it behaves as "em"
                 css_length_t line_height = css_length_t(
                     style->line_height.type == css_val_unspecified ? css_val_em : style->line_height.type,
                     style->line_height.value);
-                line_h = lengthToPx(line_height, 16, 16);
+                line_h = lengthToPx(line_height, 256, 256);
                 // line_height should never be css_val_inherited as spreadParent
                 // had updated it with its parent value, which could be the root
                 // element value, which is a value in % (90, 100 or 120), so we
                 // always got a valid style->line_height, and there is no need
                 // to keep the provided line_h like the original computation does
[...]
-            else { // original crengine computation
+            else { // original crengine computation (used when gRenderDPI off)
                 css_length_t len = style->line_height;
                 switch( len.type )
                 {
                 case css_val_percent:
-                    line_h = len.value * 16 / 100 >> 8;
+                    // line_h = len.value * 16 / 100 >> 8;
+                    // line_h = len.value * 256 / 100 >> 8; // 20190315: 100% = 256 instead of 16
+                    line_h = len.value / 100; // simplified
                     break;
                 case css_val_px:
-                    line_h = len.value * 16 / enode->getFont()->getHeight() >> 8;
+                    // line_h = len.value * 16 / enode->getFont()->getHeight() >> 8;
+                    // line_h = len.value * 256 / enode->getFont()->getHeight() >> 8; // 20190315
+                    line_h = len.value / enode->getFont()->getHeight(); // simplified
                     break;
[...]
             int fh = enode->getFont()->getHeight();
             int fb = enode->getFont()->getBaseline();
-            int f_line_h = (fh * line_h) >> 4; // font height + interline space
+            int f_line_h = (fh * line_h) >> 8; // font height + interline space
             int f_half_leading = (f_line_h - fh) /2;
             txform->setStrut(f_line_h, fb + f_half_leading);
[...]
             LVFont * font = (LVFont*)srcline->t.font;
             int fh = font->getHeight();
-            int fhWithInterval = (fh * interval) >> 4; // font height + interline space
+            int fhWithInterval = (fh * interval) >> 8; // font height + interline space
             frmline->height = (lUInt16) fhWithInterval;
             m_y += frmline->height;

The value we provide here (85, 105) gets mapped to:

    s->line_height.type = css_val_percent;
    s->line_height.value = def_interline_space << 8;

No visible change in rendering, except that our 85%, 105% and 130% works.
Should I go on with that?

If yes, no worry with stuff like int f_line_h = (fh * line_h) >> 8; possibly integer overflowing ? These kind of computations are done in 32 bits right? Even if line_h is LUint16 and fh (int or lUInt16) , so the multiplication possibly overflowing lUint16 before the >>8, it should be fine ? Or do we need some cast'int (lUInt32) or (int) somewhere?

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Mar 15, 2019

@poire-z : If that doesn't break or visibly change anything else, I'm all for more granularity! Thanks for looking into it :).

Yeah, C (generally) implicitly casts to the widest member's data type when doing arithmetic. As fh appears to be an int every time, computations are done w/ 32bit, and as the result is also always stored in an int, no risk of truncation ;).

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Mar 16, 2019

Incidentally, you should "never" specify CSS line-height with a unit, or at least not that unit. em and % have inheritance issues

Looking at the code, it seems crengine manages % and em for line-height just like if they were unitless numbers.
When we set 120 here, it's set internally as being 120%, but it is handled more like if it were line-height: 1.2.

From what I understand from https://developer.mozilla.org/en-US/docs/Web/CSS/line-height and https://stackoverflow.com/questions/15586245/when-a-relative-line-height-is-inherited-it-is-not-relative-to-the-elements-fo:

  • relative values like % and em should be computed to their ... computed value on the element they were met
  • these computed absolute values - and values in css absolute units - are inherited as is to their inheriting children (if 120% on a DIV gets computed to 33px, deeper inner P get its effective line-heights as 33px)
  • when line-height is provided as a unitless number, it's inherited as-is: 1.2 on a DIV, the deeper inner P will get is as 1.2, and the computed effective line-height is computed at this P level, with the P font size, so practically 120% of this P font height.

Does my understanding sound right?

So, crengine treats % and em as in the 3rd bullet above. Which somehow is practical as we didn't notice any issue, and it prevents the bad case of "avoid using % and em because of inheritance issues".
Should this be fixed so we are specs conformant ? :)

Other length'ed properties that gets inherited in crengine is font-size, letter-spacing, text-indent - and 'm not even sure it's all allright regarding them (if parent has font-size: 120% and child font-size: 70%, it looks like child will get it's style font size: 84% (1.2*0.7). Or I'm confused...
All others inheritance just propagate the parent value to the child (mostly enum/named values).
Most of that is done at https://github.com/koreader/crengine/blob/535c0afac77af638c35974989663bcd376668cae/crengine/src/lvrend.cpp#L4267-L4446

@Frenzie

This comment has been minimized.

Copy link
Member

commented Mar 16, 2019

Does my understanding sound right?

Probably. It can be illustrated in quite a simple manner:

<!DOCTYPE html>
<style>
div {
 font-size:150%;
}
.line-height {
 line-height:100%;
}
.line-height-120 {
 line-height:120%;
}
</style>
<div>
font-size 150% of parent
  <div>
  nested diff, 150*150
    <div>
     nested diff, 150*150*150 (and so on)
    </div>
  </div>
</div>

<hr>

<div class=line-height>
line-height 100% of parent
  <div>
  nested div, still 100% of parent
    <div>
     nested diff, still 100% of parent
    </div>
  </div>
</div>

<hr>

<div class=line-height-120>
line-height 120% of parent
  <div>
  nested div, 120% of parent
    <div>
     nested diff, 120% of parent
    </div>
  </div>
</div>

Screenshot_2019-03-16_09-49-01

Should this be fixed so we are specs conformant ? :)

If it can be done quickly, of course I'd rather it conforms. Whether it's worth the effort is your call. :-)

Edit: Haha, my fingers are apparently more used to typing diff (mainly in git diff) than div. I could quickly & silently fix it, but I won't. ;-)

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Mar 17, 2019

@poire-z : On a slightly related note, are font sizes handled in a similar fashion? I wouldn't mind being able to feed CRe half-point values instead of full integers... ;).

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2019

No, nothinkg like that 16 or 256 base scaling.

We do pass integers (before that, we scaleBySize() them in frontend - so you may want to kill that to get some granularity back).
So, we pass an integer to crengine (all is in crengine/src/lvdocview.cpp).
(I added a scaleFontSizeForDPI() which is a no-op as the toggle is off - you can ignore it.)
Then, it goes thru:
https://github.com/koreader/crengine/blob/e8d21305dc60563b7d04bcb68ce69edac862480d/crengine/src/lvdocview.cpp#L3311-L3332
to find the nearest value in an array we pass from frontend/cre.cpp. It is then passed to FontMan, which I guess asks that value to freetype.

So, it's constrained to be one of the values in:
https://github.com/koreader/koreader-base/blob/6582561d074cf332ba1e9df9a459fe09b0ee4b26/cre.cpp#L1006-L1022
which is what you might want to extend or kill.
(Not sure why there is that.)

@NiLuJe

This comment has been minimized.

Copy link
Member Author

commented Mar 17, 2019

@poire-z : Thanks for the pointers, I'll try to eventually take a look at that, because it's been mildly bothersome on 300dpi screens ;).

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Mar 17, 2019

FWIW, I've been using in default.persistent.lua:
DCREREADER_CONFIG_FONT_SIZES = {12, 13, 14, 15, 16, 17, 18, 19, 20, 21, 22, 23}
and patched frontend/ui/data/creoptions.lua with (so I can see these numbers instead of Aa Aa...)

         options = {
             {
                 name = "font_size",
-                item_text = Aa * #DCREREADER_CONFIG_FONT_SIZES,
+                item_text = DCREREADER_CONFIG_FONT_SIZES, -- best if we see the size number
                 item_align_center = 1.0,
                 spacing = 15,
                 item_font_size = DCREREADER_CONFIG_FONT_SIZES,
@poire-z

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

Optimal line-spacing is generally said to be between 120 % and 145-150 % of the point size.
The line-spacing setting of 90 % (current "small") actually seems to look more like 120 %.
But I'm guessing there must be some line-height property built into the font that makes it so that what's called "100 %" here is actually something like 130-140 %?

There is the notion of line-gap set by the designer of a font if I trust what I wrote in a comment in lvrend.cpp:

    // We need current and parent nodes font metrics for most computations.
    // A few misc notes:
    //   - Freetype includes any "line gap" from the font metrics
    //   in the ascender (the part above the baseline).
    //   - getBaseline() gives the distance from the top to the baseline (so,
    //   ascender + line gap)
    //   - the descender font value is added to baseline to make height
    //   - getHeight() is usually larger than getSize(), and
    //   getSize() is often nearer to getBaseline().
    //   See: https://iamvdo.me/en/blog/css-font-metrics-line-height-and-vertical-align
    //   Some examples with various fonts at various sizes:
    //     size=9  height=11 baseline=8
    //     size=11 height=15 baseline=11
    //     size=13 height=16 baseline=13
    //     size=19 height=23 baseline=18
    //     size=23 height=31 baseline=24
    //     size=26 height=31 baseline=25
    //   - Freetype has no function to give us font subscript, superscript, x-height
    //   and related values, so we have to approximate them from height and baseline.

After hours of trying to find the cause of some alignment differences between Firefox and KOReader, with some HTML tags and not others (I thought), I think I just discovered/realized that there is something wrong with how crengine did/does compute unitless/%/em line-height: it is scaling it against font->getHeight() - and it seems it should use font->getSize() - which saddens me :(.

Can you confirm this experimentation provides some proof of that ?:

<html>
<head>
<meta http-equiv="content-type" content="text/html; charset=UTF-8" />
<style>
body { font-family: "Times New Roman"; }
div, span {padding: 0; margin: 0}
</style>
</head>
<body>

<br/><br/>

<div style="line-height: 60px"> <!-- will be changing just this -->
<div style="font-size: 60px; border: 1px solid blue">L <small>ipsum</small> <span style="font-size: 60px; background-color: yellow; border: 1px solid red">L</span></div>
<br/>
<div style="border: 1px solid blue">a <span style="font-size: 60px; background-color: yellow; border: 1px solid red">L</span> ipsum dolor sit amet</div>
<br/>
<div style="font-size: 60px; border: 1px solid blue">L <small>ipsum</small> <span style="font-size: 60px; background-color: yellow; border: 1px solid red">L</span> ipsum dolor sit amet ipsum dolor sit amet <span style="font-size: 60px; background-color: yellow; border: 1px solid red">Lp</span> ipsum dolor sit amet ipsum dolor sit amet</div>
<br/>
The end.
</body>
</html>

(Firefox on the left / KOreader on the right - don't mind the different height area of what's highlighted in yellow - that will stay different - but they help noticing the differences/overlap).

With line-height: 60px, same as font-size: 60px: -- pretty similar
image

With line-height: 1: -- quite different
image

With line-height: 1 still for Firefox on the left - but with line-height: 0.87 for KOReader on the right:
image

With line-height: 1.15 for Firefox on the left - with line-height: 1 for KOReader on the right:
image

The ratio font->getHeight() / font->getSize() for the Times New Roman font used here at 60px happens to be 1.15 - and 0.87 inverted.

It saddens me because, if we were to map our interline space buttons 100% => 1 real em, we will get a smaller interline space - so, ok then we'll choose 110 or 115% to get what we're used to.
But we'll never be able to get the real line-gap (=right ratio) the font designer has put into its font (that make out getHeight()/getSize() as returned by Freetype).
(Unless we have crengine scale the interline space we provide by the current default font getHeight()/getSize() - could that little lie be acceptable?)

@Frenzie

This comment has been minimized.

Copy link
Member

commented Mar 19, 2019

I think it's a bit more subtle than that. crengine doubtless has the principle right, with perhaps a few details slightly off. Also keep in mind that CSS 2.2 exists to clarify behavior where implementations may differ, so it's conceivable that crengine is CSS 2 compliant and Firefox is too.

https://drafts.csswg.org/css2/visudet.html#propdef-line-height

On a block container element whose content is composed of inline-level elements, 'line-height' specifies the minimal height of line boxes within the element. The minimum height consists of a minimum height above the baseline and a minimum depth below it, exactly as if each line box starts with a zero-width inline box with the element's font and line height properties. We call that imaginary box a "strut." (The name is inspired by TeX.).

The height and depth of the font above and below the baseline are assumed to be metrics that are contained in the font. (For more details, see CSS level 3.)

https://www.w3.org/TR/CSS22/visudet.html#line-height

The line box height is the distance between the uppermost box top and the lowermost box bottom. (This includes the strut, as explained under 'line-height' below.)

Full details here:

https://www.w3.org/TR/CSS22/visudet.html#leading

Still for each glyph, determine the leading L to add, where L = 'line-height' - AD. Half the leading is added above A and the other half below D, giving the glyph and its leading a total height above the baseline of A' = A + L/2 and a total depth of D' = D + L/2.

Note. It is recommended that implementations that use OpenType or TrueType fonts use the metrics "sTypoAscender" and "sTypoDescender" from the font's OS/2 table for A and D (after scaling to the current element's font size). In the absence of these metrics, the "Ascent" and "Descent" metrics from the HHEA table should be used.

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Mar 19, 2019

Yes, we got all that right (I added that strut thingy some months ago). Glad you consider it fine too :)

The issue is not about the line-height implementation: we have it similar as firefox with absolute length line-height, like the 60px first screenshot shows.
It's just about the sizing of line-height: 1 or line-height: 1em or line-height: 100%.
The specs (and the mozilla link) indeed says "font size" - but it feels it's nearly gratuitous as written and could have been "font height" :)
"Font height" feels to me like the right thing to use, at least for line-height: 1, to have the natural font interline space. That's why I never questionned until now the fact that, only for line height, crengine was using "font height" - where it uses "font size" for anythine else em (borders, padding, margin, font sizes themselves...)
But it now looks like Firefox really uses "font size" .

I reckon it's only a few pixels differences, and we never noticed anything.
But I've been working on adding support for floats, and for stuff like dropcraps, on which the publisher has finely tuned font size, line height and margins, the difference in vertical aligment shows up:

image (example from #2844).
crengine using too much makes the dropcap ... dropped a a few pixels too down.

So, it would feel right to fix that. But it will have an impact on the user and his habits with the default interline space that possibly most people were happy with it at 100% - they would have to increase it to get back what they are used to.

@Frenzie

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

possibly most people were happy with it at 100% - they would have to increase it to get back what they are used to.

It felt alright to me, but I was also under the mistaken impression that it was actually either ~1.3 or normal. I had noticed the occasional oddly large-looking line-heights but I think I didn't double check to see if it was just a design choice or something off about crengine.

It's just about the sizing of line-height: 1 or line-height: 1em or line-height: 100%.
The specs (and the mozilla link) indeed says "font size" - but it feels it's nearly gratuitous as written and could have been "font height" :)

I think I understand now. There's the normal keyword, the default, which depends on the font-family and is typically around 1.2 to 1.3. It's only when a CSS author overrides it that it's based on the font-size.

<!DOCTYPE html>
<style>
p {
 font-family:"DejaVu Sans";
 font-size:100px;
}
.noto {
 font-family:"Noto Sans"
}
</style>
<p>Look mommy, I'm 100px and my line-height is calculated as 120px, which is <code>normal</code> for DejaVu Sans.</p>
<p class=noto>Look mommy, I'm 100px and my line-height is calculated as 136.5px, which is <code>normal</code> for Noto Sans.</p>

There does seem to be something to be said for a multiplication of normal, but not as part of what EPUB authors were aiming for.

Functionally speaking, I think the GUI setting should work like this:

* {
  -cr-line-height-adjust: 1.2;
}

Where in this example it's 1.2 * the per-spec line-height, regardless whether based on normal (which is seldom if ever the same as 1) or some custom line-height value.

Except since we get this value straight from the UI, there's likely no need for the performance impact of a global selector. That's merely to illustrate the principle of how it would function, and possibly as something of interest for user style tweaks.

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

Thanks, that looks indeed like a nice way to go at that and avoid any user visible change of what we set via the UI !
Needs some thinking on how to integrate normal and this new factor into the code.

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

Wondering if this def_interline_factor (that would replace def_interline_space in the code) should only be applied on "normal" - or apply to any specified line-height (whether in em or px) as you suggested.

  • On all and any: it would kill any publisher fine grained positionning - and you would need to set 100% in the UI to get it back as designed by the publisher - so losing your prefered factor for the default font. But it's practical for most text I guess.
  • On normal only, it would affect only regular text without anything specified - and not publisher specified ones. The user could additionally use the "Ignore publisher line height" * { line-height: inherit !important; } so that everything inherits normal and the factor from the UI applies.

Any further argument to decide for one or the other? :)

@Frenzie

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

Like I implied, only applying the interline factor on normal would be inconsistent since other values will be defined by the author in relation to normal (cf. your own argument about fine-grained positioning).

Ignore publisher line height would be a good one to add to the repertoire regardless.

(Your second suggestion could work more consistently by only applying the line-height adjustment factor in case of the complete absence of any line-height value other than normal, but I think that'd probably just be confusing as a user.)

@poire-z

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2019

Ignore publisher line height would be a good one to add to the repertoire regardless.

We have it. It's the first thing I (and others) go enabling when the interline is too big (possibly because of this font height/size bug) and our UI toggle buttons have no effect:

id = "lineheight_all_inherit";
title = _("Ignore publisher line heights"),
description = _("Disable line-height specified in embedded styles, and may allow KOReader's line spacing settings to work on books where they would not."),
css = [[* { line-height: inherit !important; }]],
},

It currently "unlock" the interline UI switch - by essentially removing any publisher set line-height, so all elements inherit from the root node interline, which is what the UI sets (in current crengine).

Your second suggestion could work more consistently by only applying the line-height adjustment factor in case of the complete absence of any line-height value other than normal, but I think that'd probably just be confusing as a user

That's the current behaviour: it works when nothing is specified, and you need to use the above tweak to "unlock" and have it applied on elements where some line-height were specified.

So, if the UI is now to only set a factor that would be applied on all specified - or inherited from the root node which will have line-height: normal -, there would no more be any "lock" (and the need to unlock): it would always work.

Like I implied, only applying the interline factor on normal would be inconsistent since other values will be defined by the author in relation to normal (cf. your own argument about fine-grained positioning).

Same, that's the current behaviour - and the need to "unlock" and lose the fine grained (but currently wrong) positionning.
But I don't know if authors do anything "in relation to normal". If they do fine grained tuning, they usually set line-height: 1.2 on all their elements or paragraphs to have what they consider the good default for the font - it's with such publishers' books that we have that need to "unlock" stuff.

I'm quite used to this locked / unlock :) It allows me to know that's some fine-tuned book or not. We currently screw the fine-tuning because of the bug.
Doing the factor on all/any won't allow us to "see" the book is fine-tuning - and you'll only get the correct fine-tuning when you set the UI to 100%.
But this locked / unlock is indeed confusing to new users.

@Frenzie

This comment has been minimized.

Copy link
Member

commented Mar 20, 2019

But I don't know if authors do anything "in relation to normal".

In my blog styles I open with font:1em/1.5 "DejaVu Sans" because I find normal too small. There are also reasons of minimizing browser differences involved, but for that it'd suffice to set line-height to normal or inherit on the same elements, rather than declaring line-height as a specific value.

I don't think it's that unlikely that an author would decide to only adjust the line-height on specific elements to achieve an effect.

Doing the factor on all/any won't allow us to "see" the book is fine-tuning - and you'll only get the correct fine-tuning when you set the UI to 100%.

There are at least two alternative approaches that might be slightly more user-friendly.

  1. Allow easy tuning of the default line-height (i.e., with a result like html { line-height: 1.5 }) in addition to the line-height scaling factor. Of course this is possible through user styles regardless, but I mean with two separate adjustment bars. Default line-height and line-height scaling. Or just the same thing with a set of default user styles, since this is probably a more advanced setting.
  2. By default, don't apply the default line-height scaling factor on these "fine-tuned" documents but allow for later applying it manually anyway. (Possibly combined with some kind of "force line height scaling on all documents" setting.)
@poire-z

This comment has been minimized.

Copy link
Contributor

commented Mar 24, 2019

OK, so in koreader/crengine#273, I decided to go the easy way and have our interline progress buttons apply their factor to all computed line-heights, whether they come from the default "normal" or publishers specified line-height in % or em or absolure size. Somehow, it feels quite allright, and it is quite easier to not have to go into Style tweaks and enable "Ignore publisher line heights".
(I gave some thoughts to an additional toggle, or the use of -cr-line-height-adjust - but there were no default or purpose that jumped out at me as being obvious or easy to explain :)

@Frenzie

This comment has been minimized.

Copy link
Member

commented Mar 24, 2019

Yup, it just seems unexpected to me any other way. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.