Skip to content

Conversation

@campaul
Copy link
Contributor

@campaul campaul commented Jun 17, 2025

Fix for #410 and a prerequisite for finishing #403
Edit: this does have some interactions with #403 but turns out the fixes can be implemented independently.

There are two components to this change:

  1. Only apply text alignment if the element being aligned is inline or inline-block.
  2. Use the alignStlye from the container instead of from the element being aligned.

This PR also expands test/text-align-center.html with test cases for some other common scenarios

@campaul
Copy link
Contributor Author

campaul commented Jun 18, 2025

The text-align-center test is currently failing due to imperceptible differences in font rendering. There are a few pixels different in the "r" at the end of "inner" in the 4th example and the "r" at the end of outer in the 5th example. This issue also isn't consistent across systems, as the test actually passes for me on some of the devices I've checked.

Do you think we should consider this a bug and try to get the font rendering identical, or would it be reasonable to add a small fuzz value to the comparison check? I've found a value of -fuzz 6% is enough to make this test pass.

Test

html

Reference

ref

@rodarima
Copy link
Member

Interesting, not sure what is causing those differences. I'm leaving here a magnified picture of the problem:

d

I would be inclined to think that they use floating point numbers internally to store the position of each glyph and there may be some rounding effects moving the r slightlighly to the right, causing the problem in one case but not in the other. It would be heavily dependant on your platform and the specific versions of the rendering libraries you use underneath.

Could we reduce the text length (say to one word only) and see if the problem goes away? I would prefer not adding fuzzing as that may conceal other problems.

@campaul
Copy link
Contributor Author

campaul commented Jun 21, 2025

Changing inner/outer to internal/external seems to have done the trick. Based on some other testing I did it seems like the issue has more to do with specific words than the length of the string.

Copy link
Member

@rodarima rodarima left a comment

Choose a reason for hiding this comment

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

Thanks, this looks great!

@rodarima rodarima added this to the Release 3.3.0 milestone Jun 29, 2025
campaul added 2 commits July 7, 2025 23:04
Inline elements are now aligned based on the text-align value of
their containing element instead of their own text-align value.
Block level elements are no longer affected by the text-align
property.

Fixes: dillo-browser#410
@rodarima rodarima merged commit 5012112 into dillo-browser:master Jul 7, 2025
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants