:valign=>:center or :bottom in text_box slightly inaccurate

[leovitch]

When you use :valign=>:bottom in Text::Box, the resulting text isn't quite at the bottom, and there's a similar problem with :valign=>:center. Of course, you're always at the mercy of the font metrics, but after a quick read through the code it sure looks like a bug in Text:Box.

Text::Box ends up calculating the height as the number of (wrapped) lines times the line height. That's the correct amount to advance down the page, but it's actually slightly bigger than the actual extent of the text because for most fonts ascender+descender is measurably smaller than the line height, and so on the final line of text nLines*line_height is a bit too big. Obviously this is most noticeable with one-line text boxes.

The pastie http://pastie.org/1294734 is an example, just drop it in prawn/examples/text and run it to see that this effect is not microscopic; it can easily be 2-3 points away from the expected position. It also demonstrates a way to fix it by providing a subclass of Box which overrides process_vertical_alignment... though hopefully this fix can make its way into Prawn in some form!

[Bluejade]

practicingruby@dfc7bd1

[derek-watson]

This issue should not be closed, it is still a problem in 1.0.0.rc2, the fix suggested by @Bluejade works correctly and should be merged.

[practicingruby]

@derek-watson I'm not sure why this issue was closed in the first place, but we can definitely re-open if the issue is still present.

Because 1.0.0.rc2 is a fairly old release, I'd want to be sure we can reproduce this either in the latest release (0.14.0), or even better, on master. Please confirm that, and I'll re-open.

If you want to move things along even further, you can try preparing a pull request with the fix and a test. A commit from 3 years ago might merge cleanly, but it's just as likely that it'll need to be changed somewhat if the underlying code has changed.

Right now there are many open issues in Prawn, and I'm currently the only active maintainer. The more you can do to help me prep this fix for release, the sooner it will get taken care of.

Thanks!

[practicingruby]

@derek-watson Seems you're already working on a fix! Didn't see that until after I commented. Looking forward to seeing a pull request, then 😄

[practicingruby]

Updating this ticket with a fresh example and screenshot of current behavior on master:

require_relative "lib/prawn"

Prawn::Document.generate('vertical_align.pdf') do
  text_box "The rain in spain falls mainly in the plains.", 
    :at => [100,450], :width => 100, :height => 100, :size => 16

  stroke_rectangle [100, 450], 100, 100

  text_box "The rain in spain falls mainly in the plains.", 
    :at => [250,450], :width => 100, :height => 100, 
    :valign => :center, :size => 16

  stroke_rectangle [250, 450], 100, 100

  text_box "The rain in spain falls mainly in the plains.", 
    :at => [400,450], :width => 100, :height => 100, 
    :valign => :bottom, :size => 16

  stroke_rectangle [400, 450], 100, 100
end

Produces:

I think we can agree that the top align looks correct. It's the centering and bottom align that has some complications to it.

For center valign: it looks like we're currently making the gap between the top and the ascender equal to the gap between the bottom and the descender. Looking at how some other software does it, it looks like what we should be doing is keeping the gap between ascender and the top of the box equal to that of the baseline and the bottom of the box.

I propose that we go ahead and change the behavior to compute the bottom gap based on the baseline

For bottom valign:, it looks like we're including the line gap. Looking for examples elsewhere, some things I found do this, while others do not. Prawn's text box actually has an option for choosing what behavior you want for this (:final_gap), but it does not appear to operate on alignment, only the final cursor position.

I propose we keep behavior as-is when :final_gap is set to true (the default), but remove the gap when :final_gap is set to false and vertical align is set to bottom.

Will these changes make everyone happy? If not, let me know where the problems are.

/cc @bradediger, who hopefully knows more about this stuff than I do.

[practicingruby]

It occurs to me that for the sake of completeness, we may want to also add an :initial_gap option which would insert a bit of line gap in the top-aligned example. Unsure if it should be off or on by default.

[bradediger]

/cc @bradediger, who hopefully knows more about this stuff than I do.

Unfortunately not. @Bluejade is the text box expert, and I have only interacted with that code tangentially due to my work on tables. Sorry to pass the buck but I have no direct knowledge here.

@sandal: Your assessment of the problem and proposed solutions seem spot-on to me. The lower alignment example seems to be a defect; the center alignment is potentially debatable in that a different example with heavier descenders on the last line might make the box look more balanced. I don't know of any literature on the topic, but it does make intuitive sense that the eye matches the ascender of the first line with the baseline of the last line, not its descender.

Count me as a 👍 on this general approach.

[practicingruby]

Anyone else with experience on this topic is welcome to chime in. Otherwise, I will plan to either work on a patch that does what is mentioned above myself, or accept one from anyone who wants to put in the work!

[practicingruby]

@Bluejade your thoughts are also welcome of course... it's just been tough to get in touch with you 😦

[jessedoyle]

@sandal There is already a line_gap method in Prawn::Text::Formatted::Box.

The final gap can be removed by (in process_vertical_alignment):

when :bottom
  @at[1] = @at[1] - (@height - height) + @descender
  @at[1] -= line_gap
end

I was able to implement the fixes to valign: :center and valign: :bottom that you described above on a local fork. I'd be happy to tackle this issue and submit a pull request.

[practicingruby]

@jessedoyle A pull request would be welcome, thanks!

