css: consolidate @font-face parsing - #713
Conversation
|
Will review it. |
|
What is it with this |
| // Bumped: LVEmbeddedFontDef gained _weight (replacing _bold), | ||
| // changing the serialise layout. Stale cache entries are discarded | ||
| // and re-rendered once. | ||
| static const char * EMBEDDED_FONT_DEF_MAGIC = "FNTD2"; |
There was a problem hiding this comment.
No need for this I think (we never bumped it before), we will bump as we usually do:
crengine/crengine/src/lvtinydom.cpp
Lines 94 to 96 in 0fd3bc4
| // Drop the whole map so any such stale resolution is redone against | ||
| // the now-updated font registry -- see FONTFACE_PARSER_REFACTOR.md, | ||
| // "Bug found via runtime verification: stale font-resolution cache". | ||
| _fontMap.clear(); |
There was a problem hiding this comment.
Is this really ok to do ? We have created styles in the previous part of the DOM, associated a font. if we clear this map, won't these past associations be lost, or have old index resolved to not the same font as before the clear()?
(Just asking, no longer really clear to me how this works.)
| if ( name == css_at_font_face && parseFontFace ) { | ||
| str++; // skip opening '{' | ||
| parse_font_face_rule( str, doc, codeBase ); // consumes up to and including the closing '}' | ||
| skip_spaces(str); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
I understand (took me some time, so maybe worth a comment) when !parseFontFace, the content will be properly skipped below because has_nested_declaration is set to true for @font-face), right ? (Just as it was previously, but I wondered ohw it bahave when !parseFontFace).
|
The parsing looks sound and is readable. I'm a bit concerned about the "between fresh parse and cache reopen" stuff (haven't fully followed the explanations in the .md), which I think is (just?) about what you wrote in the top post:
Would this situation be noticed by some hash change ? - in which case a re-render will happen and things may end well (but I would like to see a Related concerns (or probably I'm just reformulating the previous concern :)): Dunno how much complicated/ugly it would be to register such fonts (or aliases), additionnally to per-document (our documentId), per this-document fragment number ? (and how much memory wasted to register the same font-file under the same name for 500 fragments).
But in this case, when the first fragment was parsed it didn't find the font, so it uses the default font, which sounds fine: that default font is associated to the style (or maybe not? it's just its family name, so the style will get the same hash as a node from a later fragment and will be reused ?) - and all is at it should be, no problem, even when we find that font-family name when parsing a later DocFragment, it's too late, it won't affect it, and it should not, no ? (or maybe not? we'll the use the default font that was associated to this style shared with the node from the first fragment, and we won't use the one from that font-face now defined?).
Yes, we can/will remove that code. I insisted on getting these |
|
I think the most correct way to deal with the "between fresh parse and cache reopen" issue would be to scope the This would be a change from the old implementation where If we scope the declaration to the fragment, then fragment A would never see a Based on previous discussions with Claude it looks like that change would be feasible. The cost shouldn't be high as similar mechanisms are already in place to scope normal CSS rules, including caching to avoid duplicating rules between fragments. However it looked like it would mean touching the code in quite a few places to pass the fragment ID to everywhere it's needed so I didn't proceed with it at the time. I'll have a go at implementing that scoping change - probably later today. |
|
Yes, feels like the most proper solution. Let's see how kludgy it would be. |
3 participants
Comments
@poire-z - I'm interested in your thoughts on this change and whether you think it is warranted.
This follows up on the
@font-faceparsing that we discussed briefly during the work on the font manager. I decided to see what it would look like if@font-faceis moved into the main CSS parser and this is the outcome.The changes are pretty contained and I find easier to follow as this avoids special cases like reparsing on embedded
@font-face:I've spent quite a bit of time looking for any areas where parsing the CSS in a single phase might cause problems - captured in the the fontface test epub and the test plan. I know of a single edge case that occurs in malformed epubs where a
@font-faceis used in an early DocFragment but only declared in a later one (test case 4 in the epub). In that situation the font isn't applied on the first pass through the epub but is if the document is closed and then reopened - admittedly odd behaviour but should never occur in practice (it only happens with malformed epubs) and only affects the font - all other rendering is correct.Rationale
@font-facewas previously parsed by a separate, one-off scanner that only looked at the EPUB's OPF-listed spine content, before the main CSS stylesheet parser ever ran. That split was the root cause of several independent bugs: styletweak-declared fonts were invisible to it entirely, numericfont-weightvalues weren't part of its parsing, and its batch pre/post-scan design needed aforceReinitStyles()workaround to paper over registration-ordering gaps. Parsing@font-faceas part of normal CSS parsing — the same path every other rule already goes through — fixes that whole bug class at once instead of patching each symptom separately.Summary
@font-faceparsing out of the standalone EPUB pre-scan (EmbeddedFontStyleParserinepubfmt.cpp) and into the normal CSS stylesheet parser (parse_font_face_rule()inlvstsheet.cpp), so@font-facerules are picked up by every CSS source that goes through stylesheet parsing: external stylesheets,<head><style>blocks, and KOReader styletweaks.LVEmbeddedFontDef's_boldboolean with a_weightinteger, so thefont-weightdescriptor can carry any parsed numeric value, not just bold/normal.Bugs fixed
font-weighton@font-face(e.g.font-weight: 900) was silently dropped and the face registered at 400, so requesting that weight synthesised it from the wrong base face instead of using the actually-declared one.@font-facedeclared in a KOReader styletweak was silently ignored, since the old parser only scanned the EPUB's own OPF-listed content, not tweak CSS merged in afterward. Styletweaks now register fonts the same way as any other stylesheet source, from the first page.Other notes
_bold→_weight); cache magic bumped so stale cache entries are discarded and re-rendered once rather than misread.EmbeddedFontStyleParserand the EPUB pre/post-scan +forceReinitStyles()cycle it drove are removed as dead code, no longer needed now that registration happens inline during stylesheet parsing.tests/FONTFACE_TEST_PLAN.md,tests/make_fontface_test_epub.py(generatestests/fontface-test.epub), andtests/font_face_demo.css— manual/visual regression coverage for registration ordering, the numeric-weight fix, and the styletweak fix.Known follow-up (not fixed here)
@font-facerule was registered, if two DocFragments end up with byte-identical computed styles. Worth its own investigation.DocFragmentpath, tracked separately.Test plan
tests/FONTFACE_TEST_PLAN.mdfor the manual/visual verification procedure.This change is