Improve vertical alignment and related adjustments #2762
Conversation
|
General logic and rendering of the test file look really promising, nice work! That said, it seems all lines are shifted compared to the old rendering, depending on the font and line height. For the standard fonts, the position seems to be to high, touching the top of the line in some instances; for custom fonts, at least DejaVu, the position looks to low now. I think the old logic was in part trying to work around inaccuracies or limitations with the baseline calculations. Needs more investigation. Rendering the test file on PHP 8.1.2, I get a For the implementation: Maybe split out the logic for establishing the common baseline and final line height so that it would be done while adding frames to the line ( Just some quick notes for now, hope to find time for a more in-depth look on the weekend. |
Ah, yes. I wasn't taking into account the line height when determining the baseline. I look into it and update the logic to take that into account.
Interesting, I didn't run into that on 8.1.0. I think maybe this error is something that XDebug raises when it's enabled? Anyway it's definitely due to the lack of strict comparison on objects. Will add the missing parameter. |
Both with XDebug and without for me. |
bsweeney
force-pushed
the
vertical-align
branch
from
d82a425
to
e59d881
Compare
|
In addition to the line height issue I noticed an alignment problem when an inline-block element has multiple lines. Based on some testing vertical alignment should be against, nominally, the baseline of the last line of the element. Current logic doesn't even consider that there may need to be a height adjustment on top of the baseline calculation. I updated the test file to include some additional use cases. |
bsweeney
force-pushed
the
vertical-align
branch
from
e59d881
to
850fb1b
Compare
|
OK I pushed an updated commit that, though there are some placements that aren't quite right, I think gets us close enough to the expected rendering. I do think some improvements can be made in when the calculations are performed. I don't know that we can fully prevent calling the move method multiple times since the actual vertical positioning is partially dependent on the alignment of all the frames within the line. But pre-calculating some things before we get to this point should be possible. One change I did make that should help with performance is to cache the "outside baseline height" since that shouldn't change at the point where it's calculated right now. |
|
I did find an issue but I'm not sure, yet, if it's because one of the calculations is bad or because a value isn't updated. The calculation to determine if an adjustment is needed to the line height because of frame alignment is producing an incorrect result for positioned frames. You can see the issue by rendering the following: Additionally, it looks like unnecessary calculations and/or operations are being performed because of float imprecision. I'll probably go ahead and use the new helpers from 271c651 to do a more precise comparison and avoid the extra work. |
It looks like the issue was due to a missing adjustment to the line y position when a block is re-positioned during reflow. Pushing a change for that as a separate commit since it's not directly related to the change in vertical alignment calculations. |
bsweeney
force-pushed
the
vertical-align
branch
2 times, most recently
from
661a0a1
to
89a689f
Compare
|
OK, I think I've done enough with this for the next release. |
|
Great work @bsweeney! I am noticing a difference compared to the current HTML: <html>
<head>
<style type="text/css">
body {
font-family: serif;
line-height: 100%;
}
.border {
border: 1pt solid black;
}
h1 {
font-size: 18pt;
margin: 0.67em 0;
}
</style>
</head>
<body>
<div class="border">
<h1>Header</h1>
</div>
</body>
</html>Which renders like this on And like this on And for comparison, this is how it renders in Chrome: With the images stacked like this it may not be particularly clear, but depending on the other styles and surrounding elements, the difference can become quite big. Setting the margin to
Chrome: |
bsweeney
force-pushed
the
vertical-align
branch
from
89a689f
to
af89205
Compare
It looks like the issue was that a change in the baseline due to a modified line height was only applied if it was positive. I think the updated branch does a better job in that situation now. |
|
That particular bug appears to be solved now, although I'm still seeing some irregulaties. For example, a block element with an image is getting extra whitespace that shouldn't be rendered: HTML: <html>
<head>
<style type="text/css">
body {
font-family: serif;
line-height: 100%;
}
.border {
border: 1pt solid black;
}
.stretcher img {
max-height: 3cm;
width: auto;
}
.stretcher {
display: block;
background-color:green;
font-size: 40pt;
}
</style>
</head>
<body>
<div class="border">
<div class="stretcher">
<img src="https://picsum.photos/768/348">
</div>
</div>
</body>
</html>
In general it looks like there is additional whitespace in several places that wasn't there before. I know it's hard to tell when it's an improvement (getting closer to spec) or when it's a bug, but it does seem like this is not rendered correctly either: HTML: <html>
<head>
<style type="text/css">
body {
font-family: 'dejavu sans',sans-serif;
line-height: 100%;
}
.border {
border: 1pt solid black;
}
table {
width: 100%;
}
td.blue {
background-color: #2F62AF;
text-transform: uppercase;
color: white;
font-weight: normal;
padding: 2mm;
vertical-align: top;
}
.label {
display: inline-block;
min-width: 60mm;
padding-right: 2mm;
background-color: red;
}
.value {
display: inline-block;
}
</style>
</head>
<body>
<table>
<tr>
<td class="blue">
<div class="wrapper"><span class="label">Shipping Method:</span><span class="value">Flat rate</span></div>
<div class="wrapper"><span class="label">Payment method:</span><span class="value">Credit Card</span></div>
</td>
</tr>
</table>
</body>
</html>
And for reference, Chrome (which seems to apply a tighter wrap/lower line-height than It looks like this appended whitespace issue happens in more situations, but sometimes it's difficult to isolate it from the entire context (which is much larger documents with loads of styles), so I'm focussing on the most obvious ones for now. |
bsweeney
force-pushed
the
vertical-align
branch
from
af89205
to
125e0f6
Compare
|
OK, this latest branch refresh addresses those issues. There is one regression in the branch for which I don't see an easy resolution. The minimum height of a frame enclosing a line is now the height of the line. This is incorrect when the line-height is less than 1. You can see it in the following: I'm trying to decide whether to release now without this branch (lots of good fixes already, including PHP 8.1 support), release now with this branch in its current state (noting the regression in the release notes), or delay the next release a little longer. Because the resolution of this regression is not obvious to me yet I'm leaning toward releasing now (with or without the branch), and following up with a 1.2.1 release as soon as the regression is addressed. |
✔️ Confirmed - thank you so much for your patience with these issues I reported! 🙏 Regarding the release: for me personally, the fixes from this branch would be more than welcome, also considering that a line height of less than 1 is not a very sensible/normal value (more like a hack if you ask me), so I would say the positive effect on 'normal' situations would outweigh the negative effect on more 'exotic' situations. |
|
After having tested some more with the HTML: <html>
<head>
<style type="text/css">
body {
font-family: 'dejavu sans',sans-serif;
}
.wrapper, h1 {
position: relative;
}
.wrapper {
border: 1pt solid black;
background-color: green;
}
h1 {
margin: 25pt 5mm;
background-color: red;
font-size: 40pt;
line-height: 100pt;
}
.wrapper-height-bar, .line-height-bar {
position: absolute;
top: 0;
left: 1mm;
width: 1mm;
background-color: blue;
}
.wrapper-height-bar {
height: 150pt;
}
.line-height-bar {
height: 100pt;
}
</style>
</head>
<body>
<div class="wrapper">
<div class="wrapper-height-bar"></div>
<h1>
<div class="line-height-bar"></div>
Header
</h1>
</div>
</body>
</html>Now I could be measuring the wrong thing here, but in Chrome it does line up perfectly. Result:
Chrome: Things I noticed here:
Not sure all of that is useful information, but I wanted to share it anyway, in case it provides you with some hints. |
|
I realize that this could potentially be a big change, because depending on the font, I have seen line-height factors of up to 1.5. I've always worked with relatively low line heights in dompdf (since version 0.6 or something), and this may simply have been that same calculation error all along. In any case, it seems to have gotten worse with this Here's a test illustrating the differences: <html>
<head>
<style type="text/css">
body {
font-family: 'dejavu sans',sans-serif;
font-size: 16pt;
line-height: 16pt;
}
.wrapper {
position: relative;
border: 1pt solid black;
}
.height-bar {
background-color: purple;
width: 1mm;
position: absolute;
top: 0;
left: -2mm;
}
.inline-block {
display: inline-block;
}
</style>
</head>
<body>
<div class="wrapper">
<div class="height-bar" style="height:96pt;"></div>
Lorem ipsum dolor sit amet, consectetur adipiscing elit,<br>
sed do eiusmod tempor incididunt ut labore et dolore magna<br>
aliqua. Ut enim ad minim veniam, quis nostrud exercitation<br>
ullamco laboris nisi ut aliquip ex ea commodo consequat.<br>
Duis aute irure dolor in reprehenderit in voluptate velit<br>
esse cillum dolore eu fugiat nulla pariatur.
</div>
<div class="wrapper">
<div class="height-bar" style="height:96pt;"></div>
<div>Lorem ipsum dolor sit amet, consectetur adipiscing elit,</div>
<div>sed do eiusmod tempor incididunt ut labore et dolore magna</div>
<div>aliqua. Ut enim ad minim veniam, quis nostrud exercitation</div>
<div>ullamco laboris nisi ut aliquip ex ea commodo consequat.</div>
<div>Duis aute irure dolor in reprehenderit in voluptate velit</div>
<div>esse cillum dolore eu fugiat nulla pariatur.</div>
</div>
</body>
</html>
|
|
Yeah, I think combining the height adjustment and line positioning adjustment is causing problems. So that we can go ahead and release the PHP compatibility updates I'm going to go ahead and push this change off for a 1.2.1 release. That'll give us a bit of time to re-assess the logic. |
|
@Spreeuw, any observations you can make are definitely useful. I appreciate the effort you've put in to validate this change. I have started working on separating the line positioning and height adjustments and it does show some improvements though it's far from ready to share. With all the changes we've made to improve the accuracy of calculations, one of the things that may be causing differences between Dompdf and browser rendering may be the fontHeightRatio. This is a legacy setting which is the "ratio applied to the fonts height to be more like browsers' line height." The default is 1.2, but if the calculations we're making are more accurate this may be best set to 1.0 by default. And removed entirely in a future release. |
bsweeney
force-pushed
the
vertical-align
branch
from
125e0f6
to
72cbbf2
Compare
bsweeney
force-pushed
the
vertical-align
branch
2 times, most recently
from
72d61b7
to
87028a2
Compare
bsweeney
force-pushed
the
vertical-align
branch
from
87028a2
to
9d95dcb
Compare
bsweeney
force-pushed
the
vertical-align
branch
from
9d95dcb
to
3d0692a
Compare
This change simplifies the vertical alignment logic by using a consistent frame of reference for djustments. 1. Determine the line baseline by calculating the "outer baseline" of the frames contained on the line. 2. For each frame on a line calculate the vertical alignment adjustment 3. Add the line height difference to the previous calculation 4. Re-position the frame per the calculated adjustment 4. Determine if the line height is adjusted after the frame is aligned 5. Re-position any following lines if the previous line height was adjusted The outer baseline height calculation is cached to aid in performance since this operation is performed inside-out after the frame has reflowed.
WIP ... need more descriptive commit and write-up here. Basically just re-wrote the logic so that vertical alignment is evaluated consistently. Addresses previously reported alignment issues, plus a few more.
Probably could use some optimization.
test file