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

Allow <link> in body for external resource links #616

Closed
wants to merge 1 commit into
base: master
from

Conversation

4 participants
@igrigorik
Member

igrigorik commented Feb 3, 2016

External links augment the current document and should not be restricted
to metadata content. For example, the server may want to emit a
prefetch, preload, or similar link within body content based on the
generated content (well after the head section was flushed).

Similarly, the position of a rel=stylesheet link in body can act as an
optimization and signal to the user agent that DOM content above it may
be painted - see [1]. However, note that this update does not define or
change how or when CSS is applied - that's a separate discussion.

All current browsers allow and process external resource links in body,
this update reflects what's implemented and being used by developers.

[1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=27303#c37

source Outdated
@@ -11518,6 +11518,7 @@ gave me some of the songs they wrote. I love sharing my music.&lt;/p>
<dd><span>Metadata content</span>.</dd>
<dd>If the <code data-x="attr-itemprop">itemprop</code> attribute is present: <span>flow content</span>.</dd>

This comment has been minimized.

@domenic

domenic Feb 3, 2016

Member

This should expand the two lines about itemprop to include "or the rel attribute is present and the created link is...". It should be both flow and phrasing content in those cases.

source Outdated
@@ -11518,6 +11518,7 @@ gave me some of the songs they wrote. I love sharing my music.&lt;/p>
<dd><span>Metadata content</span>.</dd>
<dd>If the <code data-x="attr-itemprop">itemprop</code> attribute is present: <span>flow content</span>.</dd>
<dd>If the <code data-x="attr-itemprop">itemprop</code> attribute is present: <span>phrasing content</span>.</dd>
<dd>If the <code data-x="attr-itemprop">rel</code> attribute is present and the created link is a <code data-x="external resource link">link to an external resource</code>: <span>flow content</span></code>.</dd>
<dt><span data-x="concept-element-contexts">Contexts in which this element can be used</span>:</dt>

This comment has been minimized.

@domenic

domenic Feb 3, 2016

Member

Similarly, the "Contexts in which this element can be used" section on itemprop also needs the extra clause.

source Outdated
@@ -11561,7 +11562,7 @@ gave me some of the songs they wrote. I love sharing my music.&lt;/p>
or an <code data-x="attr-itemprop">itemprop</code> attribute, but not both.</p>
<p class="note">If the <code data-x="attr-link-rel">rel</code> attribute is used, the element is
restricted to the <code>head</code> element. When used with the <code
restricted to the <code>head</code> element unless it is a <code data-x="external resource link">link to an external resource</code>. When used with the <code

This comment has been minimized.

@domenic

domenic Feb 3, 2016

Member

Wrap this paragraph to 100 chars

@domenic

This comment has been minimized.

Member

domenic commented Feb 3, 2016

The lists of flow and phrasing content in 3.2.4.2 need to be updated, as do the "Element content categories" section at the bottom of the spec.

@igrigorik

This comment has been minimized.

Member

igrigorik commented Feb 3, 2016

@domenic ack, updated.

@domenic

This comment has been minimized.

Member

domenic commented Feb 3, 2016

This mostly LGTM. Two questions for other editors:

  • Is "has a rel attribute" in the diff redundant? I think all external resource links might necessarily have a rel attribute, but it isn't 100% clear to me...
  • Does anyone object to merging this? It was pretty contentious back in https://www.w3.org/Bugs/Public/show_bug.cgi?id=27303, but I think that stylesheets-in-body are pretty widely used, for reasonably-good reasons, and we should allow that.
@igrigorik

This comment has been minimized.

Member

igrigorik commented Feb 3, 2016

Re, stylesheets-in-body: note that the use cases are not scoped to stylesheets only. As I noted in the original description, this is also useful for pre{load,fetch,connect, render}, etc. Also, allowing rel=stylesheet in body doesn't say anything about how it affects rendering, which is what the meat of the discussion in 27303 is all about.. one answer is that it doesn't and it blocks rendering of the entire page.

@annevk

This comment has been minimized.

Member

annevk commented Feb 4, 2016

Should we change conformance if we don't know the processing requirements yet? Am a little uneasy about that.

What does "created link" mean?

Also, what <link> elements are we trying to prevent from appearing in the <body> after this change?

@zcorpan

This comment has been minimized.

Member

zcorpan commented Feb 4, 2016

Yeah I think I agree with @annevk that we should wait with this until we've settled on the behavior.

@domenic

This comment has been minimized.

Member

domenic commented Feb 4, 2016

Do we anticipate that being done soon? I don't think we should hold up this authoring-requirement change on it. It remains the case that using link in body is a useful thing for authors today, no matter the state of the processing model in the spec.

Besides, it seems like the behavior is well-specced, even if Edge disagrees and Blink is planning to become non-interoperable too.

What does "created link" mean?

I think this refers to the spec's existing clause:

Two categories of links can be created using the link element: Links to external resources and hyperlinks. The link types section defines whether a particular link type is an external resource or a hyperlink. One link element can create multiple links (of which some might be external resource links and some might be hyperlinks); exactly which and how many links are created depends on the keywords given in the rel attribute. User agents must process the links on a per-link basis, not a per-element basis.


Also, what <link> elements are we trying to prevent from appearing in the <body> after this change?

Things like <link rel="alternate"> or <link rel="icon"> or...

@annevk

This comment has been minimized.

Member

annevk commented Feb 4, 2016

<link rel=icon> is an external resource link.

@domenic

This comment has been minimized.

Member

domenic commented Feb 4, 2016

Well, that seems kind of like a bad thing to allow in the body.... maybe we need a more nuanced definition?

@annevk

This comment has been minimized.

Member

annevk commented Feb 4, 2016

If we wanted to allow all external resource links though I think it would be better to just use that phrase directly. Something more akin to "if link has its itemprop attribute set or it is an external resource link".

@zcorpan

This comment has been minimized.

Member

zcorpan commented Feb 4, 2016

The behavior is being actively discussed in blink-dev, and if browsers end up implementing blocking parsing behavior, I think it would be good to specify that soon.

Is it really useful for authors today when it still blocks all rendering in Safari/Chrome/Opera?

@igrigorik

This comment has been minimized.

Member

igrigorik commented Feb 4, 2016

If we wanted to allow all external resource links though I think it would be better to just use that phrase directly. Something more akin to "if link has its itemprop attribute set or it is an external resource link".

Sounds reasonable to me.

The behavior is being actively discussed in blink-dev, and if browsers end up implementing blocking parsing behavior, I think it would be good to specify that soon. Is it really useful for authors today when it still blocks all rendering in Safari/Chrome/Opera?

Once again, let's not get hung up on the processing of rel=stylesheet case. For example...

As a developer I want to emit <link rel={preload, prefetch, preconnect, dns-prefetch, prerender} href=...> to improve performance of my pages: the ~static header of my content is flushed soon after the request is received by the server; following that, my server fetches data from other services/databases and generates + streams the response to the client as that data becomes available. Today, the conformance requirements for <link> do not allow me to inject any of the above directives once the header is flushed, but I don't know all the directives until I have this data.. I should be allowed to provide these directives in body content.

With regards to stylesheets, note that none of the browsers ignore rel=stylesheet in body. The difference is in whether those block painting of all or some subset of content - that's what the discussion on blink-dev and in 27303 is all about. Personally, I see these discussions as separate and non-blocking.

Things like <link rel="alternate"> or <link rel="icon"> or...

Why should these be restricted to metadata section? In practice all browsers will process them regardless of where they are in the document. As in, I get it, conceptually it is metadata and the best practice is to put it at the top, but there are valid cases where you might not know this information when generating the head content.

@domenic

This comment has been minimized.

Member

domenic commented Feb 8, 2016

With regards to stylesheets, note that none of the browsers ignore rel=stylesheet in body. The difference is in whether those block painting of all or some subset of content - that's what the discussion on blink-dev and in 27303 is all about. Personally, I see these discussions as separate and non-blocking.

I tend to agree with this. I am surprised people are wanting to block on the behavior changes Blink proposes.

Why should these be restricted to metadata section? In practice all browsers will process them regardless of where they are in the document.

Of course. But that's not what authoring conformance criteria are about. They're about best practices and making your source code easier to read for humans and getting the best experience (e.g. letting the browser show the favicon or alternate links or similar before it finishes parsing the whole document). Unlike stylesheets, there is no downside to placing these early; they don't block rendering.

As in, I get it, conceptually it is metadata and the best practice is to put it at the top, but there are valid cases where you might not know this information when generating the head content.

I can't imagine any such cases. I'd prefer we wait until we receive author complaints (as we have done with style) before changing these requirements.

@igrigorik

This comment has been minimized.

Member

igrigorik commented Feb 8, 2016

As in, I get it, conceptually it is metadata and the best practice is to put it at the top, but there are valid cases where you might not know this information when generating the head content.

I can't imagine any such cases. I'd prefer we wait until we receive author complaints (as we have done with style) before changing these requirements.

The case is simple and very common (e.g. Google search):

  • request comes in, server flushes static header
  • server constructs + streams body content as data becomes available
    • server wants to emit preload/prefetch directives based on dynamic content, but is technically not allowed per authoring requirements.

In short, we encourage streaming, and requiring that all metadata is known upfront breaks that since you have to buffer the entire response.

@domenic

This comment has been minimized.

Member

domenic commented Feb 19, 2016

@igrigorik I'm not talking about preload and prefetch, but about icon, alternate, and search, mostly. Those are metadata and should be in the head, in my opinion.

Here's my whole breakdown of what I think should be in the head vs. allowed in the body, going off the link types defined in HTML:

  • head only: alternate, author, help, icon, license, next, prev, search, sidebar (?!?!)
  • allowed in body: pingback, prefetch, stylesheet

Maybe someone can convince me that icon belongs in the second category, then the division can be that external resource links are always allowed in the body. But it seems pretty bad to delay the favicon until later. I guess I could be persuaded to simply add a document-conformance "should" for icon being in the head...


Editors, I'd like to move forward on this; it's bad form to keep a nice PR hanging in this manner. I don't think we need to block changing document conformance criteria on defining the rendering model for stylesheets; those seem like separate concerns. Can you weigh in with your feelings on my above division? Is "external resource links are allowed in the body" the right way to go, or do you also feel icon should be in the head, or do you have an entirely different set of criteria?

@domenic domenic self-assigned this Feb 19, 2016

@annevk

This comment has been minimized.

Member

annevk commented Feb 19, 2016

I think I would be fine with author/help/license/next/prev/search appearing in the body too, in the name of streaming. icon seems somewhat bad since you want that kind of information early on (a reason why I think manifests are a bad idea) and sidebar we should just remove.

@zcorpan

This comment has been minimized.

Member

zcorpan commented Feb 19, 2016

OK, let's not block this.

I think we should consider consumers also. rel=author isn't so much for browsers, but other tools. Are there tools that just look at links in head and abort processing when head is closed? It seems safer to start small and only allow in body the ones where there is demand.

As usual when content models change, double-check the various sections if they need changes (content-venn diagram, content models section, indices).

@domenic

This comment has been minimized.

Member

domenic commented Feb 19, 2016

I think we should consider consumers also. rel=author isn't so much for browsers, but other tools. Are there tools that just look at links in head and abort processing when head is closed? It seems safer to start small and only allow in body the ones where there is demand.

These are my thoughts exactly.

I think the real question is icon. I am leaning toward the solution of:

  • all external resource links are allowed in the body, but
  • the icon section has a "should" saying authors should place it in the head

This seems most consistent with the idea that authors are allowed to do anything for performance/streaming/etc. (which only external resource links impact), but also preserves our intuition that it's better to get icons early on when loading the page.

@zcorpan

This comment has been minimized.

Member

zcorpan commented Feb 19, 2016

I don't think we should allow icon in body at all. Its processing is a bit weird. You can have multiple links, and the last one applies at the time the browser wants to fetch an icon (or /favicon.ico). When is that? When head is closed is one approach that makes sense to me. Possibly we could require that explicitly, and make icons in body be ignored. (What do browsers do?)

@domenic

This comment has been minimized.

Member

domenic commented Feb 19, 2016

OK. I am comfortable with that too, although it requires more spec work. Should we create a separate piece of information about each link ("is allowed in body") that each link definition specifies? Or should we do something like "all external resource links except icon"?

@zcorpan

This comment has been minimized.

Member

zcorpan commented Feb 19, 2016

I think I prefer the former. It's a reminder to consider whether it makes sense to allow new types in body instead of having them be allowed by default.

@domenic

This comment has been minimized.

Member

domenic commented Feb 19, 2016

Sounds good to me. @igrigorik, are you up for making these modifications? If you'd rather one of us do it, that seems fine to me too, since we're the ones making this so complicated :). Just let us know.

I think the idea would be:

  • After the <link rel="author license" ...> example, add a paragraph like A link element may be <dfn>allowed in the body</dfn>, depending on its link type. By default, a link element is not allowed in the body, but certain link types override this in their definitions.
  • Change the note "If the rel attribute is used, the element is restricted to the head element." to "If the rel attribute is used, the element is only sometimes [allowed in the body]."
  • Update the things already in this PR (content model etc.) to use the "allowed in the body" concept, instead if the PR's current "rel attribute is present and the created link is a link to an external resource"
  • Go through all of the definitions for pingback, prefetch, and stylesheet, and add the following type of sentence to their first paragraph: "link elements specifying the stylesheet keyword are [allowed in the body]."
@igrigorik

This comment has been minimized.

Member

igrigorik commented Feb 20, 2016

👍 I can take a run at it early next week.

@igrigorik

This comment has been minimized.

Member

igrigorik commented Feb 22, 2016

@domenic @zcorpan updated, ptal.

source Outdated
@@ -11606,6 +11612,11 @@ gave me some of the songs they wrote. I love sharing my music.&lt;/p>
</div>
<p>A <code>link</code> element may be <dfn data-x="allowed-in-the-body">allowed in the body</dfn>,

This comment has been minimized.

@zcorpan

zcorpan Feb 23, 2016

Member

You can omit data-x and just do <dfn>allowed in the body</dfn> and <span>allowed in the body</span>.

source Outdated
@@ -11561,8 +11567,8 @@ gave me some of the songs they wrote. I love sharing my music.&lt;/p>
or an <code data-x="attr-itemprop">itemprop</code> attribute, but not both.</p>
<p class="note">If the <code data-x="attr-link-rel">rel</code> attribute is used, the element is
restricted to the <code>head</code> element. When used with the <code
data-x="attr-itemprop">itemprop</code> attribute, the element can be used both in the
only sometimes <span data-x="allowed-in-the-body">allowed in the body</code>. When used with the

This comment has been minimized.

@zcorpan

zcorpan Feb 23, 2016

Member

Wrong end tag

source Outdated
@@ -11606,6 +11612,11 @@ gave me some of the songs they wrote. I love sharing my music.&lt;/p>
</div>
<p>A <code>link</code> element may be <dfn data-x="allowed-in-the-body">allowed in the body</dfn>,
depending on its <a href="#linkTypes">link type</a>. By default, a link element is not
<span data-x="allowed-in-the-body">allowed in the body</span>, but certain link types may override

This comment has been minimized.

@zcorpan

zcorpan Feb 23, 2016

Member

Remove "may"

@zcorpan

This comment has been minimized.

Member

zcorpan commented Feb 23, 2016

Update https://html.spec.whatwg.org/multipage/dom.html#phrasing-content-2 and https://html.spec.whatwg.org/multipage/dom.html#flow-content-2

link (if the itemprop attribute is present)

->

link (if the itemprop attribute is present, or if it is allowed in the body)

(Maybe "allowed in the body" should cover the itemprop attribute, so this can just say "link (if it is allowed in the body)")

@zcorpan

This comment has been minimized.

Member

zcorpan commented Feb 23, 2016

Similarly change https://html.spec.whatwg.org/multipage/indices.html#element-content-categories (flow content and phrasing content rows)

@igrigorik

This comment has been minimized.

Member

igrigorik commented Feb 23, 2016

@zcorpan ack, updated. Hopefully we're getting closer :)

source Outdated
@@ -11606,6 +11610,10 @@ gave me some of the songs they wrote. I love sharing my music.&lt;/p>
</div>
<p>A <code>link</code> element may be <dfn>allowed in the body</dfn>, depending on its

This comment has been minimized.

@zcorpan

zcorpan Feb 24, 2016

Member

s/may/can/

@zcorpan

This comment has been minimized.

Member

zcorpan commented Feb 24, 2016

I think this looks pretty good. The wrapping is a bit off in some places, but one of us can take care of that when merging.

@zcorpan zcorpan assigned zcorpan and unassigned domenic Feb 24, 2016

@zcorpan

This comment has been minimized.

Member

zcorpan commented Feb 24, 2016

I noticed a new issue. links can have multiple keywords at once, but we don't want to allow them in body unless all the keywords are allowed in the body. I think the current prose does allow it.

<link rel="icon pingback" href="...">
source Outdated
@@ -21908,8 +21916,9 @@ interface <dfn>HTMLHyperlinkElementUtils</dfn> {
<h5>Link type "<dfn><code data-x="rel-prefetch">prefetch</code></dfn>"</h5>
<p>The <code data-x="rel-prefetch">prefetch</code> keyword may be used with <code>link</code>,
<code>a</code>, and <code>area</code> elements. This keyword creates an <span data-x="external
resource link">external resource link</span>.</p>
<code>a</code>, and <code>area</code> elements, and such elements are <span>allowed in the body

This comment has been minimized.

@zcorpan

zcorpan Feb 24, 2016

Member

s/and such/and, for <code>link</code> elements, such/

(Also don't have a line break between a word and a tag; do line breaks anywhere where there is a space instead.)

source Outdated
@@ -11516,12 +11518,15 @@ gave me some of the songs they wrote. I love sharing my music.&lt;/p>
<dl class="element">
<dt><span data-x="concept-element-categories">Categories</span>:</dt>
<dd><span>Metadata content</span>.</dd>
<dd>If the <code data-x="attr-itemprop">itemprop</code> attribute is present: <span>flow content</span>.</dd>
<dd>If the <code data-x="attr-itemprop">itemprop</code> attribute is present: <span>phrasing content</span>.</dd>
<dd>If the <code data-x="attr-itemprop">itemprop</code> attribute is present, or the link type

This comment has been minimized.

@zcorpan

zcorpan Feb 24, 2016

Member

s/link type/element/

source Outdated
data-x="attr-itemprop">itemprop</code> attribute, the element can be used both in the
<code>head</code> element and in the <code>body</code> of the page, subject to the constraints of
the microdata model.</p>
only sometimes <span>allowed in the body</span>. When used with the <code data-x="attr-itemprop">

This comment has been minimized.

@zcorpan

zcorpan Feb 24, 2016

Member

(Wrap after <code instead)

@igrigorik

This comment has been minimized.

Member

igrigorik commented Feb 24, 2016

@zcorpan ack, updated and squashed. I reworded https://github.com/whatwg/html/pull/616/files#r53937890 a bit, hope that works.

@domenic

This comment has been minimized.

Member

domenic commented Feb 24, 2016

FWIW this looks good to me (aside from the tiiiny nit that the first character of the commit message should be capitalized ;). I see @zcorpan has taken over in-depth review duties so I will leave the final say to him. Just wanted to say I'm happy with it.

Allow external links in body
External links augment the current document and should not be restricted
to metadata content. For example, the server may want to emit a
prefetch, preload, or similar link within body content based on the
generated content (well after the head section was flushed).

Similarly, the position of a rel=stylesheet link in body can act as an
optimization and signal to the user agent that DOM content above it may
be painted - see [1]. However, note that this update does not define or
change how or when CSS is applied - that's a separate discussion.

All current browsers allow and process external resource links in body,
this update reflects what's implemented and being used by developers.

[1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=27303#c37
@igrigorik

This comment has been minimized.

Member

igrigorik commented Feb 24, 2016

@domenic capitalized :)

@zcorpan

This comment has been minimized.

Member

zcorpan commented Feb 25, 2016

Changes LGTM but #616 (comment) is not addressed yet.

@igrigorik

This comment has been minimized.

Member

igrigorik commented Feb 25, 2016

One link element can create multiple links (of which some might be external resource links and some might be hyperlinks); exactly which and how many links are created depends on the keywords given in the rel attribute. User agents must process the links on a per-link basis, not a per-element basis.

This, to me, implies that we don't need additional logic for "unless all the keywords are allowed in the body". <link rel="icon pingback" href="..."> is treated as two separate links, only one of which is allowed in body.

@zcorpan

This comment has been minimized.

Member

zcorpan commented Feb 26, 2016

But the "allowed in the body" concept applies to the element. My reading is that multiple link types get ORed for "allowed in the body", not ANDed. (I can try to fix this myself in the next few days.)

@zcorpan

This comment has been minimized.

Member

zcorpan commented Mar 1, 2016

I added a new commit in #777

zcorpan added a commit that referenced this pull request Mar 3, 2016

Allow pingback/prefetch/stylesheet links in body
Some external links augment the current document and should not be
restricted to metadata content. For example, the server may want to emit
a prefetch link within body content based on the generated content (well
after the head section was flushed).

Similarly, the position of a rel=stylesheet link in body can act as an
optimization and signal to the user agent that DOM content above it may
be painted - see [1]. However, note that this update does not define or
change how or when CSS is applied - that's a separate discussion.

All current browsers allow and process external resource links in body,
this update reflects what's implemented and being used by developers.

[1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=27303#c37

PR: #616
@zcorpan

This comment has been minimized.

Member

zcorpan commented Mar 3, 2016

Thanks!! Landed as 179983e

@zcorpan zcorpan closed this Mar 3, 2016

sideshowbarker added a commit to validator/validator that referenced this pull request May 30, 2016

tripu added a commit to tripu/validator that referenced this pull request Aug 19, 2016

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