Remove useless $ie-font-ratio variable from vertical_rhythm partial #766

Closed
wants to merge 13 commits into
from

Conversation

Projects
None yet
8 participants
@JohnAlbin
Contributor

JohnAlbin commented Mar 10, 2012

I've expressed my dislike for the documentation for $ie-font-ratio before. See http://compass-style.org/reference/compass/typography/vertical_rhythm/#dsq-comment-252111493

I've recently poked around at the source code and discovered that the $ie-font-ratio is the worst kind of global variable:

  • It's only used once
  • The value should never be changed
  • It pollutes the global namespace without any practical benefit
  • It isn't documented in any useful way

There is no such thing as the "ie font ratio". The only Google results are copies of this source code. http://www.google.com/search?q=%22ie+font+ratio%22

But what that variable actually does is set the font-size to use a percentage value for IE6. This is a well-known issue with IE6. It won't resize text that has been set in pixels. Why the variable doesn't explain that is beyond me.

So, given the downsides of keeping this variable, this commit removes that global and puts the 100% and 16px into the actual rule that is needed for IE6. It also adds useful inline docs for why its needed.

@JohnAlbin

This comment has been minimized.

Show comment Hide comment
@JohnAlbin

JohnAlbin Mar 10, 2012

Contributor

Oh, I also found 2 typos in that file which were also fixed in that commit.

Contributor

JohnAlbin commented Mar 10, 2012

Oh, I also found 2 typos in that file which were also fixed in that commit.

@JohnAlbin

This comment has been minimized.

Show comment Hide comment
@JohnAlbin

JohnAlbin Mar 11, 2012

Contributor

I realized I only partially explained the reason we use percentages. So I expanded the inline comments and pushed a new commit.

IE6 won't resize text that has been set in pixels.

The other issue with IE6 is that it weirdly scales text whose root is set in ems. You can use ems for fonts elsewhere in the doc without issue, just not on the root element.

Contributor

JohnAlbin commented Mar 11, 2012

I realized I only partially explained the reason we use percentages. So I expanded the inline comments and pushed a new commit.

IE6 won't resize text that has been set in pixels.

The other issue with IE6 is that it weirdly scales text whose root is set in ems. You can use ems for fonts elsewhere in the doc without issue, just not on the root element.

@chriseppstein

This comment has been minimized.

Show comment Hide comment
@chriseppstein

chriseppstein Mar 12, 2012

Member

@ericam Can you chime in here?

Member

chriseppstein commented Mar 12, 2012

@ericam Can you chime in here?

@JohnAlbin

This comment has been minimized.

Show comment Hide comment
@JohnAlbin

JohnAlbin Mar 12, 2012

Contributor

After further investigation, I'm reminded that IE7, IE8 and possibly IE9 all refuse to resize text set in pixels. Though, IE7-9 have a page zoom feature which mitigates that bug somewhat. Accessibility experts still call it a bug not to be able to resize the font without resizing the entire page.

So we can simplify the CSS further by just applying the font-size: 100% * ($font-size / 16px); for all browsers and not wrap that in an IE6-specific hack.

Contributor

JohnAlbin commented Mar 12, 2012

After further investigation, I'm reminded that IE7, IE8 and possibly IE9 all refuse to resize text set in pixels. Though, IE7-9 have a page zoom feature which mitigates that bug somewhat. Accessibility experts still call it a bug not to be able to resize the font without resizing the entire page.

So we can simplify the CSS further by just applying the font-size: 100% * ($font-size / 16px); for all browsers and not wrap that in an IE6-specific hack.

JohnAlbin added a commit to JohnAlbin/compass that referenced this pull request Mar 12, 2012

@barraponto

This comment has been minimized.

Show comment Hide comment
@barraponto

barraponto Mar 12, 2012

Contributor

@JohnAlbin that variable was documented as a constant, for what it's worth.

Contributor

barraponto commented Mar 12, 2012

@JohnAlbin that variable was documented as a constant, for what it's worth.

@JohnAlbin

This comment has been minimized.

Show comment Hide comment
@JohnAlbin

JohnAlbin Mar 12, 2012

Contributor

Right, constants are really useful in 2 situations:

  1. You use the same constant value repeatedly in your code.
  2. You create a constant value that can be re-used by developers in their code.

The $ie-font-ratio fails both of those definitions of a "constant". Yes, the value is something you don't want to change. But this value is only ever used once on a website, to fix the IE bugs I described earlier. And it can't be re-used by Compass users because that IE bug only apply to a CSS rule using the root selector; again, there's only one of those on any website.

Right now its just noise that gets in the way of Compass users from figuring out how to use the vertical_rhythm partial. That's my real motivation here: improve the documentation so users are more likely to use the partial.

One thing I didn't comment on is that the commit also changes the selector from body {} to html {}. There's 2 reasons for that:

  1. The normalize.css project uses the html element to add that rule. As do most modern examples of this IE work-around.
  2. The partial currently has token rem support. But it doesn't actually work at all because the rem is relative to the root element, i.e. the html element. Currently, the establish-baseline() doesn't set the root font size. It sets the body font size, so anytime rem is used on the site it is relative to the browser default 16px size and not to the font size set with establish-baseline(). (Note: I've already got some code to add proper rem support with px backup, but that's a separate commit.)
Contributor

JohnAlbin commented Mar 12, 2012

Right, constants are really useful in 2 situations:

  1. You use the same constant value repeatedly in your code.
  2. You create a constant value that can be re-used by developers in their code.

The $ie-font-ratio fails both of those definitions of a "constant". Yes, the value is something you don't want to change. But this value is only ever used once on a website, to fix the IE bugs I described earlier. And it can't be re-used by Compass users because that IE bug only apply to a CSS rule using the root selector; again, there's only one of those on any website.

Right now its just noise that gets in the way of Compass users from figuring out how to use the vertical_rhythm partial. That's my real motivation here: improve the documentation so users are more likely to use the partial.

One thing I didn't comment on is that the commit also changes the selector from body {} to html {}. There's 2 reasons for that:

  1. The normalize.css project uses the html element to add that rule. As do most modern examples of this IE work-around.
  2. The partial currently has token rem support. But it doesn't actually work at all because the rem is relative to the root element, i.e. the html element. Currently, the establish-baseline() doesn't set the root font size. It sets the body font size, so anytime rem is used on the site it is relative to the browser default 16px size and not to the font size set with establish-baseline(). (Note: I've already got some code to add proper rem support with px backup, but that's a separate commit.)
@chriseppstein

This comment has been minimized.

Show comment Hide comment
@chriseppstein

chriseppstein Mar 12, 2012

Member

I should point out that the constant can be marked @private in the comments and then it will not be documented on the website :)

Member

chriseppstein commented Mar 12, 2012

I should point out that the constant can be marked @private in the comments and then it will not be documented on the website :)

@JohnAlbin

This comment has been minimized.

Show comment Hide comment
@JohnAlbin

JohnAlbin Mar 12, 2012

Contributor

That's good to know! I'll keep that in mind while looking at the other constants in vertical_rhythm.

But the $ie-font-ratio variable can still only ever be used once. It shouldn't be a variable or constant, hidden or not.

Contributor

JohnAlbin commented Mar 12, 2012

That's good to know! I'll keep that in mind while looking at the other constants in vertical_rhythm.

But the $ie-font-ratio variable can still only ever be used once. It shouldn't be a variable or constant, hidden or not.

@chriseppstein

This comment has been minimized.

Show comment Hide comment
@chriseppstein

chriseppstein Mar 12, 2012

Member

@JohnAlbin That's a matter of coding style and preference. Should a comment be used or a name to describe the magic number and why it is what it is. As long as it's not distracting users from the important stuff, I don't see a compelling reason to bikeshed this issue.

Member

chriseppstein commented Mar 12, 2012

@JohnAlbin That's a matter of coding style and preference. Should a comment be used or a name to describe the magic number and why it is what it is. As long as it's not distracting users from the important stuff, I don't see a compelling reason to bikeshed this issue.

@mirisuzanne

This comment has been minimized.

Show comment Hide comment
@mirisuzanne

mirisuzanne Mar 12, 2012

Member

