-
Notifications
You must be signed in to change notification settings - Fork 45
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 support for display: inline-block/inline-table. And other fixes #325
Conversation
- Delay computation of alignment when vertical-align: top or bottom, as we need the full line to be laid out to know the final line height and baseline. - Also make the image vertical alignment code not assuming that baseline_to_bottom=0 (even if it's true for images), so it's ready to be re-used when implementing "display: inline-block" (where the baseline is not the bottom of the box). - Fix half_leading by setting half_leading_bottom, as otherwise 1px could be lost, causing possible alignment issues (but actually not anymore with the added delayed computation).
Wow, I waited many years for browsers other than Opera implement inline-block. Advanced stuff! ;-) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed 10 of 10 files at r2.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @poire-z)
crengine/include/cssdef.h, line 32 at r2 (raw file):
css_d_list_item_block, // display: list-item css_d_inline_block, css_d_inline_table, // (needs to be before css_d_table, as we use tests like if: (style->display > css_d_table))
Also makes some kind of logical sense. :-)
Added a commit, that will allow hyphenation in footnote popups, see koreader/koreader#5699 (comment). @NiLuJe : is |
@poire-z: Yep, allocated once EDIT: Oops, not technically on the stack, which is why access might be slower ;). |
I'm not familiar with the call stack, but does this actually need to be static? (Re-allocating/initializing on the stack for each call is likely to be faster than accessing static storage). |
Dunno if it "needs" to be. That array is passed further to Still think it needs to be on the stack? Bonus question: is it ok to if ( WNEFLAG(TEXT_SHOW_UNICODE_CODEPOINT) ) {
[...]
}
#define MIN_WORD_LEN_TO_HYPHENATE 4
#define MAX_WORD_SIZE 64
else if ( WNEFLAG(TEXT_HYPHENATE) && HyphMan::isEnabled() && txt.length() >= 4 ) { |
I edited my first answer, because it was actually inaccurate (and conflicting ^^). Where static storage gets allocated is devised by the compiler, and everything's set up at process startup time. So it's not technically on the stack, and, by the time it gets used, that usually ends up being "far" from the stack. That, plus other shenanigans explain the potential performance penalty vs. everything being on the stack. Unlike dynamic memory (i.e., malloc/new -> heap), I'd never worry about the cost of automatic memory allocation (-> stack). That said, in this case, the fact that the gist of it is done in another function which only gets a pointer adds another potential layer of fun ^^. I wouldn't overly worry about it, though. Yes, you can define stuff in function scope, and it's properly scoped by the compiler. In this case, I'd probably move it at the top of the if ladder instead of in-between rungs, but that's mostly stylistic (f.g., I hate that break between the closing brace and the else with a fiery passion ^^). |
Also, fun fact: unlike automatic storage, static storage is, by contract (i.e., C99 says so) zero-initialized. |
TL;DR: My gut feeling would be, yeah, keep it on the stack, the cost of zeroing 128 bytes should be negligible. TL;DR²: I'd only use Not that it's an issue here, but it's also process wide, meaning it's not thread-safe (which explains why TLS (thread local storage) exists). |
So, that would be just removing the Or I might just make the
even when they are just used by one late branch - and are a bit out of context that far away? |
IIRC, on Linux, we get 8MB of stack per-thread, and that's it (unless someone does wonky pthread shit ;p). (FWIW, that's virtual memory on Linux, it'll only consume as much physical memory as needed at the time). Blowing it up will crash (stack overflow). This is (partly) why So, yeah, anything relatively large ought to go on the heap ;). Here, we're talking about 128 bytes, so, yep, fairly insignificant ;). |
As for the define stuff, like I said, my comment was mostly stylistic. If you want it closer to the code, and you don't actually need it in the branch's conditional itself, I'd move it inside the branch, which should work just as well ;). |
Floats among floats should be cleared and shouldn't overflow the top float box (they were overflowing its margin bottom). Also, when they are cleared, the space cleared should not be removed from the bottom margin, so it's fully shown. Also fix uninitialized value (reported by clang-tidy).
Generic support for baseline of blocks. Will be needed to support "display: inline-block" (boxes whose baseline is the baseline of the last line box it contains) and "inline-table" (boxes whose baseline is the baseline of the first line box, or the bottom of the first table row). - FlowState::addContentLine() takes an additional parameter, that we set when adding a line (ignored when adding paddings and margins). - renderBlockElement() takes an optional *baseline so callers can get it additionally to the returned height. - Also allows storing a baseline in RenderRectAccessor.
- Enhanced Block Rendering: add a new toggable feature to allow rendering correctly an element with display: inline-block as an inline box (like an image, part of a line). - initNodeRendMethod() and lvrend.cpp setNodeStyle(): wrap elements with display: inline-block or inline-table in an internal inlineBox element (mostly just like we wrap an element with float: in a floatBox element). An inlineBox is erm_inline, and only needs to get the vertical-align property of its child. - renderBlockElementEnhanced(): deal with inlineBox specifically, but mostly the same as we handle a floatBox element. - getRenderedWidths(): recursive call to measure an inlineBox while measuring its containing final block. - renderFinalBlock(): add an inlineBox to the LFormattedText object the same way we add images. - lvtextfm: handle inlineBox mostly as we handle images (same vertical alignment code), and render them when measuring text to get their width, height, and baseline. Store their size and position in their RenderRectAccessor (once the words are aligned, and their position on the line known). Move some code from the float drawing section in an added getAbsMarksFromMarks() function, as we need it too with inlineBoxes when drawing selected text. - ldomDocument::createXPointer(pt): allow recursive call, used when pt is inside an inlineBox or a floatBox (former code for floatBox replaced by this cleaner way).
crengine native text selection (and internal bookmarks) highlighting was expecting single-column full-width paragraphs with nothing else on the line, and may be ugly when floats, table cells, or RTL/BiDi text are involved. This adds an alternative method for drawing them by using getSegmentRects(), and getting multiple small rects that will each compose the final highlighting. It can be toggled by using ldomXRange.setFlags(2) on the range to highlight (the original method will still be used when using ldomXRange.setFlags(1)). Also avoid division by zero (reported by clang-tidy).
Pass text nodes content to HyphMan, and add soft-hyphens where it says hyphenation is allowed (with the user or language current hyphenation settings). (This will allow popup footnotes to be hyphenated, as MuPDF does hyphenate on soft-hyphens.) Also re-order some flags in a more logical order.
(Rebased with minor changes in appropriate commits, to fix/avoid a few clang-tidy warnings.) |
Looks like I didn't test with images having themselves I guess I just forgot to add the new crengine/crengine/src/lvtinydom.cpp Lines 5372 to 5385 in fed8da7
|
I think that without a table/table-row a table-cell is basically just a (inline?) block. Not 100 % sure though. In your case the display: table may be supposed to generate an implicit table-row and/or table-cell; at least I seem to recall having seen something like that in the spec. (Sorry, don't quite feel like double checking atm but maybe it helps?) |
https://www.w3.org/TR/CSS2/tables.html#anonymous-boxes might be that spec and explain how it is expected to be done. |
We'd better look at https://www.w3.org/TR/CSS22/tables.html Even though it's probably 99 % the same, the updates are generally about removing ambiguities and adding more clarifications. (On the flipside, older specs may be easier to understand due to being less verbose.) Here it says that all missing table elements must effectively be generated if they're not present: |
Indeed :| But I had a go at it, and we'll be using even more Boxes! <style>
.itable { display: inline-table; }
.iblock { display: inline-block; }
.cell { display: table-cell; }
.wrapper > span { border: 1px solid #000; padding: 5px;}
</style>
<fieldset>
<legend>inline-table</legend>
<div class="itable wrapper">
<span class="cell">table-cell</span>
<span class="iblock">inline-block</span>
<span class="cell">table-cell</span>
</div>
</fieldset> But I get quite per-specs / per-Firefox results, and better results than before for inline-tables (which must really be wrapped by an anonymous table element), so that must be the right way to do it, Anyway, by looking at a dozen of fancy publishers EPUBs, I found 2 that had some stuff So, this fix, because it adds many new boxes which might not be as rare as floats, and may break past highlights, I'll need to bump gDOMVersionCurrent / DOM_VERSION_CURRENT so only new books will benefit from it. I'll need a few more days to work out all that, and it will need some testing. (Although I'm a bit concerned I introduced inline-block/inline-table in 2020.01 without a new DOM_VERSION - I'd rather have make them enabled only for a new DOMVersion, because some books are ridiculously abusing inline-blocks for large text, so I may have broken past highlights with that - I may make them depending on the coming up DOM_VERSION, so only books opened/highlighted with 2020.01 may have some issues - but not those highlighted with any 2019/2018... this compatibility stuff is quite a malediction...). |
Another nearly unrelated specs reading/english gramar question:
A fragmentainer is just the page/screen rectangle in my context. if ( row->height < context.getPageHeight()/2 && row->height < provided_available_width/2 ) {
content_line_flags = RN_SPLIT_BEFORE_AVOID | RN_SPLIT_AFTER_AVOID;
} Re-reading that spec text, it looks like it should be (I'll be doing that for single-columns tables only, where it's mostly just used as a wrapper - but small rows with only 2-3 lines of text better be kept together, just need to decide on the threshold, and that spec is giving one.) |
We've had some pretty big changes already, so I'm planning for in a week, maybe two. End of the month isn't really a thing I want to turn into a habit. In December I didn't feel it was quite ready around the 12th or so and I didn't really have time after that. :-)
Yeah, I'd say that ideally the DOM wouldn't change (except on HTML-parsing bug fixes) with CSS changes. Actually browsers implement something like that too. There's the DOM, which arguably is what we don't quite have. The DOM seldom changes once HTML parsing has reached a certain level of maturity (which isn't too hard with HTML5), and there's the shadow DOM. What we have may be a bit like the browser's shadow DOM. See https://developer.mozilla.org/en-US/docs/Glossary/Shadow_tree and https://developer.mozilla.org/en-US/docs/Web/Web_Components/Using_shadow_DOM although those don't look as informative as I'd have expected. I don't really know where you'd get better info, just that I've seen it.
I think it's saying that the whole row should be transferred to the next page if it's smaller than half the fragmentainer width & height. I'm not sure I understand why they're linking width to height though.
Doesn't that "and" translate into logical OR? |
Yeah, I'd read that as width || height too, FWIW. Spoiler : I'm unfamiliar
with their usual style ;).
…On Fri, Feb 7, 2020, 14:50 Frans de Jonge ***@***.***> wrote:
I've seen you already moved a few issues from 2020.02 to 2020.03 - any
feeling of when you'll be releasing 2020.02? Middle of month or end of
month?
We've had some pretty big changes already, so I'm planning for in a week,
maybe two. End of the month isn't really a thing I want to turn into a
habit. In December I didn't feel it was quite ready around the 12th or so
and I didn't really have time after that. :-)
this compatibility stuff is quite a malediction...
Yeah, I'd say that ideally the DOM wouldn't change (except on HTML-parsing
bug fixes) with CSS changes. Actually browsers implement something like
that too. There's the DOM, which arguably is what we don't quite have. The
DOM seldom changes once HTML parsing has reached a certain level of
maturity (which isn't too hard with HTML5), and there's the shadow DOM.
What we have may be a bit like the browser's shadow DOM.
See https://developer.mozilla.org/en-US/docs/Glossary/Shadow_tree and
https://developer.mozilla.org/en-US/docs/Web/Web_Components/Using_shadow_DOM
although those don't look as informative as I'd have expected. I don't
really know where you'd get better info, just that I've seen it.
A fragmentainer is just the page/screen rectangle in my context.
How do you understand "than both"?
I think it's saying that the whole row should be transferred to the next
page if it's smaller than half the fragmentainer width & height. I'm not
sure I understand why they're linking width to height though.
Re-reading that spec text, it looks like it should be &&, but I would more
logically have gone with || - any strong opinion when reading this specs
snippet?
Doesn't that "and" translate into logical OR?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#325?email_source=notifications&email_token=AAA3KZW5ABZSFLEMBZOYAMLRBVRIZA5CNFSM4KFI2RJ2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOELC63QQ#issuecomment-583396802>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA3KZRXNS43RLHTPVQFKI3RBVRIZANCNFSM4KFI2RJQ>
.
|
Put it another way: John, Jim & Jack are brothers. Means John is the smallest of the brothers, not that he's smaller than two of 'em standing on each other's shoulders ;). |
Explained that way, it looks so obvious... :), thanks! |
Yep, a DOM tailored for rendering, which is its main usage in crengine.
@pkb 's work was to make the code dealing with the first one always walk parents and siblings to look if there's any of our "shadow element autoBoxing/floatBox/inlineBox/tabularBox" so to skip/walk/count them, so the output/input can be made like if they weren't there. |
English grammar isn't formal logic, sorry. ;-) I don't think French is really any different than Dutch, German or English in that regard, is it? Basically we just have a sentence that a good editor would probably suggest to disambiguate. But anyway, on my end that was more of a rhetorical question. I think it means logical OR within the context of your piece of code. Btw, I think the specs have their own GH pages; there might already be an issue or a PR about them. Not suggesting you go wasting a lot of of time looking for that, but it could be worth trying for a few minutes. |
Yep, I met a few of the discussions there while looking for other stuff - but I didn't think of making an explicite search for this topic :| |
FWIW as a native English speaker, I find myself scratching my head over that particular phrasing. It's... not great. |
It's Speclish, not English. ;-) Speclish has more in common with Euro English than with native English, but it's less easily comprehensible than either one. |
Yup, managed to randomly hit that on my current book, too :D. (On full-page images in the front matter, which made for a few puzzling "how many times do I have to page next until I see some text?!" :D) |
In the meantime, a small styletweak will help :) |
I went the heavy handed way and switched the rendering mode to legacy, but, good to know, thanks ;). |
See individual commit messages for details.
Some additional notes:
lvtinydom.cpp: fix Use-after-free
Closes Memory error 'Use-after-free' #324.lvtextfm: fix/cleanup lastnonspace code bits
hopefully makes some dubious code rightlvtextfm: fix vertical-align: top & bottom
should implement them the right way, by delaying positionning until the full line is laid out. Closes Fix vertical-align: top/bottom/text-top/text-bottom #302 (comment).Previous work on vertical-align in Fix and extend support of vertical-align, and other rendering fixes #240 and line-height: reworked implemenation for better conformance #273.
Better selection highlighting by using getSegmentRects()
should have in-progress text selection show just as already-made highlights, which will improve text selection in RTL text.Should close Problem in select RTL text in PDF (EPUB has been fixed) koreader#5735
Add support for display: inline-block/inline-table
andrenderBlockElementEnhanced: compute baseline of block
Follow up to Enhanced block rendering (floats, collapsing margins...) #299
Enhanced block rendering (floats, collapsing margins...)
, many of its stuff is re-used to render inline-block boxes (box among text, laid out just like inline images) just like we render float content.I did not find much specs and docs about inline-block / inline-table, here are the ones I read:
https://www.w3.org/TR/CSS21/visudet.html#propdef-vertical-align only the last 2 paragraphs :)
https://blog.mozilla.org/webdev/2009/02/20/cross-browser-inline-block/
http://www.gtalbot.org/BrowserBugsSection/Safari3Bugs/baseline-inline-table-vertical-align.html
https://stackoverflow.com/questions/19352072/what-is-the-difference-between-inline-block-and-inline-table
https://www.brunildo.org/test/inline-block.html
https://www.brunildo.org/test/inline-block2.html
Some screenshots:
KOReader showing https://www.brunildo.org/test/inline-block.html:
Mix of floats and inline-block:
Inline-block in a float in an inline-block in a float:
Snippet from https://stackoverflow.com/questions/19352072/what-is-the-difference-between-inline-block-and-inline-table/56305302#56305302, Firefox on the left, KOReader on the right:
This change is