Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove Safari line-height browser bug fix (Orig title: Allow more options for line-height, e.g. unitless) #11

Open
ronilaukkarinen opened this issue Apr 1, 2016 · 7 comments

Comments

@ronilaukkarinen
Copy link

I love using megatype for responsive typography, but in every element modular scale is not needed or necessary, but even for those elements it's nice to have "automated" sizing. However, if I set the type with typeset-mixin, I can't override line-height value. Here's an example:

    @include min-width(3 4) {
      @include typeset($sans, 18px, 36px, 0, 2);
      line-height: 1.5;
    }

Is it possible to have it like this: @include typeset($sans, 18px, 1.5, 0, 2);, or that the mixin would respect the definition after it?

@uvisgrinfelds
Copy link

@ronilaukkarinen It is possible to do so by usingline-height: 1.5!important override.

@ronilaukkarinen
Copy link
Author

Yes I know, but I rather not use !important since it's forcing the parameter, thus 'wrong'. That's the way I've been doing it, but I wonder why typeset won't accept directly in the mixin or allow line-height to be added without forcing it.

@tbredin
Copy link
Contributor

tbredin commented Apr 19, 2016

This looks strange to me. As far as I know, adding a line-height definition after the typeset mixin should work as an override, at least for the breakpoint that you've specified it for.

Note that each parameter of the typeset mixin can accept pixels, rems, or unitless numbers (which are interpreted as baseline units, ie: multiples of your current rootsize for that breakpoint). So you can actually currently enter @include typeset($sans, 18px, 1.5, 0, 2); and it will work, however a unitless value for the leading / line-height parameter in the typeset mixin is interpreted as baseline units, just for consistency when used alongside the other parameters.

Line height is also always output unitless, regardless of input.

@tbredin
Copy link
Contributor

tbredin commented May 10, 2016

Hey there, I realised this is likely related to a webkit bug, and a subsequent fix I have implemented in MegaType for webkit browsers. I have opened a bug with webkit; with unitless line-height webkit sometimes renders the line-height differently to the computed height (which reports itself as the correct value in inspector and when queried, despite rendering incorrectly on-screen).

For more information, check out the issue I opened, and the contained codepen that demonstrates the issue.

https://bugs.webkit.org/show_bug.cgi?id=155286

In MegaType, this results in type shifting off the grid sometimes on long pages, which can add up to a big problem. The fix in MegaType is output as something like:

@media screen and (-webkit-min-device-pixel-ratio: 0) {
    dd, dt, figcaption, p {
        line-height: 2rem;
    }
}

Which is a just a browser hack to apply line-height in rem units for webkit until this bug is fixed. Note that as this is higher specificity than your override, the override will be ignored.

As this is temp fix, and will be removed from MegaType soon, I recommend implementing something like the following, also as a temp fix until this is resolved in webkit.

@mixin lineheight($val) {
    line-height: $val;

    @media screen and (-webkit-min-device-pixel-ratio: 0) {
        line-height: $val;
    }
}

@include min-width(3 4) {
    @include typeset($sans, 18px, 36px, 0, 2);
    @include line-height(1.5);
}

I know this isn't ideal, but when MegaType removes this fix, you can also update the above mixin to remove the browser hack, or find/replace to remove the mixin altogether.

I'll leave this issue open to track the status of the webkit bug and subsequent removal of the MegaType fix.

@tbredin
Copy link
Contributor

tbredin commented Jun 22, 2016

I'm no longer replicating this in latest webkit, assume it's been resolved, so am removing the fix from MegaType.

Should now be easier to override line-height in webkit with MegaType version 1.0.9

@tbredin tbredin closed this as completed Jun 22, 2016
@tbredin
Copy link
Contributor

tbredin commented Jun 22, 2016

Scratch that, further testing is turning up some edge cases that are still rounding incorrectly

@tbredin tbredin reopened this Jun 22, 2016
@tbredin
Copy link
Contributor

tbredin commented Sep 26, 2016

As this adds a lot of media query calls, and is only still a problem in Safari as far as I can see, I've added a config option to turn it off globally if it's not important in a project to have to-the-pixel precision in Safari.

For example, this is common if working on a vertical rhythm in a single column, as it will never appear misaligned beside other content in an adjacent column.

Here's the default, but set to false to disable this output.

// fix line-height rounding errors in webkit safari
$webkit-line-height-fix: true !default;

@tbredin tbredin changed the title Allow more options for line-height, e.g. unitless (Orig title: Allow more options for line-height, e.g. unitless) Sep 26, 2016
@tbredin tbredin changed the title (Orig title: Allow more options for line-height, e.g. unitless) Remove Safari line-height browser bug fix (Orig title: Allow more options for line-height, e.g. unitless) Sep 26, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants