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

Various fixes related to text-indent, CSS order. Adds -cr-hint: strut-confined #326

Merged
merged 16 commits into from
Jan 29, 2020

Conversation

poire-z
Copy link
Contributor

@poire-z poire-z commented Jan 22, 2020

See individual commit messages for details.
Some additional notes:

        {
            id = "lineheight_all_normal_strut_confined";
            title = _("Enforce steady line heights"),
            description = _("Prevent inline content like sub- and superscript from changing their paragraph line height."),
            priority = -5, -- so other -cr-hint can override (this one has effect only on inline content)
            css = [[* { -cr-hint: strut-confined; }]],
        },

Before style tweak applied:
image
After:
image
(An inline image standalone on a line is not confined, as noticed at the bottom - the two other are reduced because they have a comma or a period after them.)

The main benefit (for me) is getting rid of badly styled footnote links (not even wrapped in <SUP>, that even the Ignore publisher line-height and the radical Ignore publisher font-size couldn't get rid of because of excessive vertical-align (one would have needed to look at the code and create a custom style tweak to get rid of that).
Before style tweak applied:
image
After:
image

  • lvtextfm: avoid spurious spaces when hyphenating fixes this:
    image
    when
    image
    so we get the correct:
    image

  • lvtextfm: dont adjust space after initial quotation mark/dash Fixes this too wide space:
    image

  • lvtextfm: ensure page-break:avoid on inline-block and images is not required by specs, but will allow us to finally get nice gallery in Wikipedia EPUBs by having all the items inline-block, including a 100% caption, ensuring the captions is stuck on the same page as the first items:
    image
    instead of the messy stuff we got with floats, screenshot in Enhanced block rendering (floats, collapsing margins...) #299.

  • Fix internal/footnote links not working on some books fixed internal link not working when internal file names are percent-encoded (<a href="Author%20Title.html#fnt1">[1]</a>) - I sometimes had footnotes links not working, and always assumed it was the book that was badly made :|


This change is Reviewable

Follow-up to a76d14e: update enhanced drawing flag
from 2 to 0x11, as 0, 1, 2 and 3 might be used for
categorising crengine internal bookmarks.
This might avoid breaking upstream/other client code,
which could then just add 'flags &= 0x10' somewhere to
benefit from enhanced drawing.
So it gets a fixed id and we get more cache stability.
By storing an order sequence number in the _specificity field,
which will ensure selectors with the same specificity are checked
and applied in the order we parsed them.
Embedded floats overflowing their containing paragraph, and
not directly children of that final node (so, wrapped by more
inlines nodes), would not be visited by elementFromPoint()
and text or image inside them would not be found.
Made the 'direction' argument to elementFromPoint() and
createXPointer() more explicite, and allow finding the first
or last word on a line in logical-order.
This allows not missing text and links in pages where the
first or last line is RTL or bidi.
Previously, images were incorrectly skipped in the avoidWrap
checks. So, a sequence of word-space-image-period was handled
as word-space-period, and a wrap avoid before a period forced
the whole thing to be on a same line.
Will allow a style tweak to confine text to its paragraph
line height (aka the strut):
- this will prevent sub- and superscript-like inline elements from
  increasing line height and pushing down the baseline, and will
  reposition them if needed to limit their overflow.
- this will resize inline images so they are not taller than the
  line height.
Makes thing easier to read and understand.
Also use proper lInt16 for it, as it can be negative.
Positive text-indent should apply only on the first line of
a final block (paragraph), and should not apply after a <BR>.
(Same for negative indent, that should apply on all but first
line, including the first line after a <BR>.)
Properly handle negative indents (that induce a left shift of
the first line in LTR, possible overflowing its container),
and hanging indents (that induce a right shift of the non-first
lines in LTR, without any overflow), both in RTL a LTR cases.
(Note that the text-indent "hanging" it not handled by all
current browsers, but we do, as it is used in fb2.css.)
From upstream in-progress "merge with KOReader" PR125.
@poire-z
Copy link
Contributor Author

poire-z commented Jan 22, 2020

(To be bumped after coming up release.)

Some word segment might start with a non-text source, without
a ->t.font field, which could cause a segfault when accessed.
Very rare and hard to reproduce, so did not try to make that
skipping more part of the whole flow (we should probably also
not provide them to HyphMan::hyphenate() to not miss some
hyphenation opportunities).

Also renamed inner 'start' and 'end' vars to avoid shadowing
upper ones with the same names, to avoid confusion.
@poire-z
Copy link
Contributor Author

poire-z commented Jan 24, 2020

Added a commit to fix some possible segfault I had a hard time reproducing - witnessed only 2 crashes, last one enough to notice it was indeed something flagged as OBJECT at wstart - with a Wikipedia EPUB while tweaking styles to try using display: inline-block, and reducing the window size to around 300x300...:

set view dimen {
    ["h"] = 328,
    ["w"] = 314
}
wstartflag0 wstart85 wend90
wstartflag0 wstart121 wend134
wstartflag8000 wstart0 wend5

0x00007fffe7dd21e2 in LVFormatter::processParagraph (this=this@entry=0x7fffffffb4f0, start=start@entry=0, end=end@entry=2,
    isLastPara=isLastPara@entry=true)
    at /space/kobo/koreader/base/thirdparty/kpvcrlib/crengine/crengine/src/lvtextfm.cpp:3213
3213                     int _hyphen_width = ((LVFont*)m_srcs[start]->t.font)->getHyphenWidth();

Related to changes from #320.

Since f3b5958 and possibly looking for hyphenation on more
than one word word, we may set hyph flags at multiple positions,
and have non last words larger than they should, causing the
appearance of a space. So, only add the hyphen width on
the last word of a line.
Noticed when hyphenating "extre<SPAN id=z/>mement", hyphenable
as "extre-me-ment" and chosing the 2nd one: it would be rendered
as "extre me-" (with a spurious space) on first line and "ment"
on next line.
If a paragraph starts with a quotation mark or dash, and is
followed by a space (classic or nbsp): don't have this space
width widened or shortened in the process of text justification.
This generalizes the existing code that was doing this only
for 4 kind of dashes: we do it for more opening marks, dashes,
bullets, and other ascii chars that could start a line.
@poire-z
Copy link
Contributor Author

poire-z commented Jan 27, 2020

Added more commits, top post updated with explanation and screenshots.

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.

CSS: Hanging indents are shifted to the right
1 participant