Skip to content
This repository has been archived by the owner on Sep 6, 2021. It is now read-only.

Use integer line-height to avoid rounding differences #3849

Merged
merged 4 commits into from
May 17, 2013

Conversation

redmunds
Copy link
Contributor

Yet another attempt at #3478.

@iwehrman Can you try this branch to see if it fixes the issue for you?

@iwehrman
Copy link
Contributor

Nailed it!

nail

@redmunds
Copy link
Contributor Author

@TomMalbran Care to review this one? I updated fix so it doesn't round off em values.

@ghost ghost assigned TomMalbran May 16, 2013
@TomMalbran
Copy link
Contributor

Sure I can.

This looks good and finally fixes the problem, but since we previously increased the line height ratio a lot to avoid rounding it made a big gap on a really big font size between the light backgrounds of 2 consecutive rows. Since we now need to round the value, maybe we could decrease the line height ratio a bit, 1.27 seems to work nicely on Win XP. But with 1.27 the fourth view commands integration test is failing, for a similar problem to the ones fixed. So this test would need to be fixed too.

@redmunds
Copy link
Contributor Author

I definitely want to fix any gaps, but a ratio of 1.27 seems too weird for me. I tried 1.25 with Math.round() but that didn't fix the original problem. Maybe 1.25 with Math.ceil() will work?

@TomMalbran
Copy link
Contributor

1.25 and ceil seems to be working on Windows... does it work on Mac too?

@redmunds
Copy link
Contributor Author

@TomMalbran 1.25 and ceil looks good on Mac, both retina and non-retina. I see some gaps if you make font-size super large, but that's not a common workflow.

Chnages pushed. Ready for re-review.

@TomMalbran
Copy link
Contributor

This looks great. I can see some gaps at some big font-sizes but is just a 1px gap, and I don't think we can do nothing about it, but it is a lot less than with the previews line height ratio.

Before merging this I see a failure in the fourth test in view command handlers. Do we need to fix that now or add a new issue for it? It has a similar problem to the other 2 tests, when we switched from 1px updates to line ratios ones. And there is a issue with Travis, with one of the tests, which I am not sure why it is here. Maybe a final merge with master could fix it?

@redmunds
Copy link
Contributor Author

Sorry, I forgot about the unit test. Pushed a fix.

@peterflynn
Copy link
Member

Fwiw -- I have a feeling we'll switch to a different way of highlighting Find results in a month or two anyway, so hopefully the gaps at larger sizes will eventually go away for free...

@TomMalbran
Copy link
Contributor

Great, tests are passing now. Merging.

TomMalbran added a commit that referenced this pull request May 17, 2013
Use integer line-height to avoid rounding differences
@TomMalbran TomMalbran merged commit c918b0b into master May 17, 2013
@TomMalbran TomMalbran deleted the randy/issue-3478-again branch May 17, 2013 22:50
@redmunds
Copy link
Contributor Author

Thanks, @TomMalbran !

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants