Closed tabatkins closed 1 week ago
Doesn't seem to diminish what you can do with it. If that increases the chances of it happening, I'm all for it.
Yeah, it should be absolutely no loss in correct functionality; the only loss is in catching some authoring errors earlier.
You actually gain some functionality via the new tokens
type, letting you, for example, provide shadows or transforms in an attribute.
Sounds reasonable to me and would probably better match author expectations, thus improve the overall experience.
By the way, attr()
currently does not support some of the more recent type additions like <image>
, <position>
and <resolution>
. Iʼm not sure whether this is intentional.
Besides being able to parse as a complete <color>
, attributes could also hold color components separately. Therefore, attr()
would be useful inside functional color notation. Itʼs quite likely there (but also elsewhere), that an implied unit would be used, such that an attribute value of 1
might represent 1
, 1.0
, 100%
(as in <alpha-value>
and <cmyk-component>
), 1%
or 1deg
(as in <hue>
). To make this easier to handle, I think this part:
%
, A keyword matching one of the<length>
,<angle>
,<time>
, or<frequency>
units
should be changed to use a familiar calc()
-like expression instead.
attr(foo px)
and calc(attr(foo number) * 1px)
would become attr(foo * 1px)
. attr(foo %)
and calc(attr(foo number) * 1%)
would become attr(foo * 1%)
. calc(attr(foo %) * 100)
and calc(attr(foo number) * 100%)
would become attr(foo * 100%)
. There is some discussion at the end of https://bugzilla.mozilla.org/show_bug.cgi?id=435426, where I suggested something similar, though I wasn't smart enough to envision the calc(attr(foo number) * 1px)
hack that allows you to actually not loose functionality :)
I see no value in allowing image or position - I think you're opening a dangerous door once you allow structures like <div data-foo="linear-gradient(red, blue)"/>
. Although adding resolution seems harmless enough.
For what it's worth we've implemented attr() as specified, although we're not yet shipping so that shouldn't factor into amy decision to change the definition. Yes, the type checking on parse seemed an unnecessary step, as the value is going to be validated again anywhere when you evaluate the property. I also agree the type is required - the most common use for attr() from where I sit is attr(href uri)
, which only works if there is a type. And the distinction between "length" and "px" is also valuable.
@Crissov - as we read it, attr
can already be used anywhere, including to represent color components - these are already valid expressions:
color: rgb(attr(red number), attr(green number), attr(blue number));
width: calc(50px * attr(multiplier number, 4));
But again, I don't think think there's any value in changing the attr()
syntax to be some sort of calc-lite: You're not adding any new functionality with this suggestion, and the existing syntax is not impractical. Change for change's sake should be strongly discouraged.
However, as we're looking at attr I'd like to suggest removing the restriction on the fallback value not being an attr()
expression itself. This restriction no longer makes sense if the values are being evaluated later, as they are for var()
Yeah, I agree with the above regarding the simplicity, not so sure about the attr
-in-attr
thing, that means you need to do cycle detection on that too, and that's one of the most expensive parts of var()
.
For what is worth, the other potentially annoying thing from a browser developer perspective is the opportunity for XSS. We right now sanitize away "unsafe" attributes / elements like xlink:href
in <use>
elements or what not. attr()
opens the door to make any attribute potentially an image load, and thus which attributes should you sanitize becomes either "all of them", or starts depending on the CSS rules that apply on the page, or what not. I wonder if @hsivonen or @freddyb have thoughts about this kind of thing. Though IIRC the only thing we used to sanitize from style
attributes and stylesheets is -moz-binding
, so maybe image loads and such are not a problem at all and I'm just being paranoid.
Being able to turn any attribute into an image load URL would indeed bypass the URL attribute sanitizer in Gecko.
Does Gecko currently support using attr()
to take a URL from any attribute as e.g. a background image?
Loading an image this way doesn't seem to work in Firefox, Chrome or Safari. But it seems to be because the attr() function isn't parsing.
Does Gecko currently support using attr() to take a URL from any attribute as e.g. a background image?
We currently don't support it, but implementing this feature would allow it.
Do we have valid use cases of attr
-in-attr
?
In my intuition, attr()
is designed for passing extra input to CSS. So IMO it's an incorrect usage to have complicated computation inside attr
in attr
-- if such kind of computation is needed, it seems more natural to be done either in Javascript or with custom properties.
Anyway, it would be a great news for me if I don't need to implement that 😃
Any attribute that overrides or partially overrides another is a potential use case.
attr(title, attr(pattern))
to explain <input>
constraintsattr(download, attr(href))
to represent hyperlinksattr(data-selection, attr(data-default))
to consume hierarchical custom data attributes@gibson042 The use cases you gave are totally valid, and in general, using attr()
in fallback value doesn't seem to introduce cyclic dependency by itself.
And I was thinking of a different thing though. Sorry for the confusion.
What if the referenced attribute itself contains attr()
? For example:
<div id=target foo="attr(bar *)" bar="attr(foo *)"></div>
#target { --whatever-property: attr(foo *); }
Or a detour via var()
:
<div id=target foo="var(--prop-a)" bar="var(--prop-b)"></div>
#target {
--prop-a: attr(bar *);
--prop-b: attr(foo *);
}
So do we have valid use cases like that?
If not, we might want to consider making attr(foo)
ivact if attribute foo
itself contains attr()
(and possibly var()
?)
I'm generally in favor of the proposal modulo the concerns that Emilio and Henri raised.
I don't know if we have valid use cases for variable references inside attr()
-referenced attributes, or vice versa. But if we model attributes as another kind of variable, then we should be able to use the same iacvt processing when encountering cycles.
Is there any reason not to make all attr()
values valid at parse time?
I do find it a little odd that the second argument to attr()
names CSS syntax types and other operations like dimension value construction, and how e.g. string
means take the literal string content and turn it into a <string>
token, while number
means to parse the attribute value as a <number>
token. I'm half wondering whether it makes sense to align with the syntax used for custom property registration for things like the latter.
The CSS Working Group just discussed Switch advanced attr() to being var()-like
.
I don't know if we have valid use cases for variable references inside
attr()
-referenced attributes, or vice versa. But if we model attributes as another kind of variable, then we should be able to use the same iacvt processing when encountering cycles.
This makes me wonder whether there would be extra complexity to handle cycle involving both attr
and var
. Maybe not a difficulty spec-wise, but the implementation may need some refactor.
Being able to turn any attribute into an image load URL would indeed bypass the URL attribute sanitizer in Gecko.
How does the sanitizer work against CSS variable? Or it doesn't matter?
This makes me wonder whether there would be extra complexity to handle cycle involving both attr and var. Maybe not a difficulty spec-wise, but the implementation may need some refactor.
Yeah, I'd prefer to avoid this if possible.
How does the sanitizer work against CSS variable? Or it doesn't matter?
I don't think it matters. If you're allowing CSS variables means that you're allowing <style>
elements / style
attributes, and then you may as well set background-image
directly (without a variable).
The interesting case here is where you disallow styles, but end up injecting styles anyway due to an attr()
function.
To make this easier to handle, I think this part: [omitted] should be changed to use a familiar calc()-like expression instead.
I don't like this approach; either * 1px
is a special-cased token sequence that just indicates the same thing that "px" does in the current spec, or a full calculation is possible, which requires some complex new parsing (to allow for an ident at the beginning, which isn't part of calc()) and hooking into the "math function" part of the spec. That's a lot of complexity to avoid either a keyword or just using attr() inside a calc(). It's also unclear how this would mesh with the non-numeric attr() cases.
Yes, the type checking on parse seemed an unnecessary step, as the value is going to be validated again anywhere when you evaluate the property.
While not technically necessary, I think it's useful as a way to trigger the fallback value. This functionality isn't present in var(), but I think that attributes are more likely to accidentally contain trash (most particularly, the empty string or whitespace, instead of being deleted) than custom properties are, and being able to trigger your default value in those cases seems useful.
Although adding resolution seems harmless enough.
Yes, I'm adding the same types that Typed OM is aware of: resolution and flex are new additions.
not so sure about the attr-in-attr thing, that means you need to do cycle detection on that too, and that's one of the most expensive parts of var().
Currently the spec requires the attribute value to be a simple literal; attr(foo length)
would fail for foo="calc(1px + 2px)"
, as it's looking for a dimension token. I probably want to relax that, but I do plan on explicitly disallowing attr(), var(), and env() in the attribute value; their presence will make the value illegal so it'll use the fallback instead. This avoids all the dependency-tracking issues, without losing anything significant I think; writing foo="attr(bar px)"
seems like a weird thing to do.
It also keeps the information flow to a minimum; you'll only get precisely the attribute you requested injected into your stylesheet, not an unexpected attribute or a random custom property.
(I'm excluding env() because it's an another arbitrary reference, especially once author-defined env() gets nailed down.) (I'll also put a note in the spec about excluding future reference functionality, to remind us to amend this spec if we add more stuff in the future. For example, I think I'll want to exclude custom functions once they exist.)
attr() opens the door to make any attribute potentially an image load,
The threat surface is just that y'all sanitize URLs proactively, yeah? And this would allow URLs to get fed into the system that y'all haven't pre-examined.
It doesn't look like this allows any attacks in XSS terms or anything; an attribute can only cause an image load if the author purposely put an attr(foo url) in an image-loading property, which seems pretty explicit about their intent. And with attr() being disallowed from the attribute's value, it can't shift the attack surface to an unexpected attribute either.
Is this right? How bad is the sanitization issue?
Is there any reason not to make all attr() values valid at parse time?
Yeah, after discussion on the call, this is my plan. It's technically a behavior change for existing attr() uses, but a minor one; if an author is currently writing something like content: "foo"; content: attr(foo) 10px;
(accidentally relying on the second declaration being invalid and instead using the first), then after this change it would instead accept the second declaration and just be iacvt. That seems like a pretty insignificant worry to me, tho. (In particular, if they're currently writing an invalid 'content' by itself, with no preceding 'content' to fall back to or 'content' from another lower-specificity rule to cascade with, then iacvt is identical behavior to parsing failure in user-observable respects.)
The interesting case here is where you disallow styles, but end up injecting styles anyway due to an attr() function.
Can you elaborate on this, @emilio?
Can you elaborate on this, @emilio?
I just meant that the interesting case in terms of the sanitization-bypass that I mentioned is when you're disallowing the style attribute and <style>
elements (as otherwise it is pointless).
Is this right? How bad is the sanitization issue?
I'm not the right person to evaluate that; @hsivonen maybe?
The CSS Working Group just discussed [css-values-4] Switch advanced attr() to being var()-like
.
Reminder to please review the new text at https://drafts.csswg.org/css-values/#attr-notation
I like it. I think it'd just suggest adding one more attr-type (called tokens
or auto
?) that just doesn't do first-pass type validation (at least, nothing beyond https://drafts.csswg.org/css-syntax-3/#typedef-declaration-value), and passes the whole content of the attribute as a list of tokens. Most of the time, I think the validation by type is useful, but occasionally it may be limitting, so it could be nice to allow bypassing it, since the underlying machinery will be there anyway.
That way, you could write stuff like that:
<style>
.foo { background: attr(data-light-background auto, white); }
@media (prefers-color-scheme: dark) {
.foo { background: attr(data-dark-background auto, black);}
}
</style>
<div class=foo data-dark-background="center / contain no-repeat url("logo.svg"), #222 35% url("pattern.png");">
…
</div>
We (the RealObjects PDFreactor development team) had a look at this issue and the new specification text:
We agree with the general idea of this change. In fact, our current implementation already employs a similar approach.
Also we have no objections to the new text.
We are much in favor of the tokens
"type", as it would grant significant additional possibilities, with no relevant drawbacks.
I like tokens
, and something similar could be useful for variables too, as I proposed in https://github.com/w3c/csswg-drafts/issues/2749#issuecomment-396043654
I ended up not putting tokens
in yet (despite suggesting it as well) because it still requires a pass over the values to ensure there's no reference functions mixed in. I have an issue about whether or not to allow color functions and math functions as well, for the same reason; if we decide to allow them (after verifying there are no reference functions in them), then I'll pop in tokens
as well.
The latest spec seems very generous on the fallback value:
- Otherwise, if the
attr()
function has a fallback value as its last argument, replace theattr()
function by the fallback value. If there are anyvar()
orattr()
references in the fallback, substitute them as well.
This is different from the old CSS3 spec that, we no longer verify the fallback against the given type.
So I got a few questions:
Is this change intentional? In other words, do we still want to verify fallback against the given type? Btw, some developers may assume that we do verify the fallback, like we do that for registered custom properties.
(If yes for 1) When using the fallback, while the given type is a specific unit (e.g., px
), do we want to parse the fallback as a number or a pixel value? For example, should we resolve attr(something-invalid px, 100)
into 100
or 100px
?
Note: There's an existing WPT expecting 100
attr()
still valid at computed time? Again, some developers may assume no, similar to registered custom properties.It's an intentional change. It lets you provide fallback of a different type entirely when the argument is missing. (Like width: attr(w px, auto);
)
Fallback isn't parsed at all. The WPT looks like it's wrong? It will fail to parse the attribute (since "300px" isn't a number), then substitute in 200, which isn't a valid 'width' value in non-quirks pages, making the div iacvt so it reverts to "auto" and won't match the ref.
Yes, that's fine. Fallback isn't parsed by attr() at all.
There's a compatibility issue found via WPT css/css-variables/variable-generated-content-dynamic-001.html
<style>
#parent { --x: attr(data-attr); }
#child::before { content: var(--x); }
</style>
<div id=parent><div id=child data-attr="foo"></div></div>
Before the spec change, we got a ::before
pseudo-element on #child
with "foo".
Now the resolution of attr()
is similar to var()
. So we first compute --x
on #parent
by substituting attr()
, and get the empty string. Then in #child::before
we inherit --x
from its parent, and get an empty content
.
In other words, we got a behavioral change even when the declaration is valid.
Besides, could you clarify the substitution value for types string
and url
when the attribute is missing? The current spec says:
string
and url
types: "No value triggers fallback"I suppose there is always no substitution value when the attribute is missing? So that the behavior is different from when the attribute value is the empty string.
Ah, my intention was that a lack of value is different from an empty value (and a lack of value always triggers fallback). I'll clarify the spec.
The latest draft has the attribute name defined as <wq-name>
, which would make all of these valid:
attr(foo)
attr(m|foo)
attr(|foo)
attr(*|foo)
I don't think that last one was intentional, but if it was we need to define what it means with <element a:foo="1" b:foo="2">
, as attribute order is not significant in XML.
Can this be used with @property
? When syntax:"<number>"
is present, it is possible to parse the HTML attribute value as <number>
?
@property --foo {
syntax: "<number>";
initial-value: 0;
inherits: true;
}
@heycam pointed out back in 2019 something I think got lost in the subsequent conversation:
I do find it a little odd that the second argument to
attr()
names CSS syntax types and other operations like dimension value construction, and how e.g.string
means take the literal string content and turn it into a<string>
token, whilenumber
means to parse the attribute value as a<number>
token. I'm half wondering whether it makes sense to align with the syntax used for custom property registration for things like the latter.
This is a great point. @property ... { syntax: ...; }
and attr(... ...)
should use the same syntax for referring to CSS syntax types.
These edits have now been performed, at https://drafts.csswg.org/css-values-5/#attr-notation, so I'm closing this issue.
Currently, the "advanced" attr() functionality that lets you specify a type to parse as and a fallback to use is specified to require treating the attr() as the specified type during parse time. That is,
width: attr(foo color);
must be rejected as invalid at parse time, becausewidth
doesn't take<color>
arguments.This definition predates the introduction of var() and its specifying of how to handle values that are invalid when substituted in; at the time attr() was written, "invalid at computed-value time" didn't exist, so the only way we knew how to handle this sort of thing was to eagerly typecheck and specify defaults.
One of our Chrome engineers (@xiaochengh) is looking into implementing advanced attr(), and reviewing it with them, it looks like implementing the function as specified will be quite complicated; handling its type-checking will require additions to every manually-parsed grammar in the browser.
However, switching attr() over to acting like var() should be much simpler. All the machinery for handling something substituted at computed value time, and possibly being iacvt, already exists and can be leaned on pretty easily for all properties.
(The type argument does still have value here. For one, it's a switch that dictates whether you take the attribute value as a literal string, or CSS-tokenize it. For two, it advertises to later stylesheet readers what's expected. For three, it helps you guard against garbage in attributes, which is more likely than garbage in variables, and use a fallback instead. For four, the
px
/etc values are useful for parsing plain numeric attributes, which are common in XML languages, into the appropriate CSS unit, without needing calc() shenanigans.)So the proposal, in full:
attr(foo string)
.tokens
type (or maybe spell it*
) that does no parsing, but does still tokenize the value and substitute it accordingly (rather than substituting it as a<string>
), to handle more complicated cases, now that we're free from the tyranny of parse-time type-checking. This type will only trigger fallback if the attribute isn't present at all.Thoughts?
(Note for the Agenda+; I won't be attending the Nov 6th call, so please schedule it for the Nov 13th call instead.)