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

Refine fix for #3478 #3757

Merged
merged 3 commits into from
May 9, 2013
Merged

Refine fix for #3478 #3757

merged 3 commits into from
May 9, 2013

Conversation

redmunds
Copy link
Contributor

@redmunds redmunds commented May 9, 2013

This is a refined attempt at #3478. Rounding the line-height up a pixel seems to fix it.

@TomMalbran Can you take a look?

@TomMalbran
Copy link
Contributor

This should do the trick, I was just about to test it when I saw your request. The request looks fine, but i was able to see the 1px difference on Windows too by taking a screen shot and zooming in, so the comment isn't accurate.

If we are using ceil to round the numbers, then we need to adjust the original line height, since Math.ceil(12 * 1.25) = 16, which is the value that the line height will get by increasing the font by 1 and then decreasing it.

This works with pixels, but should we care if it might not work with ems, since we are not using ceil which makes this work? Right now $.css("font-size") always returns the values in pixels, so it should work until the jQuery function changes.

@redmunds
Copy link
Contributor Author

redmunds commented May 9, 2013

I changed the line-height ratio constant in this file to be 1.26. Or do you mean the base css?

@TomMalbran
Copy link
Contributor

Yes, the base css is font-size: 12px and line-height: 15px but the line height for the base font-size is now Math.ceil(12 * 1.26) = 16, so the base line height should be 16px too.

@redmunds
Copy link
Contributor Author

redmunds commented May 9, 2013

On both Mac and Windows, when I look in Chrome Dev Tools at a page with default font size, I'm seeing line-height:16px. I also took screenshots and zoomed in on text, and I'm not seeing the problem on either.

@redmunds
Copy link
Contributor Author

redmunds commented May 9, 2013

@TomMalbran Updated comment.

@TomMalbran
Copy link
Contributor

Here https://github.com/adobe/brackets/blob/master/src/styles/brackets_theme_default.less#L125 is defined as 15px.

Pressing Ctrl+0 should restore the base values.

@redmunds
Copy link
Contributor Author

redmunds commented May 9, 2013

_adjustFontSize() gets called any time a doc is loaded, so the base css is overridden.

@TomMalbran
Copy link
Contributor

Right, there is no need to change it, maybe for completeness, or to know the base font-size.

Besides that and if it doesn't matter that the line height in ems and px would be different, when we actually can support em units having a way to retrieve the original value, this is good to merge. An alternative solution is to remove the rounding and use a higher value for the line-height ratio like 1.3.

I tested that this fixes the problem in Windows and I suppose that it should fix it in Mac too.

@redmunds
Copy link
Contributor Author

redmunds commented May 9, 2013

@TomMalbran Changing ratio to 1.3 fixes problem on Mac. Change pushed.

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

Great. Merging

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.

2 participants