8000 Allow <link> in body for external resource links by igrigorik · Pull Request #616 · whatwg/html · GitHub
[go: up one dir, main page]

Skip to content

Conversation

igrigorik
Copy link
Member

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
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@domenic
Copy link
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
Copy link
Member Author

@domenic ack, updated.

8000
@domenic
Copy link
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
Copy link
Member Author

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
Copy link
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
Copy link
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
Copy link
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
Copy link
Member
annevk commented Feb 4, 2016

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

@domenic
Copy link
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
Copy link
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
Copy link
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
Copy link
Member Author

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
Copy link
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
8000 Copy link
Member Author

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
Copy link
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?

@annevk
Copy link
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
Copy link
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
Copy link
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
Copy link
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
Copy link
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
Copy link
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
Copy link
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
Copy link
Member Author

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

@zcorpan
Copy link
8000
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
Copy link
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
Copy link
Member Author

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

source Outdated

</div>

<p>A <code>link</code> element may be <dfn>allowed in the body</dfn>, depending on its
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

s/may/can/

@zcorpan
Copy link
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
Copy link
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
<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
< 8000 /span> Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

@igrigorik
Copy link
Member Author

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

@domenic
Copy link
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.

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
Copy link
Member Author

@domenic capitalized :)

@zcorpan
Copy link
Member
zcorpan commented Feb 25, 2016

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

@igrigorik
Copy link
Member Author

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
Copy link
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
Copy link
Member
zcorpan commented Mar 1, 2016

I added a new commit in #777

zcorpan pushed a commit that referenced this pull request Mar 3, 2016
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
Copy link
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 pushed 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

Development

Successfully merging this pull request may close these issues.

4 participants

0