I see three distinct questions here.

  1. Do we need a constant for the ie-font-ratio? Since it should never be changed, and is only has one use, I agree with @JohnAlbin that it should be removed.

  2. Do we need to declare the % and px sizes separately? This is a larger debate between designers who like more control, and designers who like to leave control in the hands of the user. A %-based font-size will be relative to the user's default font size (16px by default, but they can adjust). A px-based font-size will override the user's default. The user can still zoom to override that, but their default setting is ignored. There are valid arguments both ways, so until we have hashed it out in more detail I vote for sticking with what we had. No point in changing default behavior on all our users without more discussion.

  3. Do we apply the rules to HTML instead of BODY? I haven't done cross-browser testing on this. If applying to HTML works on all the browsers, I agree that this is a good move.

Pull out the changes to 2 (you can start an issue here or a thread on google groups to discuss more), and do the testing on 3. At that point, I'm in favor of this patch.

Member

mirisuzanne commented Mar 12, 2012

I see three distinct questions here.

  1. Do we need a constant for the ie-font-ratio? Since it should never be changed, and is only has one use, I agree with @JohnAlbin that it should be removed.

  2. Do we need to declare the % and px sizes separately? This is a larger debate between designers who like more control, and designers who like to leave control in the hands of the user. A %-based font-size will be relative to the user's default font size (16px by default, but they can adjust). A px-based font-size will override the user's default. The user can still zoom to override that, but their default setting is ignored. There are valid arguments both ways, so until we have hashed it out in more detail I vote for sticking with what we had. No point in changing default behavior on all our users without more discussion.

  3. Do we apply the rules to HTML instead of BODY? I haven't done cross-browser testing on this. If applying to HTML works on all the browsers, I agree that this is a good move.

Pull out the changes to 2 (you can start an issue here or a thread on google groups to discuss more), and do the testing on 3. At that point, I'm in favor of this patch.

@mirisuzanne

This comment has been minimized.

Show comment Hide comment
@mirisuzanne

mirisuzanne Mar 12, 2012

Member

@chriseppstein It looks to me like the name is arbitrary enough that it has to be explained in a comment either way. This isn't really an IE font ratio. It's the font ratio all browsers use by default, we just target IE6 to hack around lack-of-zoom.

I think we should be discussing a new feature: $use-relative-base-font-size. If set to true, all browsers get the % declaration. set to false, most browsers get the px declaration and (if $legacy-support-for-ie6) IE6 still gets a % value.

In that case the constant would actually be used in two places, but should get a new name and be made private.

Member

mirisuzanne commented Mar 12, 2012

@chriseppstein It looks to me like the name is arbitrary enough that it has to be explained in a comment either way. This isn't really an IE font ratio. It's the font ratio all browsers use by default, we just target IE6 to hack around lack-of-zoom.

I think we should be discussing a new feature: $use-relative-base-font-size. If set to true, all browsers get the % declaration. set to false, most browsers get the px declaration and (if $legacy-support-for-ie6) IE6 still gets a % value.

In that case the constant would actually be used in two places, but should get a new name and be made private.

@chriseppstein

This comment has been minimized.

Show comment Hide comment
@chriseppstein

chriseppstein Mar 12, 2012

Member

@ericam I actually already added that feature in 0.12. it's called $relative-font-sizing. It defaults to pixels, but it should be very easy to add support for rem once the browser support is in place.

Member

chriseppstein commented Mar 12, 2012

@ericam I actually already added that feature in 0.12. it's called $relative-font-sizing. It defaults to pixels, but it should be very easy to add support for rem once the browser support is in place.

@mirisuzanne

This comment has been minimized.

Show comment Hide comment
@mirisuzanne

mirisuzanne Mar 12, 2012

Member

@chriseppstein It looks to me like your $relative-font-sizing only affects adjustments to the font size. I'm talking about how the base font size is set in establish-baseline. Currently, it is always set relative for IE6, and fixed-px for all other browsers. This patch (currently) would change that to always-relative.

Member

mirisuzanne commented Mar 12, 2012

@chriseppstein It looks to me like your $relative-font-sizing only affects adjustments to the font size. I'm talking about how the base font size is set in establish-baseline. Currently, it is always set relative for IE6, and fixed-px for all other browsers. This patch (currently) would change that to always-relative.

