-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 conformance requirements and example for imagesizes/imagesrcset #5606
Conversation
Closes #5604. This also fixes nearby double-negatives for other link attributes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you so much for making this update @domenic 👍 One question around the use of the picture
element. Other than that, this is a great update.
source
Outdated
<!-- ... later, or perhaps inserted dynamically ... --> | ||
<img src="wolf.jpg" alt="A rad wolf" | ||
srcset="wolf_400px.jpg 400w, wolf_800px.jpg 800w, wolf_1600px.jpg 1600w" | ||
sizes="50vw"></code></pre> | ||
</div> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could this also be used with the picture
element's source
? If so, then the wording above needs to change a bit to indicate that it can be used in conjunction with the img
and picture
elements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. @yoavweiss @domfarolino can you confirm that imagesrcset/imagesizes works for picture source elements too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They do. An example:
<link rel=preload as=image imagesrcset="cropped_400.jpg 400w, cropped_800.jpg 800w ..." media="
(max-width: 800)">
<link rel=preload as=image imagesrcset="wide_400.jpg 400w, wide_800.jpg 800w ..." media="(min-width: 801)">
<picture>
<source srcset="cropped..." media="(max-width: 800px)">
<img srcset="wide...">
</picture>
They could similarly be used for format selection, but in this case, only 1 preload would make sense (probably with the most likely-to-be supported format)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very interesting. I notice your example does not use the href=""
attribute. Should we update the conformance requirements to allow omitting href=""
?? That'd be a fairly big change, but it appears that the processing model does allow for it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After working through your example below, I've pushed a commit that update's link's conformance requirements to allow either imagesrcset or href. I also added an example similar to yours (but using density instead of width).
Please take a look; this change has gotten a good deal bigger in scope, and will be a great improvement if I managed to line everything up.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removal of double negative is nice!
present. The exception is if the <code data-x="attr-link-imagesrcset">imagesrcset</code> attribute | ||
is present; in that case the <code data-x="attr-link-href">href</code> attribute may be omitted. | ||
If the <code data-x="attr-link-href">href</code> attribute is present, then its value must be a | ||
<span>valid non-empty URL potentially surrounded by spaces</span>.</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we reword this to require either the href or imagesrcset attribute and perhaps not talk about the destination of the link at all? At least from a quick search we mostly use destination to mean a request's destination field and we don't seem to refer to this concept anywhere (it's also not a dfn
).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should still have a definition for the href
attribute... although I guess a/anchor do not really have one: https://html.spec.whatwg.org/#attr-hyperlink-href. The non-normative attribute description for a is the "address of the hyperlink". Hmm... I'll just change it to s/destination/address for now?
<td> <span>Valid source size list</span> | ||
<tr> | ||
<th> <code data-x="">imagesrcset</code> | ||
<td> <code data-x="attr-link-imagesrcset">link</code> | ||
<td> Images to use in different situations (e.g., high-resolution displays, small monitors, etc.) | ||
<td> Images to use in different situations (e.g., high-resolution displays, small monitors, etc.) (for <code data-x="attr-link-rel">rel</code>="<code data-x="rel-preload">preload</code>") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<td> Images to use in different situations (e.g., high-resolution displays, small monitors, etc.) (for <code data-x="attr-link-rel">rel</code>="<code data-x="rel-preload">preload</code>") | |
<td> Images to use in different situations (e.g., high-resolution displays and small monitors; for <code data-x="attr-link-rel">rel</code>="<code data-x="rel-preload">preload</code>") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did something slightly different here; let me know what you think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of the problem here (and I guess below) is having both e.g. and etc. so I think we still need to make that change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the problem with having both e.g. and etc.?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Examples are by definition a limited set. So making it an unlimited set again with etc. is weird at best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems fine to note that there are more examples beyond the two given here. Note that this is a preexisting pattern in the HTML spec...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm OK changing this but I would prefer not to, since it makes sense to me as-is and is a preexisting pattern in the spec.
<td> <span>Valid source size list</span> | ||
<tr> | ||
<th> <code data-x="">imagesrcset</code> | ||
<td> <code data-x="attr-link-imagesrcset">link</code> | ||
<td> Images to use in different situations (e.g., high-resolution displays, small monitors, etc.) | ||
<td> Images to use in different situations (e.g., high-resolution displays, small monitors, etc.) (for <code data-x="attr-link-rel">rel</code>="<code data-x="rel-preload">preload</code>") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Part of the problem here (and I guess below) is having both e.g. and etc. so I think we still need to make that change.
@@ -13043,7 +13043,7 @@ interface <dfn>HTMLLinkElement</dfn> : <span>HTMLElement</span> { | |||
|
|||
<p>The <code>link</code> element allows authors to link their document to other resources.</p> | |||
|
|||
<p>The destination of the link(s) is given by the <dfn><code | |||
<p>The address of the link(s) is given by the <dfn><code |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does it mean for a link element to have an address though? And what does it mean to lack it? If nothing is tied to this, I'm not sure why we say it. And it would be much clearer conformance-wise to say that a link element must have either an href or imagesrcset attribute specified.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We say it because IMO it's important to define what href
means. If the only "definition" of href
is that it or imagesizes
must occur, that doesn't give the reader any idea what href
is, which is bad.
We don't always define attributes, it's true. But I think we should.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe then "The URLs of the other resources are given by the href and imagesrcset attributes. Either the href or imagesrcset attribute must be specified." (This ties it back to the definition of the link element.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds like a reasonable direction. Although what do you mean by "other" resources? Can it just be "the URL of the resource linked"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, since the contrast with document is gone "other" isn't needed. You do need to allow for plural though because of srcset.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sigh, OK, I tried this and I don't like it for two reasons:
-
This section defines
href
, whereas the definition ofimagesrcset
and its conformance requirements is down below. So keeping this paragraphhref
focused, as I've done, works better in that context. We could try to move the definition and conformance requirements forimagesrcset
up and merge them into this paragraph, but then it's disconnected fromimagesizes
, which causes its own problems. -
You may want to specify both
href
andimagesrcset
, so the wording in the PR is more correct than "Either thehref
or theimagesrcset
attribute must be specified".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So should we then repeat this definition for imagesrcset below? It currently doesn't seem to have a definition. If you say href or imagesrcset must be specified it works per https://infra.spec.whatwg.org/#terminology.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The definition below is that it's a srcset attribute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that this is a bit inconsistent; defining something as "a srcset attribute" isn't as helpful as defining it as "the address of the link". (Because "a srcset attribute" links to a section that talks entirely about constraints on the attribute's value, instead of talking about what a srcset attribute means or represents.) However, I think this is a reasonable direction to go for now, in terms of making minimal changes.
I'll push a commit on using "or" instead of the current more long-winded phrasing.
@annevk ping on picking this back up, and letting me know how strongly you feel about the remaining points. |
Closes #5604.
This also fixes nearby double-negatives for other link attributes.
/cc @schalkneethling @whatwg/documentation @sideshowbarker
/indices.html ( diff )
/semantics.html ( diff )