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

Add `main { display: block; }` #87

Merged
merged 1 commit into from Feb 11, 2013

Conversation

Projects
None yet
6 participants
@mathiasbynens
Contributor

mathiasbynens commented Feb 1, 2013

…because that’s how browsers implement it.

Note: after pulling this in you will need to run the build script (I didn’t do that yet).

@mathiasbynens

This comment has been minimized.

Show comment
Hide comment
@mathiasbynens

mathiasbynens Feb 1, 2013

Contributor

@rem, could you update http://html5shiv.googlecode.com/svn/trunk/html5.js once this gets pulled in? (Or just add <main> to the list of elements + to the CSS rule manually.)

Contributor

mathiasbynens commented Feb 1, 2013

@rem, could you update http://html5shiv.googlecode.com/svn/trunk/html5.js once this gets pulled in? (Or just add <main> to the list of elements + to the CSS rule manually.)

@remy

This comment has been minimized.

Show comment
Hide comment
@remy

remy Feb 1, 2013

Collaborator

Just a note to say I've compiled @mathisbynens' version and released up to googlecode hosted versions.

Collaborator

remy commented Feb 1, 2013

Just a note to say I've compiled @mathisbynens' version and released up to googlecode hosted versions.

@mathiasbynens

This comment has been minimized.

Show comment
Hide comment
@mathiasbynens

mathiasbynens Feb 1, 2013

Contributor

Awesome, thanks @rem!

Contributor

mathiasbynens commented Feb 1, 2013

Awesome, thanks @rem!

@aFarkas

This comment has been minimized.

Show comment
Hide comment
@aFarkas

aFarkas Feb 1, 2013

Owner

@mathiasbynens

My problem with this is that, we have a simple test for HTML5 style support. Only if this isn't supported the changed code kicks in. Which means, that your change does not work in current Chrome/Safari/Firefox/Opera/IE. Only on old browsers.

See also #86. As said there, it's not really decided, whether we want to add this style. It's really best to do this in your stylesheet.

If we want to add this style, we have to change the test above to something like this

Note: I will change the build script to grunt.js soon.

Owner

aFarkas commented Feb 1, 2013

@mathiasbynens

My problem with this is that, we have a simple test for HTML5 style support. Only if this isn't supported the changed code kicks in. Which means, that your change does not work in current Chrome/Safari/Firefox/Opera/IE. Only on old browsers.

See also #86. As said there, it's not really decided, whether we want to add this style. It's really best to do this in your stylesheet.

If we want to add this style, we have to change the test above to something like this

Note: I will change the build script to grunt.js soon.

@aFarkas

This comment has been minimized.

Show comment
Hide comment
@aFarkas

aFarkas Feb 4, 2013

Owner

@mathiasbynens @jonathantneal and @paulirish

any thoughts about this?

Owner

aFarkas commented Feb 4, 2013

@mathiasbynens @jonathantneal and @paulirish

any thoughts about this?

@paulirish

This comment has been minimized.

Show comment
Hide comment
@paulirish

paulirish Feb 4, 2013

Collaborator

The revised style detect is pretty hefty and forces a layout, while the current sniff does not. I want to avoid the former if we can.

Can we look for window.HTMLMainElement and add the styles based on that?

Collaborator

paulirish commented Feb 4, 2013

The revised style detect is pretty hefty and forces a layout, while the current sniff does not. I want to avoid the former if we can.

Can we look for window.HTMLMainElement and add the styles based on that?

@jonathantneal

This comment has been minimized.

Show comment
Hide comment
@jonathantneal

jonathantneal Feb 4, 2013

Collaborator

To what Paul said about layout, appending a stylesheet to the document or documentElement will have an adverse effect in IE, resulting in some extra space at the top of the document. However, that is secondary to the issue at hand - whether or not we had main to the polyfilled styles. I think we should add it to the styles as display block.

Regarding sniffs, since main throws a lot of newer browsers into a polyfill-warranted status, we should reevaluate how and when we throw those styles in. I like it when we guarantee that our polyfilling stylesheet comes first, so that it can be easily overwritten by all other CSS. When we do this, we're effectively mocking the ua stylesheet, and so then we can be more liberal in our sniffing.

Collaborator

jonathantneal commented Feb 4, 2013

To what Paul said about layout, appending a stylesheet to the document or documentElement will have an adverse effect in IE, resulting in some extra space at the top of the document. However, that is secondary to the issue at hand - whether or not we had main to the polyfilled styles. I think we should add it to the styles as display block.

Regarding sniffs, since main throws a lot of newer browsers into a polyfill-warranted status, we should reevaluate how and when we throw those styles in. I like it when we guarantee that our polyfilling stylesheet comes first, so that it can be easily overwritten by all other CSS. When we do this, we're effectively mocking the ua stylesheet, and so then we can be more liberal in our sniffing.

aFarkas added a commit that referenced this pull request Feb 11, 2013

Merge pull request #87 from mathiasbynens/master
Add `main { display: block; }`

@aFarkas aFarkas merged commit 4c0857d into aFarkas:master Feb 11, 2013

@aFarkas

This comment has been minimized.

Show comment
Hide comment
@aFarkas

aFarkas Feb 13, 2013

Owner

The way @jonathantneal outlines is the only way, I can currently think of.

I think, as long document.body is null adding our stylesheet won't affect performance in most browsers. We could change our test to something "stupid" like this:

supportsHtml5Styles = !(!document.body || !('hidden' in a));

About the extra space in IE / html5shiv styles comes first.
I tryed to test the IE space bug (in IE7/8 mode of IE9), but couldn't reproduce it. Can you give me a simple testcase?

From code and my testing our html shiv stlyes always comes first. Is there something special, I miss?

Owner

aFarkas commented Feb 13, 2013

The way @jonathantneal outlines is the only way, I can currently think of.

I think, as long document.body is null adding our stylesheet won't affect performance in most browsers. We could change our test to something "stupid" like this:

supportsHtml5Styles = !(!document.body || !('hidden' in a));

About the extra space in IE / html5shiv styles comes first.
I tryed to test the IE space bug (in IE7/8 mode of IE9), but couldn't reproduce it. Can you give me a simple testcase?

From code and my testing our html shiv stlyes always comes first. Is there something special, I miss?

@aFarkas aFarkas referenced this pull request Feb 13, 2013

Closed

3.6.2pre tag #88

@jonathantneal

This comment has been minimized.

Show comment
Hide comment
@jonathantneal

jonathantneal Feb 13, 2013

Collaborator

We will probably have to be liberal in our sniffing.

I tried sniffing this.HTML( Main | Section | Etc )Element, but it's a mess. Several HTML4 and HTML5 elements are missing. There is no pattern. When I asked the spec writers about this, they indicated that these global constructors have always been inconsistent and otherwise reactionary. That ended that road.

Collaborator

jonathantneal commented Feb 13, 2013

We will probably have to be liberal in our sniffing.

I tried sniffing this.HTML( Main | Section | Etc )Element, but it's a mess. Several HTML4 and HTML5 elements are missing. There is no pattern. When I asked the spec writers about this, they indicated that these global constructors have always been inconsistent and otherwise reactionary. That ended that road.

@aFarkas

This comment has been minimized.

Show comment
Hide comment
@aFarkas

aFarkas Feb 14, 2013

Owner

@jonathantneal

Yeah, I know. Especially the main element has no additional properties to a normal element (-> HTMLElement). I don't know any browser, which implements a constructor for those kind of elements.

Therefore, we have only two options here:

  1. Do the "expensive" display block test. (But don't know how expesnive it really is)
  2. Make the styles less obtrusive or better non obtrusiv and add styles in all browsers (As I said, as long as there are no elements to style loaded, we won't create a bigger performance issue.

About the obtrusiveness. I can't reproduce the two problems you mentioned (testcases/information how to reproduce are very welcome.). And beside the main discussion here, I would like to have styles as less obtrusive as possible.

Owner

aFarkas commented Feb 14, 2013

@jonathantneal

Yeah, I know. Especially the main element has no additional properties to a normal element (-> HTMLElement). I don't know any browser, which implements a constructor for those kind of elements.

Therefore, we have only two options here:

  1. Do the "expensive" display block test. (But don't know how expesnive it really is)
  2. Make the styles less obtrusive or better non obtrusiv and add styles in all browsers (As I said, as long as there are no elements to style loaded, we won't create a bigger performance issue.

About the obtrusiveness. I can't reproduce the two problems you mentioned (testcases/information how to reproduce are very welcome.). And beside the main discussion here, I would like to have styles as less obtrusive as possible.

@jonathantneal

This comment has been minimized.

Show comment
Hide comment
@jonathantneal

jonathantneal Feb 14, 2013

Collaborator

Right now, I use something like this.

(function (element, head) {
    element.innerHTML = ".<style>"+
        "article,aside,details,figcaption,figure,footer,header,hgroup,main,nav,section,subline,summary{display:block}"+
        "audio,canvas,video{display:inline-block}"+
        "audio:not([controls]){display:none;height:0}"+
        "mark{background:#FF0;color:#000}"+
        "[hidden]{display:none}"+
    "</style>";

    head.insertBefore(element.lastChild, head.firstChild);
})(document.createElement("p"), document.getElementsByTagName("head")[0] || document.documentElement);
Collaborator

jonathantneal commented Feb 14, 2013

Right now, I use something like this.

(function (element, head) {
    element.innerHTML = ".<style>"+
        "article,aside,details,figcaption,figure,footer,header,hgroup,main,nav,section,subline,summary{display:block}"+
        "audio,canvas,video{display:inline-block}"+
        "audio:not([controls]){display:none;height:0}"+
        "mark{background:#FF0;color:#000}"+
        "[hidden]{display:none}"+
    "</style>";

    head.insertBefore(element.lastChild, head.firstChild);
})(document.createElement("p"), document.getElementsByTagName("head")[0] || document.documentElement);
@laukstein

This comment has been minimized.

Show comment
Hide comment
@laukstein

laukstein Jul 15, 2013

It works now on all recent browsers except IE, even IE10. So what about conditional comment <!--[if lt IE 9]>...<![endif]--> since we need it also for IE10?
Of course the best would be if everybody would insert main { display: block; } manually.

laukstein commented Jul 15, 2013

It works now on all recent browsers except IE, even IE10. So what about conditional comment <!--[if lt IE 9]>...<![endif]--> since we need it also for IE10?
Of course the best would be if everybody would insert main { display: block; } manually.

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