@chriseppstein

This comment has been minimized.

Show comment Hide comment
@chriseppstein

chriseppstein Mar 12, 2012

Member

@ericam I guess I don't understand what the trade-offs of that would be. Why should the user consider this?

Member

chriseppstein commented Mar 12, 2012

@ericam I guess I don't understand what the trade-offs of that would be. Why should the user consider this?

@mirisuzanne

This comment has been minimized.

Show comment Hide comment
@mirisuzanne

mirisuzanne Mar 12, 2012

Member

See my point 2 above. It relates to the users default font size setting in browser preferences. I don't know how often users change that setting from the default 16px, but they can. If we set our base size in px (we currently do) we override that browser setting. I think that's generally considered acceptable, but it is a choice we are making. Declaring our base font size in % instead (as this patch does) would be a change. That % would be relative to the browser setting.

If your browser has default font size set to 16px (the default on all browsers):

font-size: 100%; == font-size: 16px;

But if you change the browser setting to e.g. 24px:

font-size: 100%; == font-size: 24px; and font-size: 16px; == font-size: 66.667%;

Member

mirisuzanne commented Mar 12, 2012

See my point 2 above. It relates to the users default font size setting in browser preferences. I don't know how often users change that setting from the default 16px, but they can. If we set our base size in px (we currently do) we override that browser setting. I think that's generally considered acceptable, but it is a choice we are making. Declaring our base font size in % instead (as this patch does) would be a change. That % would be relative to the browser setting.

If your browser has default font size set to 16px (the default on all browsers):

font-size: 100%; == font-size: 16px;

But if you change the browser setting to e.g. 24px:

font-size: 100%; == font-size: 24px; and font-size: 16px; == font-size: 66.667%;

@ry5n

This comment has been minimized.

Show comment Hide comment
@ry5n

ry5n Mar 13, 2012

Contributor

@JohnAlbin The $ie-font-ratio constant really confused me at first. In the last few weeks I basically came to the same conclusion, that it should either be sunk into a one-time calculation in +establish-baseline or it should be stored as the default we're all familiar with, namely, $browser-default-font-size: 16px.

@ericam I'd also like to discuss a configurable variable to change the behaviour of +establish-baseline. I prefer to honour the user/browser's baseline font size, and it would be nice to have that option in core.

BTW, I've also been working on adding full rem support (with pixel fallbacks) to the vertical-rhythm module. I'd love to be part of a separate discussion on that.

Contributor

ry5n commented Mar 13, 2012

@JohnAlbin The $ie-font-ratio constant really confused me at first. In the last few weeks I basically came to the same conclusion, that it should either be sunk into a one-time calculation in +establish-baseline or it should be stored as the default we're all familiar with, namely, $browser-default-font-size: 16px.

@ericam I'd also like to discuss a configurable variable to change the behaviour of +establish-baseline. I prefer to honour the user/browser's baseline font size, and it would be nice to have that option in core.

BTW, I've also been working on adding full rem support (with pixel fallbacks) to the vertical-rhythm module. I'd love to be part of a separate discussion on that.

@JohnAlbin

This comment has been minimized.

Show comment Hide comment
@JohnAlbin

JohnAlbin Mar 13, 2012

Contributor

@ry5n that's exactly what I've been working on! I've forked your rem repo to take a look. See also #771.

Contributor

JohnAlbin commented Mar 13, 2012

@ry5n that's exactly what I've been working on! I've forked your rem repo to take a look. See also #771.

@ry5n

This comment has been minimized.

Show comment Hide comment
@ry5n

ry5n Mar 13, 2012

Contributor

@JohnAlbin Cool! I just pushed some changes to it, should be stable. Not sure if the full extent is needed, may just need the px-to-rem portion. You'll likely also want to take a look at this Gist, which is my (heavily-modified) vertical-rhythm partial.

Contributor

ry5n commented Mar 13, 2012

@JohnAlbin Cool! I just pushed some changes to it, should be stable. Not sure if the full extent is needed, may just need the px-to-rem portion. You'll likely also want to take a look at this Gist, which is my (heavily-modified) vertical-rhythm partial.

@mirisuzanne

This comment has been minimized.

Show comment Hide comment
@mirisuzanne

mirisuzanne Mar 13, 2012

Member

@ry5n and @JohnAlbin - Looks cool. Excited to see where that goes. It deserves a ticket of it's own, though. One of you want to get that started?

Member

mirisuzanne commented Mar 13, 2012

@ry5n and @JohnAlbin - Looks cool. Excited to see where that goes. It deserves a ticket of it's own, though. One of you want to get that started?

@ry5n

This comment has been minimized.

Show comment Hide comment
@ry5n

ry5n Mar 13, 2012

Contributor

I've created a new issue for this: chriseppstein#775. Also excited to see what we can do.

Contributor

ry5n commented Mar 13, 2012

I've created a new issue for this: chriseppstein#775. Also excited to see what we can do.

@chriseppstein

This comment has been minimized.

Show comment Hide comment
@chriseppstein

chriseppstein Mar 14, 2012

Member

I think this patch is fine, but it needs to be rebased against master so that it applies cleanly there. And I agree that the magic 16px should be explained by setting it to $browser-default-font-size.

Member

chriseppstein commented Mar 14, 2012

I think this patch is fine, but it needs to be rebased against master so that it applies cleanly there. And I agree that the magic 16px should be explained by setting it to $browser-default-font-size.

Merge branch 'stable'
Conflicts:
	VERSION.yml
	doc-src/Gemfile.lock
@krisbulman

This comment has been minimized.

Show comment Hide comment
@krisbulman

krisbulman Mar 14, 2012

+1 for $browser-default-font-size, so much better

+1 for $browser-default-font-size, so much better

@scottdavis

This comment has been minimized.

Show comment Hide comment
@scottdavis

scottdavis Mar 14, 2012

Member

If this is a purely cosmetic change im -1 but if the can we solved by using private im +1

Member

scottdavis commented Mar 14, 2012

If this is a purely cosmetic change im -1 but if the can we solved by using private im +1

@JohnAlbin

This comment has been minimized.

Show comment Hide comment
@JohnAlbin

JohnAlbin Mar 14, 2012

Contributor

Ok. I replaced the $ie-font-ratio variable (which makes no sense) with a $browser-default-font-size variable.

I also modified the code per @ericam's point 2.

No point in changing default behavior on all our users without more discussion.

Agreed. I perfer using % to set the font-size for all browsers, but I'll open a new issue to make that an option in $establish-baseline(). So you can toggle between the default (which will remain the same) and the new way.

Contributor

JohnAlbin commented Mar 14, 2012

Ok. I replaced the $ie-font-ratio variable (which makes no sense) with a $browser-default-font-size variable.

I also modified the code per @ericam's point 2.

No point in changing default behavior on all our users without more discussion.

Agreed. I perfer using % to set the font-size for all browsers, but I'll open a new issue to make that an option in $establish-baseline(). So you can toggle between the default (which will remain the same) and the new way.

@JohnAlbin

This comment has been minimized.

Show comment Hide comment
@JohnAlbin

JohnAlbin Mar 14, 2012

Contributor

Hmm… I did rebase the latest commit against master as Chris requested. But the issue is now showing all the changes between my commit on master vs. stable. Whoops! Let me create a new pull-request with the commit, but let's keep the discussion here.

Contributor

JohnAlbin commented Mar 14, 2012

Hmm… I did rebase the latest commit against master as Chris requested. But the issue is now showing all the changes between my commit on master vs. stable. Whoops! Let me create a new pull-request with the commit, but let's keep the discussion here.

@mirisuzanne

This comment has been minimized.

Show comment Hide comment
@mirisuzanne

mirisuzanne Mar 14, 2012

Member

Looks good. I'm all for making that variable private, as well.

Member

mirisuzanne commented Mar 14, 2012

Looks good. I'm all for making that variable private, as well.

@ry5n

This comment has been minimized.

Show comment Hide comment
@ry5n

ry5n Mar 14, 2012

Contributor

+1 for private

Contributor

ry5n commented Mar 14, 2012

+1 for private

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment