Skip to content
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 the "as" attribute for the <link> element #1449

Merged
merged 13 commits into from
Jun 30, 2016
Merged

Add the "as" attribute for the <link> element #1449

merged 13 commits into from
Jun 30, 2016

Conversation

sideshowbarker
Copy link
Contributor

No description provided.

@sideshowbarker
Copy link
Contributor Author

This at least needs technical review from @yoavweiss before merging.

<th> <code data-x="">as</code>
<td> <code data-x="attr-link-as">as</code>
<td> <span data-x="concept-request-destination">Destination</span> for a preloaded response (for <code data-x="attr-link-rel">rel</code>="<code data-x="rel-preload">preload</code>")
<td> <span data-x="concept-request-destination">destination</span>
Copy link
Member

Choose a reason for hiding this comment

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

I think this column is initial-caps. Maybe "Request destination"?

@domenic domenic added the addition/proposal New features or enhancements label Jun 21, 2016
@@ -116118,7 +116119,7 @@ interface <dfn>External</dfn> {
<tr>
<th> <code data-x="">as</code>
<td> <code data-x="attr-link-as">as</code>
<td> <span data-x="concept-request-destination">Destination</span> for a preloaded response (for <code data-x="attr-link-rel">rel</code>="<code data-x="rel-preload">preload</code>")
<td> <span data-x="concept-request-destination">Request destination</span> for a preloaded response (for <code data-x="attr-link-rel">rel</code>="<code data-x="rel-preload">preload</code>")
Copy link
Member

Choose a reason for hiding this comment

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

I meant the third column :P

Copy link
Member

Choose a reason for hiding this comment

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

Err, last column.

@yoavweiss
Copy link
Contributor

Also, thanks @sideshowbarker for your work on this! :)

@sideshowbarker
Copy link
Contributor Author

Should it be an enumerated attribute?

@domenic @yoavweiss @igrigorik please see b11ef97 and lemme know if I got it right or not, as far as what we need specified here

@@ -12439,12 +12439,27 @@ interface <dfn>HTMLBaseElement</dfn> : <span>HTMLElement</span> {
<dd>Also, the <code data-x="attr-link-title">title</code> attribute <span data-x="attr-link-title">has special semantics</span> on this element.</dd>
<dt><span data-x="concept-element-dom">DOM interface</span>:</dt><!--TOPIC:DOM APIs-->
<dd>
<pre class="idl">[<span>HTMLConstructor</span>]
<pre class="idl">enum <dfn data-x="dom-Destination">Destination</dfn> {<!--
Copy link
Member

Choose a reason for hiding this comment

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

HTML enumerated attributes typically don't use IDL enum, but are DOMString.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HTML enumerated attributes typically don't use IDL enum, but are DOMString.

OK but please see @domenic’s comments at #1449 (comment) and #1449 (comment) and see also, e.g., https://html.spec.whatwg.org/multipage/embedded-content.html#texttrackkind

Copy link
Member

Choose a reason for hiding this comment

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

https://html.spec.whatwg.org/multipage/embedded-content.html#htmltrackelement is the track element's interface and kind there is a DOMString, "limited to only known values". The TextTrack interface's kind does not use "limited to only known values".

https://html.spec.whatwg.org/multipage/infrastructure.html#limited-to-only-known-values is only defined for DOMString types.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HTML enumerated attributes typically don't use IDL enum, but are DOMString.

https://html.spec.whatwg.org/multipage/infrastructure.html#limited-to-only-known-values is only defined for DOMString types.

So I should drop the enum from the IDL and revert to the as IDL attribute just being DOMString.

If so what should I add/change in the IDL to address @domenic’s #1449 (comment) and #1449 (comment) comments?

Copy link
Member

Choose a reason for hiding this comment

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

Yes.

No need to change anything else in the IDL; DOMString + limited to only known values gives us the feature-detection we need, same as input type etc.

Though attributes that are "enumerated attributes" have prose saying how each keyword maps to a state.

I can try to fix this myself with a new commit. :-)

Copy link
Member

Choose a reason for hiding this comment

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

This is another level of indirection. :-)

f6c53bc

Copy link
Member

Choose a reason for hiding this comment

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

@annevk says he doesn't like case-insensitive. I suppose it would be possible to use IDL enum instead.

Any others have opinions about the case-sensitivity of this attribute?

Copy link
Member

Choose a reason for hiding this comment

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

Having HTML duplicate the list of fetch destinations is sucky. Can we have Fetch define this enum? I'm surprised it doesn't already; it seems strange you can set the destiniation only with <link rel="preload"> and not using the fetch API.

Copy link
Member

Choose a reason for hiding this comment

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

Oh hey, Fetch does define the enum for us! https://fetch.spec.whatwg.org/#requestdestination Just use that!!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh hey, Fetch does define the enum for us! https://fetch.spec.whatwg.org/#requestdestination Just use that!!

Thanks for catching that. See 772f6ba. I’m very happy RequestDestination is defined in Fetch for us but sad that I didn’t read the IDL in the Fetch spec carefully enough myself to realize it already had it…

@zcorpan
Copy link
Member

zcorpan commented Jun 27, 2016

Pls see #1449 (comment) (this comment thread is not-so-usefully hidden from view...)

@annevk
Copy link
Member

annevk commented Jun 27, 2016

FWIW, I don't know if it currently is, but I made the case on IRC that this should be case-sensitive. Otherwise we limit the namespace of a value defined by other standards. I think generally we should just go for case-sensitive where we can. HTML has been inconsistent on this thus far, case-insensitive only really makes sense for ASCII, and case-sensitive is just simpler to test and implement.

@zcorpan zcorpan added the do not merge yet Pull request must not be merged per rationale in comment label Jun 27, 2016
@zcorpan
Copy link
Member