[jessedoyle]

@sandal Here's a few examples of the implemented fixes:

Using the same code you demonstrated above:

Notice here how the center aligned element has top/bottom gaps equal to the distance from the bottom to the baseline.

Using :final_gap => false on the last element:

Here we can see that the bottom aligned element does not have a final gap between the box bottom and the descender.

I'll submit a pull request shorty. Let me know what you think!

[practicingruby]

@jessedoyle: Thanks. I think we still need :initial_gap though, which would put a bit of space between the top of the box and the text (equal to the line_gap). When set to false, the gap would be suppressed.

The default for :initial_gap should be set to match the existing behavior, so as to not break backwards compatibility.

[jessedoyle]

@sandal Adding an :initial_gap option should not be difficult.

Would the option only be relevant to :valign => :center? Or would it also apply to :valign => :top (as this would not be the same as existing behaviour)?

[practicingruby]

It'd apply to both :top and :center, for sure.

I'm trying to think through how :bottom should work. If you align to the bottom and shrink to fit, on overflow the initial line gap should probably be preserved if :initial_gap is set to true. So it seems like it applies to all of the cases.

[jessedoyle]

@sandal I'm not sure I agree with your last comment.

If you set :valign => :bottom, regardless of what :initial_gap would be set to, shouldn't the text render as close to the bottom of the box as possible?

In this case, there would always be an 'initial gap', which consists of the empty space at the top of the box.

This seems to be logical to me, even if the font size is reduced due to :shrink_to_fit.

[practicingruby]

@jessedoyle: Think of the initial gap and final gap as simply being a top and bottom margin, and then we're trying to vertically align the whole box.

The edge case with bottom alignment I'm worried about is this one:

If we ignore :initial_gap in bottom alignment, we can end up in situations where the top line of text touches the top line of the box, which is identical to what would happen if we ignored :final_gap in top alignment.

So when :initial_gap is set to true, we need to include it in the height calculations, so that it triggers an overflow and shrinks the text.

[practicingruby]

I'm wondering if we need to think a little more on all of this. I'm having trouble thinking of when you would want to set :initial_gap independent of :final_gap. If we're only supporting booleans, it seems like you'll want them both on or both off.

But if we were to offer finer grained control, we might want to implement something like :padding_top and :padding_bottom instead, which could be given arbitrary values. The default for both would be font.line_gap, but setting them to zero would completely suppress the gap. But this opens the door for having uneven spacing on the top and bottom of a block of text, which might be desirable.

What do you think?

[jessedoyle]

@sandal: I definitely understand you now.

I agree that there should be some more thought on this.

Honestly, I don't think an initial_gap option is necessary. It would have to default to false to match current behaviour, and I don't think that users would find such an option useful. Instead, they would likely call move_down elsewhere to achieve similar output.

Furthermore, my efforts to implement the :initial_gap option have not yet been successful.

Here's my current code for process_vertical_alignment:

def process_vertical_alignment(text)
  return if defined?(@vertical_alignment_processed) && @vertical_alignment_processed
  @vertical_alignment_processed = true

   return if @vertical_align == :top

   wrap(text)

   case @vertical_align
   when :center
     @at[1] -= (@height - height + @descender) * 0.5
   when :bottom
     @at[1] -= (@height - height)
     @at[1] += line_gap if @final_gap
   end

  @height = height
end

You can change the code to:

def process_vertical_alignment(text)
  return if defined?(@vertical_alignment_processed) && @vertical_alignment_processed
  @vertical_alignment_processed = true
   wrap(text)

   case @vertical_align
   when :top
     # Do something if @inital_gap
   when :center
     @at[1] -= (@height - height + @descender) * 0.5
   when :bottom
     @at[1] -= (@height - height)
     @at[1] += line_gap if @final_gap
   end

  @height = height
end

But this causes tests to break, likely due to not returning before the wrap(text) call sets various instance variables.

Any ideas?

[practicingruby]

@jessedoyle: Thanks for the work you've done on this so far, it's a huge help.

The notion that we include a line gap at the bottom of the text box by default but not at the top is almost certainly a historical accident, and so I'm willing to consider this to be a bug, because it's never going to be desirable behavior.

With that in mind, I think we should figure out what the correct behavior is:

1. If we include any whitespace at all within text boxes, we should give full control over it. That is to say we probably need to implement all the different padding options (left, right, top, and bottom). This is a complicated change, but it can get fairly close to being backwards compatible by setting those values to 0 by default.
2. The other option I think is viable is to say that text box should be modified to drop all padding entirely, which would lead it up to the user to take care of adding whitespace above and below the box themselves.

I'm in favor of the first approach (supporting all forms of padding) because it's going to be the most natural user experience. Unfortunately, it's also the most complex, and it may mean that I'd need to write this patch myself unless you're up for doing a lot of rework.

The original suggestions I made about tweaking :final_gap behavior were to preserve 100% backwards compatibility, but on closer inspection the default behavior I was trying to preserve is basically broken. So rather than working around the problem, I think we should fix it.

[practicingruby]

@jessedoyle It seems like the work you've done gets us close to option 2. Since it's going to be a pre-requisite for implementing option 1 anyway, maybe you can revise your pull request to simply act as if :final_gap is always set to false, then work to remove the option and see what breaks?