Bug 869516 - base styles for older versions of IE #1719

Closed
wants to merge 1 commit into
from

Projects

None yet

4 participants

@craigcook
Member

DO NOT MERGE - We need to do some testing of major pages, especially those that need to perform well in IE like Firefox product pages, /new, /fx, etc.

After burning a lot of time updating a bunch of individual pages, I changed my approach and arrived at something much simpler by adding a new block in the base templates. The existing site_css block and the new page_css block are both hidden from old IEs but the extrahead block is not. This means most pages that already use extrahead to include page-specific styles continue to work as they have (since they've already been tested with old IEs).

Future pages that need page-specific styling can now use the page_css block to hide those styles from oldIE and you usually won't need to bother testing. Old IE gets the basic styling and doesn't see your new CSS at all. Use good markup and your content should degrade to something perfectly readable and presentable, just not fancy.

Note that the JavaScript blocks are NOT hidden from old IEs, in order to avoid breaking a lot of pages already built and tested for them. In the future, you can include conditional comments inside the site_js or js blocks to let old IE degrade without any fancy JS, but I didn't want to make that change global in the base templates (I tried it; lots of stuff broke).

Script-heavy pages like /firefox/partners and /firefox/os will need some attention before this can merge since the JS on those pages makes some assumptions about the CSS. We can either ensure IE gets the styling (by putting it in extrahead) or we might be able to just hide the JS from oldIE on those pages and let them degrade.

Some pages might have content hidden in the page-specific CSS that is then made visible by JS when needed (stuff like modals and close buttons). If you're explicitly hiding elements in your page styles just remember that content will be visible in old IEs since they don't get your page-specific CSS. oldIE.less does include the same .hidden class from sandstone so you can still use that to visually hide content and it'll be hidden in IE as well.

@craigcook craigcook commented on the diff Feb 27, 2014
media/css/firefox/menu.less
@@ -56,7 +56,7 @@ body.html-rtl #nav-main li ul {
#nav-main li ul {
position: absolute;
left: -999em;
- top: 48px;
+ top: 44px;
@craigcook
craigcook Feb 27, 2014 Member

This fixes a long-standing bug where the little arrows were detached from the top of the menu (a 4 pixel gap, as you might have guessed). It's not necessarily related to this PR but I threw it in.

@jpetto
Member
jpetto commented Mar 10, 2014

After mulling it over, I like the idea of a separate {% page_css %} block. Keeping the responsive bundle isolated in the {% site_css %} block would mean it'll get cached by users for every page. This would add an HTTP request, but would mean page specific CSS bundles would be smaller. The added benefit is we'd avoid regularly using @import in page specific .less files, which can be a pain during development.

I'm not sure if this was the intent, but the practice for new pages would be leaving the {% site_css %} block alone (except in rare cases where the responsive bundle needs replacing) and including the page specific bundle in {% page_css %}. If that was the intent, maybe add a comment in/above {% site_css %} stating it should be left alone in most cases.

There wouldn't be an urgent need to retro-fit existing pages to fit this style, except to take advantage of caching the responsive bundle.

@sgarrity, @alexgibson - thoughts here?

I'll check IE 7 shortly - need to download the VM.

@jpetto
Member
jpetto commented Mar 10, 2014

Did a test of firefox/partners/, firefox/os/, firefox/fx/, and firefox/new/ in IE 7 & 8. Only change needed to make them all work was moving the CSS bundle from {% site_css %} to {% extrahead %}. Not bad!

We'll likely need a more complete audit, but it doesn't look like we're in for any real pain. 🙊

Did find an existing issue in IE 8 with firefox/os/. Will file a bug.

@alexgibson alexgibson and 1 other commented on an outdated diff Mar 11, 2014
bedrock/base/templates/base.html
+ {% block ga_experiments %}{% endblock %}
+ <meta name="viewport" content="width=device-width, initial-scale=1">
@alexgibson
alexgibson Mar 11, 2014 Member

I'm guessing we probably don't want the viewport meta in the non-responsive template?

@craigcook
craigcook Mar 11, 2014 Member

Correct. An overzealous copy/paste. I'll remove it (and soon we can kill the non-responsive base!)

@alexgibson alexgibson and 1 other commented on an outdated diff Mar 11, 2014
bedrock/base/templates/base.html
@@ -4,9 +4,14 @@
assets default to windows #}
<html class="windows no-js" lang="{{ LANG }}" dir="{{ DIR }}"{% block html_attrs %}{% endblock %}>
<head>
- {% block ga_experiments %}{% endblock %}
- <meta charset="utf-8">
+ <meta charset="utf-8">{# Note: Must be within first 512 bytes of page #}
+
+<!--
@alexgibson
alexgibson Mar 11, 2014 Member

Any reason why this is commented out?

@craigcook
craigcook Mar 11, 2014 Member

This is the careers teaser ascii art. It's a comment in the HTML template, but the include itself is just plain text so the same block can be inserted in different places (it's also inserted in the console).

@alexgibson
Member

After mulling it over, I like the idea of a separate {% page_css %} block. Keeping the responsive bundle isolated in the {% site_css %} block would mean it'll get cached by users for every page. This would add an HTTP request, but would mean page specific CSS bundles would be smaller. The added benefit is we'd avoid regularly using @import in page specific .less files, which can be a pain during development.

I'm not sure if this was the intent, but the practice for new pages would be leaving the {% site_css %} block alone (except in rare cases where the responsive bundle needs replacing) and including the page specific bundle in {% page_css %}. If that was the intent, maybe add a comment in/above {% site_css %} stating it should be left alone in most cases.

I like the overall approach here, it definitely feels like the simplest way forward. The prospect of caching the responsive bundle is interesting. It would be an initial extra http request, but page specific CSS weight would go down overall on a per-request basis. Like Jon says, this would also help us to avoid using @import in every file (which could help us to use things like CSS source maps for dev).

I guess this would be a bigger change overall, that might be worth some testing for things like page speed. If we agree this is worth persuing, I'm all for it 👍

@sgarrity
Contributor

Would it be fair to characterize the plan for older-browser support like this?:

  • By default, CSS is assumed to be for "modern" browsers only. After being tested, a chunk of CSS could be deemed suitable for older browsers and manually expanded to cover all browsers.
  • The site-wide base CSS site_css has already been vetted for older browsers. Other chunks of CSS can be expanded from modern-browsers-only to all/old-browsers on an ad-hoc basis.
  • For page level CSS, moving the include from includes from site_css to extrahead has the effect of expanding it from modern-only to all browsers.

I realize I'm mostly re-stating what has already been said here. Just confirming I'm understanding the plan.

@jpetto
Member
jpetto commented Mar 11, 2014

Yep, I believe that's all correct. IE's less than 8 get the oldIE bundle and CSS within the {% extrahead %} block only. All newer browsers get the {% site_css %} and {% page_css %} blocks, as well as the {% extrahead %} block.

Future CSS wouldn't worry at all about IE's older than 8, unless specifically required.

@craigcook
Member

@sgarrity : that's a pretty accurate summary. The intent is that, from now on, we won't have to worry about how oldIE handles our new CSS because oldIE won't even see it. We still have to worry about how the markup degrades with its basic styling, but that's something we should think about anyway. OldIE just gets something slightly better than no CSS at all. To restate @jpetto's breakdown:

  • site_css is delivered to modern browsers only, hidden from IE<8.
  • page_css is delivered to modern browsers only, hidden from IE<8.
  • oldIE is delivered to IE<8 only, hidden from modern browsers.
  • extrahead is delivered to all browsers, including IE<8.

We have a lot of pages that are serving page-specific styling in the extrahead block, and oldIE still gets those styles. A lot of other pages are modifying the site_css block and either re-including the sandstone styles or using super() to keep the sandstone base and add more stylesheets on after it. I ended up adding the new page_css block rather than paint us into the corner of being forced to modify site_css whenever we want to add new styles for new pages. We should stop using extrahead for page-specific styling, at least if we want to keep it away from IE.

Furthermore, going forward, we can almost always leave the site_css block alone, giving us the base global sandstone styling on every page by default, and we'll add further page-specific styles in the new page_css block (the same as using super()). It's an added request for an extra stylesheet, but it means sandstone can always be present and cached as the starting point to be further modified. This should cut down on the re-including.

This is the setup I arrived at after trying a few approaches but I can certainly be persuaded if it's the wrong way to go about it. I think it can work but I might have missed something obvious.

One snag, as noted, is that in many places we're still delivering JavaScript to IE, but no longer serving it some of the CSS that same JS depends on. Those are the pages we need to test and tweak, either making sure IE gets the styling it needs, or else withholding the JS from IE as well.

@craigcook
Member

Moved to #1808

@craigcook craigcook closed this Mar 20, 2014
@craigcook craigcook deleted the craigcook:bug-869516-base-styles-for-oldIE branch May 1, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment