alphagov / govuk-frontend

GOV.UK Frontend contains the code you need to start building a user interface for government platforms and services.
https://frontend.design-system.service.gov.uk/
MIT License
1.18k stars 325 forks source link

Unnecessary CSS is included in IE8 stylesheet #3231

Closed querkmachine closed 1 year ago

querkmachine commented 1 year ago

Cause

Unnecessary CSS, such as repeated style rules and features that are not supported by Internet Explorer 8, is still being included in the IE8-specific stylesheet, despite efforts to automatically remove these features.

Noticed during the 4.5.0 release process.

Repeated rules

Rules are repeated in several places where media queries have been removed. postcss-unmq should be removing the duplicates and only including the rules relevant to the configured screen dimensions (by default, 1024×768)—however this doesn't appear to be happening.

The presence of both rem and px unit versions is due to node-pixrem. Having the rem unit versions is unnecessary in the IE8 stylesheet, too.

.govuk-\!-font-size-19 {
    font-size: 16px!important;
    font-size: 1rem!important;
    line-height: 1.25!important;
    font-size: 19px!important;
    font-size: 1.1875rem!important;
    line-height: 1.31579!important;
}

Unsupported rules

Unsupported CSS rules are still included, such as the flexbox-related rules here.

.govuk-button-group {
    margin-bottom: 15px;
    display: flex;
    flex-direction: column;
    align-items: center;
    margin-right: -15px;
    flex-direction: row;
    flex-wrap: wrap;
    align-items: baseline;
}

And box-shadow and box-decoration-break rules here:

.js-enabled .govuk-accordion__show-all:focus {
    color: #0b0c0c;
    background-color: #fd0;
    box-shadow: 0 -2px #fd0,0 4px #0b0c0c;
    text-decoration: none;
    box-decoration-break: clone;
}

Unsupported selectors

Selectors incorporating the :not selector are included in the stylesheet, despite not being supported by IE8.

.govuk-link--no-underline:not(:hover):not(:active) {
    text-decoration: none;
}

Same for :last-child and :last-of-type here.

.govuk-checkboxes__item:last-child,
.govuk-checkboxes__item:last-of-type {
    margin-bottom: 0;
}

Unsupported functions

Several CSS functions not supported by IE8 are still included. In this example, none of calc(), env() or max() are supported in IE8.

@supports (margin:max(calc(0px))) {
    .govuk-width-container {
        margin-right: max(15px,calc(15px + env(safe-area-inset-right)));
        margin-left: max(15px,calc(15px + env(safe-area-inset-left)));
        margin-right: max(30px,calc(15px + env(safe-area-inset-right)));
        margin-left: max(30px,calc(15px + env(safe-area-inset-left)));
    }
}

Third-party browser prefixes

Some browser-prefixed properties and values are still included, despite not being relevant to IE8.

.govuk-link {
    font-family: GDS Transport,arial,sans-serif;
    -webkit-font-smoothing: antialiased;
    -moz-osx-font-smoothing: grayscale;
    text-decoration: underline;
}

@supports at-rule

@supports at-rules are included in the stylesheet, despite IE8 not supporting them.

@supports (content-visibility:hidden) {
    .js-enabled .govuk-accordion__section-content[hidden] {
        content-visibility: hidden;
        display: inherit;
    }
}

Consequences

We ship significantly more CSS to IE8 than the browser is capable of using. This may have an exacerbated effect on performance as any contemporary IE8 users are probably running very old hardware and software.

Impact of debt

Low

Reason

Considered as low because:

Effort to pay down

High

Reason

Some of these issues could be resolved manually by wrapping the offending code in the govuk-not-ie8 mixin, so that it's not included in the IE8 stylesheet.

For others: We currently use the Oldie plugin to automatically remove unsupported features in the IE8 stylesheet. Oldie appears to be unmaintained, with issues and pull requests not being monitored. We would likely have to fork the plugin or its dependencies if we wanted to make updates to how it works.

Despite us planning to drop support for IE8 going forward, we will still be supporting it on a security and major bugfix basis for at least a year following the release of Frontend v5. We may want to pay down some of this debt so that things are in a better place before we reduce and drop IE8 support completely.

Overall rating

Low

36degrees commented 1 year ago

I think there are a couple of things that are going on in our codebase that are worth pointing out as they are somewhat hidden / confusing:

Rules are repeated in several places where media queries have been removed. postcss-unmq should be removing the duplicates and only including the rules relevant to the configured screen dimensions (by default, 1024×768)—however this doesn't appear to be happening.

We're using a vendored version of sass-mq under the hood – wherever you see the govuk-media-query mixin being used, under the hood that's calling sass-mq:

https://github.com/alphagov/govuk-frontend/blob/8620a4f203ead0458585ec525fca7fa2311d9ba3/src/govuk/helpers/_media-queries.scss#L87-L91

One of the features of sass-mq is that it 'rasterizes' media queries – so wherever govuk-media-query is used and the media query matches our desktop breakpoint, it'll be outputted without being wrapped in a media query:

https://github.com/alphagov/govuk-frontend/blob/8620a4f203ead0458585ec525fca7fa2311d9ba3/src/govuk/vendor/_sass-mq.scss#L198-L215

There's nothing intelligent going on to remove duplicates and – because this happens as part of the Sass compilation step – there's no media query left for unmq to do anything with.

We do have a few other places in the codebase where we use @media rules directly instead of calling govuk-media-query (not for breakpoints – typically used for things like @media (forced-colors: active) or @media (hover: none). Because these don't go through the mixin, they are included in the CSS and this is where unmq does help as it removes them for us.

The presence of both rem and px unit versions is due to node-pixrem. Having the rem unit versions is unnecessary in the IE8 stylesheet, too.

.govuk-\!-font-size-19 {
    font-size: 16px!important;
    font-size: 1rem!important;
    line-height: 1.25!important;
    font-size: 19px!important;
    font-size: 1.1875rem!important;
    line-height: 1.31579!important;
}

To the best of my knowledge, we've never used node-pixrem for this. Our typography scale is defined in px, and our typography helpers convert them to rem and output both font-size declarations:

https://github.com/alphagov/govuk-frontend/blob/8620a4f203ead0458585ec525fca7fa2311d9ba3/src/govuk/helpers/_typography.scss#L150-L153

There's a flag to enable this ($govuk-typography-use-rem). If we wanted to, we could update the govuk-typography-responsive mixin to consider both flags (@if $govuk-typography-use-rem and not $govuk-is-ie8 {) or consider $govuk-is-ie8 when setting the default value for $govuk-typography-use-rem.

querkmachine commented 1 year ago

Closing as we're now starting work on GOV.UK Frontend 5, which drops support for IE8.