zcorpan commented Jun 28, 2016

OK in 143fc37 I changed to IDL enumeration which makes it case-sensitive.

@sideshowbarker
Copy link
Contributor Author

OK in 143fc37 I changed to IDL enumeration which makes it case-sensitive.

LGTM

@zcorpan zcorpan removed the do not merge yet Pull request must not be merged per rationale in comment label Jun 28, 2016
@zcorpan
Copy link
Member

zcorpan commented Jun 28, 2016

Hmm @domenic's comment

Should add a conformance requirement that it be a valid destination.

is not yet addressed.

@zcorpan zcorpan added the do not merge yet Pull request must not be merged per rationale in comment label Jun 28, 2016
@domenic
Copy link
Member

domenic commented Jun 28, 2016

It's missing from the element index list of attributes

@@ -12491,6 +12517,21 @@ interface <dfn>HTMLLinkElement</dfn> : <span>HTMLElement</span> {
tokens</span> must only include the tokens from this list that the user agent implements the
processing model for.</p>

<p>The <dfn><code data-x="attr-link-as">as</code></dfn> attribute specifies the <span
data-x="concept-request-destination">destination</span> for a preloaded response from the resource
Copy link
Member

Choose a reason for hiding this comment

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

s/preloaded response/preload request

<th> <code data-x="">as</code>
<td> <code data-x="attr-link-as">link</code>
<td> <span data-x="concept-request-destination">Destination</span> for a preloaded response (for <code data-x="attr-link-rel">rel</code>="<code data-x="rel-preload">preload</code>")
<td> "<code data-x="">document</code>"; "<code data-x="">embed</code>"; "<code data-x="">font</code>"; "<code data-x="">image</code>"; "<code data-x="">manifest</code>"; "<code data-x="">media</code>"; "<code data-x="">object</code>"; "<code data-x="">report</code>"; "<code data-x="">script</code>"; "<code data-x="">serviceworker</code>"; "<code data-x="">sharedworker</code>"; "<code data-x="">style</code>"; "<code data-x="">worker"</code>; "<code data-x="">xslt</code>"; or the empty string
Copy link
Member

Choose a reason for hiding this comment

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

How about just Destination?

@@ -12491,6 +12517,21 @@ interface <dfn>HTMLLinkElement</dfn> : <span>HTMLElement</span> {
tokens</span> must only include the tokens from this list that the user agent implements the
processing model for.</p>

<p>The <dfn><code data-x="attr-link-as">as</code></dfn> attribute specifies the <span
data-x="concept-request-destination">destination</span> for a preloaded response from the resource
given by the <code data-x="attr-link-href">href</code> attribute. The possible values are the
Copy link
Member

Choose a reason for hiding this comment

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

Again, duplicating the values is not good IMO. "The possible values are any destination" makes sense to me.

@sideshowbarker
Copy link
Contributor Author

It's missing from the element index list of attributes

Added in f4576b1

@sideshowbarker
Copy link
Contributor Author

Hmm @domenic's comment

Should add a conformance requirement that it be a valid destination.

is not yet addressed.

...

Again, duplicating the values is not good IMO. "The possible values are any destination" makes sense to me.

I think for authors it’s ideally better for the allowed values to be explicitly listed at point of use in the HTML spec itself, rather than requiring them to also check another spec just to determine what the document-conformance requirements are (especially in the case like this where the microsyntax of the attribute is really simple—just one keyword among an enumerated list of keywords).

But that said, duplicating values is also not good and should be avoided.

So I’m fine personally in this case with just going back to having this change to the HTML spec state that the value must be a valid destination, with a link to the definition in the Preload spec. But would like to wait to find out if @zcorpan agrees.

@zcorpan
Copy link
Member

zcorpan commented Jun 29, 2016

I suppose the normative part of the spec should link to fetch, and the index can do that also (input type in the index already has a link instead of enumerating all the values), but it would be nice with some non-normative text inline saying what they keywords are.

@sideshowbarker
Copy link
Contributor Author

LGTM after daa16c6

I suppose the normative part of the spec should link to fetch, and the index can do that also (input type in the index already has a link instead of enumerating all the values)

Yup, as I see you made it.

but it would be nice with some non-normative text inline saying what they keywords are.

Yup, just putting it into a Note as you made it seems sufficient.

@zcorpan zcorpan removed the do not merge yet Pull request must not be merged per rationale in comment label Jun 29, 2016
@domenic
Copy link
Member

domenic commented Jun 29, 2016

I'm really not a fan of any duplication. It just means more maintenance whenever Fetch updates. This has already been a pain for referrerpolicy that is causing HTML and Referrer Policy to get out of sync (see #1419). I think it's OK to refer authors to another spec to see the valid values.

if the corresponding content attribute's value <span
data-x="case-sensitive">case-sensitively</span> matches one of the enumerated values, then the IDL
attribute must return the content attribute's value; otherwise it must return the content
attribute's default value; and on setting, the content attribute must be set to the specified new
Copy link
Member

Choose a reason for hiding this comment

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

Maybe s/; and on/. On/

@domenic domenic merged commit 2381476 into master Jun 30, 2016
@domenic domenic deleted the as-attribute branch June 30, 2016 19:20
@domenic
Copy link
Member

domenic commented Jun 30, 2016

Lovely work everyone! Thanks @yoavweiss for being willing to integrate like this. I think authors and tools really benefit from having all the relevant attributes of an element in one place.

@yoavweiss
Copy link
Contributor

And thanks @sideshowbarker and @zcorpan for actually working on this while I was traveling! :D

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
addition/proposal New features or enhancements topic: link
Development

Successfully merging this pull request may close these issues.

6 participants