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.

Sign up for GitHub

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

Jump to bottom

Fix FB2 image sizing & centering, and bottom border on empty block elements #563

Merged
merged 3 commits into from Apr 7, 2024
Merged

Fix FB2 image sizing & centering, and bottom border on empty block elements #563

merged 3 commits into from Apr 7, 2024

Conversation

poire-z
Copy link
Contributor

@poire-z poire-z commented Apr 6, 2024 •
edited by Frenzie

EPUB: avoid crash when @font-face in <head><style>

Assertions in LVFontCache::_removeDocumentFonts() would fail (when DEBUG enabled).
Noticed and investigated at #536 (comment).

FB2: fix block images sizing and centering

Followup to 1544bcf, where we made an image self container's width adjust to the image width, so text-align:center no longer does what was expected. Switch to margin-left/right:auto to get them centered again. See koreader/koreader#11623 (comment).
Also, for standalone block images, set the strut and line-height to 0 (as we do for HTML) so they their block is adjusted to the page height when the image itself exceeds it, to avoid such images to be truncated and sliced on two pages. Noticed and investigated in #562.
Should allow closing koreader/koreader#11623.

lvrend: fix positionning of bottom border on empty block elements

Fix some possible issues with empty block elements having some bottom border (included in padding_bottom) when previous vertical margins are collapsing.
(Not 100% certain this is the right fix that won't cause side effects...)
Should allow closing koreader/koreader#11594.

This change is Reviewable

@poire-z
EPUB: avoid crash when '@font-face' in <head><style>
805d81f 
Assertions in LVFontCache::_removeDocumentFonts() would fail
(when DEBUG enabled).
@poire-z
FB2: fix block images sizing and centering
d469daa 
Followup to 1544bcf, where we made an image self
container's width adjust to the image width, so
text-align:center no longer does what was expected.
Switch to margin-left/right:auto to get them centered
again.
Also, for standalone block images, set the strut and
line-height to 0 (as we do for HTML) so they their
block is adjusted to the page height when the image
itself exceeds it, to avoid such images to be
truncated and sliced on two pages.
@Frenzie
Copy link
Member

Frenzie commented Apr 6, 2024 •
edited

There's a whole bunch of margin collapse test cases over here btw. "8.3.1 collapsing margins"

http://test.csswg.org/suites/css21_dev/20110323/xhtml1/chapter-8.xht
http://test.csswg.org/suites/css21_dev/20110323/html4/chapter-8.html

@Frenzie
Copy link
Member

Frenzie commented Apr 6, 2024

margin-collapse.zip

@Frenzie
Copy link
Member

Frenzie commented Apr 6, 2024

NOT passing in the current version:

  • 9, 10, 11 (about overflow)

  • 29

  • 33, 34, 35, 37, 38

  • 101 (it passes but is meaningless without 106)

  • 102

  • 103

  • 104

  • 105 (meaningless without 106)

  • possibly 106 (font required)

  • 107, 108, 109

  • 113

  • 114

  • 115

  • 116

  • 117

and, um, probably all following but I got bored. :-)

@poire-z
Copy link
Contributor Author

poire-z commented Apr 6, 2024

The ones that fail are related to overflow, position:absolute, floats inside empty div (that I know there are tricky interactions we don't handle well), and some with imbricated tables with empty cells (that may be our code to compute table cells widths from their content don't handle well, but they really look like torture tests :))
Pretty happy with the many not failing ones anyway :)

Frenzie
Frenzie approved these changes Apr 6, 2024
View reviewed changes
Copy link
Member

@Frenzie Frenzie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anyway, lgtm

PS It's positioning ^_^

@poire-z
lvrend: fix positioning of bottom border on empty block elements
f4370bc 
Fix some possible issues with empty block elements having
some bottom border (included in padding_bottom) when
previous vertical margins are collapsing.
(Not 100% certain this is the right fix that won't
cause side effects...)
@poire-z poire-z merged commit 1a9e79e into koreader:master Apr 7, 2024
1 check passed
@poire-z poire-z deleted the misc202404 branch April 7, 2024 11:31
This was referenced Apr 7, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Frenzie Frenzie Frenzie approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@poire-z @Frenzie