Re: [PATCH] Fix line_height eval and centering of text in text input field.

Hi Achal-Aggarwal,

Thanks for this patch. We need to fix up a few things, but you did a
good job tracking down the source of the bug in our rather large code
base. I hope this is an instructive and valuable experience for you.

On 20/03/14 21:32, Achal-Aggarwal wrote:
> ---
> desktop/textarea.c | 9 +++++----
> render/font.c | 3 +--
> 2 files changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/desktop/textarea.c b/desktop/textarea.c
> index 584642d..dcaa2e8 100644
> --- a/desktop/textarea.c
> +++ b/desktop/textarea.c
> @@ -1786,7 +1786,9 @@ static void textarea_setup_text_offsets(struct textarea *ta)
> /* Single line text area; text is vertically centered */
> int vis_height = ta->vis_height - 2 * ta->border_width;
> text_y_offset += (vis_height - ta->line_height + 1) / 2;
> - text_y_offset_baseline += (vis_height * 3 + 2) / 4;
> + if (text_y_offset < 0)
> + text_y_offset = 0;

I'm not sure this forcing of non-negative y-offset is correct.

Previously if the visible height of the textarea was less than the line
height we were clipping from the top and bottom to show the middle, but
with that change we would only see the top. The top of lower case latin
letters doesn't often contain much.

> + text_y_offset_baseline += text_y_offset + (ta->line_height * 3 + 2) / 4;

That would work, but you've accumulated more potential for error.
e.g. there is a +/- half pixel error from the text_y_offset
calculation's division, and another +/- half pixel error from the
division in the text_y_offset_baseline calc.

Better would be to get the text_y_offset_baseline with only one
division, and therefore less places to lose fractions of pixels.

I expect something like this would work:

text_y_offset_baseline += (2 * vis_height + ta->line_height + 2) / 4;

> }
>
> ta->text_y_offset = text_y_offset;
> @@ -1894,9 +1896,8 @@ struct textarea *textarea_create(const textarea_flags flags,
> ret->show = &ret->text;
> }
>
> - ret->line_height = FIXTOINT(FDIV((FMUL(FLTTOFIX(1.3),
> - FMUL(nscss_screen_dpi, INTTOFIX((setup->text.size))))),
> - FONT_SIZE_SCALE * F_72));
> + ret->line_height = FIXTOINT(FDIV(FMUL(nscss_screen_dpi,
> + INTTOFIX((setup->text.size)/FONT_SIZE_SCALE)),F_72)) * 1.3 ;

This change is not right.

First, you're introducing a multiply 1.3, which is a floating point
operation. The rest of that calculation is using fixed point to avoid
using floating point.

Second you're doing setup->text.size/FONT_SIZE_SCALE which will lose any
fractional part of the original font size. E.g. your change would make
12.8pt get treated as if it were 12pt. Third, I'm not sure why you're
modifying that calculation. I don't see anything wrong with it. Have I
missed something?


> diff --git a/render/font.c b/render/font.c
> index 03c5a36..f594678 100644
> --- a/render/font.c
> +++ b/render/font.c
> @@ -45,8 +45,7 @@ void font_plot_style_from_css(const css_computed_style *css,
> css_computed_font_family(css, &families));
>
> css_computed_font_size(css, &length, &unit);
> - fstyle->size = FIXTOINT(FMUL(nscss_len2pt(length, unit),
> - INTTOFIX(FONT_SIZE_SCALE)));
> + fstyle->size = FIXTOINT(nscss_len2pt(length, unit)) * FONT_SIZE_SCALE;

Again, this change isn't relevant to the bug as far as I can see.

Cheers,

Michael