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

[typed-om] Trim CSSResourceValue and subclasses to opaque objects for this level, punt rest to level 2 #716

Open
tabatkins opened this issue Feb 22, 2018 · 35 comments

Comments

@tabatkins
Copy link
Member

There's a bunch of undefined behavior around loading, urls, and such of CSSResourceValue right now. Doing it right will require some careful design and probably some research (and probably go hand-in-hand with speccing out these behaviors in more detail for CSS in general). This isn't compatible with shipping level 1 in the near future.

I'd be inclined to punt the entire thing, but Custom Paint relies on CSSImageValue to pipe images into the PaintWorker (tho there's apparently some HTML edits that need to be made, paging @bfgeek).

So, I propose that for this level, we pare it down to the bare minimum we need to represent an image, for Custom Paint purposes. That means removing CSSURLImageValue entirely, and removing all the attributes (state, intrinsicWidth/Height/Ratio) from the classes. We'll be left with an opaque handle to an image value that doesn't expose any information about the loading pipeline, and which can only be obtained from CSS itself (no constructor anymore!), so all the undefined behavior is just what CSS already has undefined.

According to @bfgeek, this should be fine. It does negate one of his use-case examples (rendering with a fallback image), but that's fine for now; most Custom Paint stuff is fine just spamming the CSSImageValue into the canvas, even if it's currently not loaded, and just getting called back when it is loaded and drawing it correctly.

@tabatkins
Copy link
Member Author

Ugh, there's still some details that we'd need to worry about - if the stylesheet says background-image: url(foo);, and we grab that value (as a CSSImageValue) and set it into a different stylesheet, does it retain its relative-ness and resolve against the new base URL?

@tabatkins
Copy link
Member Author

tabatkins commented Feb 22, 2018

Here's a precise example of the above problem:

<!doctype html>
<iframe src="http://example.com"></iframe>
<script>
const body = document.body;
const iframe = document.querySelector("iframe");
body.style.backgroundImage = "url(foo)";
// Resolves against the document's URL.
const img = el.style.attributeStyleMap().get('background-image');
iframe.contentDocument.body.attributeStyleMap().set('background-image', img);
// What base url does this url() resolve against?
</script>

@tabatkins
Copy link
Member Author

I think I can still get away with explicitly leaning on CSS's current lack of definition, since these values can only be produced from CSS and don't expose anything directly.

@darrnshn
Copy link
Collaborator

Hey @tabatkins, do we also need to change the wording for reifying an image?

@tabatkins tabatkins reopened this Feb 23, 2018
@tabatkins
Copy link
Member Author

@darrnshn Yeah, doing that now.


I wonder if we want to actually have a different reification behavior for relative URLs, actually. Fragment URLs need to stay as fragments, but we can probably absolutize all other URLs. This is probably less confusing behavior overall; people rarely, if ever, pull properties between documents anyway, and if they do, they probably don't expect their URLs to change!

If we define that, then it's easy to add back the CSSURLImageValue and its url attribute, as it'll always be a (well-defined) absolute URL or a fragment url, none of this nonsense "resolved at some unspecified time" stuff that CSS currently does.

@tabatkins
Copy link
Member Author

tabatkins commented Feb 23, 2018

We'll still have to grapple with the fact that CSS's types don't have a nice subclass/superclass relationship; a <url> is a type of <image>, but it's also completely other, unrelated types.

I suspect what we'll eventually want is to create a more generic CSSURLValue class as well, which can be used generally, and just define that it "matches" <image> as well as some other things. Then CSSURLImageValue and CSSURLValue, plus maybe some other specialized subclasses, can just spam the same language between them. We can look into abstracting it into a mixin later, but that's not necessary right now.

So this'll still leave us with .state not being exposed, or the width/height/ratio (since they implicitly expose the state), yet, so that'll stay for level 2.

@tabatkins
Copy link
Member Author

So, issue here.

Say you have a property like 'mask-image'. It takes a url(). This url() is either an <image> or a <mask-source>. The former wants to expose width/height/ratio; the latter might want to expose some sort of information from the <mask> element. You can't tell which thing a url() is until after the URL resolves! This means that we can't tell what type to reify the value to until after network activity has happened.

@bfgeek's suggestion: in L2, have a CSSURLValue class, representing a network resource more generally, and give it a promise-returning method that gives back a concrete object representing the actual resource that was fetched. (Or lets you choose to parse it in a particular way? That way you could point to an SVG file and get it as an image or a reference, depending on your needs.)

This problem might infect general <image> usage too, if we ever let people point to SVG <linearGradient> and use it the same way as linear-gradient().

@annevk
Copy link
Member

annevk commented Feb 25, 2018

I think Firefox makes that decision based on the presence of a fragment in the URL. But yeah, it'd be good to sort out how that actually works, before adding the API.

My example also showed that browsers disagree on when to perform URL parsing here (and Chrome even seems to use a different URL parser from the one it uses normally), which is another problem. A lot of that surfaces with the normal CSSOM as well of course, and results from the underlying model not being clearly specified.

@tabatkins
Copy link
Member Author

@annevk Unfortunately, the WG resolved to do differently than Firefox's current behavior https://lists.w3.org/Archives/Public/www-style/2017May/0051.html. We really do need to wait until the response is received before we can decide what it is.

@tabatkins
Copy link
Member Author

tabatkins commented Feb 26, 2018

Okay, I think I've hit on the necessary design.

I need a CSSImageValue, subclass of CSSStyleValue, representing all images. Currently completely opaque. In the next level we'll add some subtypes for this, like CSSLinearGradientFunctionValue or something, and attributes for width/height/ratio/etc.

I also need a CSSURLValue, subclass of CSSStyleValue, representing the url() syntax. All url()s are reified as this, because in some cases we can't tell what type a url() should resolve to until after we fetch it, and this behavior might spread to all <image> cases (if we ever allow, for example, pointing to an SVG <linearGradient> as an <image>, which seems reasonable).

We'll add a CSSImageish typedef that accepts CSSImageValue and CSSURLValue, and things that want to take a CSSImageValue should take the typedef instead; if a CSSURLValue is used and doesn't end up pointing to an image, it'll just be an invalid image, no problem.

In the next level we'll add a CSSURLImageValue, subtype of CSSImageValue, which you can obtain from a CSSURLValue (either thru a constructor, or a promise-returning method, or something), which exposes the width/height/etc stuff that you expect from an image. This will never be reified directly from an underlying value, you must always manually obtain it via the converter.

I don't particularly like this design, but I think it's the best I can do while respecting the url() resolution and accommodating likely future expansion.

@tabatkins
Copy link
Member Author

Agenda+ to make sure that the WG is aware of this design and approves of it.

@tabatkins
Copy link
Member Author

CSSURLValue details:

  • will expose a .url attribute.

  • This URL will always be either absolute, or a bare hash. If absolute, it will be absolutized according to standard CSS rules (using the base URL of the stylesheet/document). The underlying value might or might not be absolutized at this point (the timing is currently undefined in CSS), but Typed OM will always present an absolutized URL. This avoids the current undefined behavior in CSS, and gives more author-friendly behavior when moving values between contexts with different base URLs.

  • will have a constructor that takes a URL; again, it will either be absolutized immediately or left as a bare hash, using the "current setting object's API base url" (per @domenic)

@darrnshn
Copy link
Collaborator

@astearns could this go on the agenda? thanks

@astearns
Copy link
Member

@darrnshn it is on the agenda for this week (the Agenda+ tag got that done)

@FremyCompany
Copy link
Contributor

LGTM

@AmeliaBR
Copy link

This URL will always be either absolute, or a bare hash.

If fragment-only URLs are going to have different behavior, should the type object have a boolean attribute indicating whether it is an absolute URL or a fragment?

people rarely, if ever, pull properties between documents anyway, and if they do, they probably don't expect their URLs to change!

SVG <use> elements from another file effectively pull in URL references, including absolute and fragment-only references, both of which are expected to remain relative to the original document. (And Chrome has broken things a few times in the past year or so as things switched to shadow DOM model.) Even same-file hash references aren't fully defined for a shadow DOM model. I tried to summarize intended behavior in the SVG spec, but it would be nice to have a more universal model to build on.

I also need a CSSURLValue, subclass of CSSStyleValue, representing the url() syntax. All url()s are reified as this, because in some cases we can't tell what type a url() should resolve to until after we fetch it,
...
In the next level we'll add a CSSURLImageValue, subtype of CSSImageValue, which you can obtain from a CSSURLValue (either thru a constructor, or a promise-returning method, or something)

I think the one thing that is missing from your model is a way to distinguish an unresolved URL from a resolved URL that isn't an image type. Maybe that could be indicated by an attribute (resolved = true), or maybe the unresolved URL would be its own type (CSSUnresolvedURLValue), which has a method or attribute to access the Promise that returns the resolved type object, whether that's a CSSImageURLValue or some other type that subclasses CSSURLValue.

CSSURLValue details:
...

  • will have a constructor that takes a URL

Does it make sense to have a constructor for the "unresolved" type? Should the constructor accept a class name of the type you're trying to resolve as? Or, to be able to fully model the behavior of mask-image and so on, a list of types in order?

@tabatkins
Copy link
Member Author

tabatkins commented Feb 28, 2018

If fragment-only URLs are going to have different behavior, should the type object have a boolean attribute indicating whether it is an absolute URL or a fragment?

CSS doesn't have any special indicator between the two in the string syntax, so I don't think Typed OM particularly needs to do anything special here.

SVG elements from another file effectively pull in URL references, including absolute and fragment-only references, both of which are expected to remain relative to the original document. (And Chrome has broken things a few times in the past year or so as things switched to shadow DOM model.) Even same-file hash references aren't fully defined for a shadow DOM model. I tried to summarize intended behavior in the SVG spec, but it would be nice to have a more universal model to build on.

The only behavioral change I'm suggesting here is that relative URLs get absolutized when you reify them, so we don't have to depend on the (unspecified) absolutization time in CSS. Absolute and fragment-only URLs will remain absolute or fragment-only, and so won't change behavior at all.

You can't extract a value from an element inside a use, so I don't think that will matter here. We're not changing the underlying CSS value.

I think the one thing that is missing from your model is a way to distinguish an unresolved URL from a resolved URL that isn't an image type. Maybe that could be indicated by an attribute (resolved = true), or maybe the unresolved URL would be its own type (CSSUnresolvedURLValue), which has a method or attribute to access the Promise that returns the resolved type object, whether that's a CSSImageURLValue or some other type that subclasses CSSURLValue.

The url() syntax will always reify as a CSSURLValue, no matter whether it's resolved or not, whether it's used as an <image> or something else. And at this level, that's all that will happen.

In L2 I expect that we'll have a .asImage() method on the class that returns a CSSURLImageValue (currently doesn't exist, will be added in L2), which is specialized to be a url to an image, and thus can expose things like width/height. (And more for any other specialized subclasses like CSSMaskReference or something.)

Does it make sense to have a constructor for the "unresolved" type? Should the constructor accept a class name of the type you're trying to resolve as? Or, to be able to fully model the behavior of mask-image and so on, a list of types in order?

CSSURLValue is the "unresolved" type.

@annevk
Copy link
Member

annevk commented Mar 1, 2018

CSS doesn't have any special indicator between the two in the string syntax, so I don't think Typed OM particularly needs to do anything special here.

So you would suggest having to use value.url.startsWith("#") or some such? It seems nice to expose this difference. I'd actually argue that in the type system you'd expose these as two different types so when can explicitly create a URL-fragment-value or a URL-value.

That way for the URL-value we could also expose normal URL manipulation down the road while not having to special case dealing with it actually being a URL-fragment-value.

@tabatkins
Copy link
Member Author

Having a readonly indicator that it's fragment-only and thus specially treated would be fine with me; I was responding to the idea (possibly misread by me) of a flippable boolean switching it from the special fragment-only behavior to treating it like a normal URL and resolving it against a base.

I'd actually argue that in the type system you'd expose these as two different types so when can explicitly create a URL-fragment-value or a URL-value.

That way for the URL-value we could also expose normal URL manipulation down the road while not having to special case dealing with it actually being a URL-fragment-value.

This seems reasonable to me.

@FremyCompany
Copy link
Contributor

Very weak preference on not having them be two different types, as they are not different objects in Edge.
Fine with a boolean to differentiate between them.

@tabatkins
Copy link
Member Author

Lots of things that might not be different objects internally are different objects when reified - see the math hierarchy.

@css-meeting-bot
Copy link
Member

The Working Group just discussed [typed-om] Trim CSSResourceValue and subclasses to opaque objects for this level, punt rest to level 2.

The full IRC log of that discussion <dael> Topic: [typed-om] Trim CSSResourceValue and subclasses to opaque objects for this level, punt rest to level 2
<dael> github: https://github.com//issues/716
<dael> astearns: now that we have TabAtkins
<TabAtkins> I'LL LITERALLY JUST BE FIVE SECONDS OR SO
<dael> [everyone curses Tab's internet]
<dael> TabAtkins: This was brought up a week o r two ago as an ask f or review. Ana had some discussion.
<Chris> s/Ana/Anne
<dael> TabAtkins: Big points: I did discuss rough outline before. Newer bits: because CSS distinguishes between normal a nd fragment-only URLs we want to reflect that in the URL structure somehow. To say this is full o r fragment. My plan was storing what it was, but Anne remarked hewould prefer if it was refered more directly either as a bolean or a sep URL class. franremy had a weak preference
<dael> TabAtkins: I'll do those edits soon. Any opinions let me know. Otherwise I'll do what I outline in the thread here.
<dael> astearns: Any other opinions for TabAtkins to consider?
<dael> astearns: Alright. We're on notice you're going to change.
<dael> TabAtkins: I guess not. Wanted to ping for review because it's a decent change.

@FremyCompany
Copy link
Contributor

I don't think we still need the Agenda+ here

@css-meeting-bot
Copy link
Member

The Working Group just discussed Loading and absolutization timing, and agreed to the following resolutions:

  • RESOLVED: We accept this proposal https://github.com/w3c/css-houdini-drafts/issues/716#issuecomment-368659261 but also expose the original URL and base URL
The full IRC log of that discussion <dael> Topic: Loading and absolutization timing
<dael> TabAtkins: CSS is currenlty vague. It happens at some point but we don't know when.
<dael> github: https://github.com//issues/716
<nainar> https://github.com//issues/716#issuecomment-368659261
<dael> TabAtkins: Her'es my proposal ^
<dael> TabAtkins: Has a URL attribute. It's either an absolute or a bare hash. If it's not a bare hash we'll use CSS ruels. Underlying value may not be absolutized, but when it reifies we turn it absolute. If you move a style from one doc to another it may be relative in the old way, but it's now absolute.
<dael> TabAtkins: I think that's what people want.
<dael> TabAtkins: When you contruct a value it's absolutized immediately.
<dael> dbaron: When is it bare hash?
<dael> TabAtkins: When you pass it in as a local reference.
<dael> TabAtkins: That's spec in the value spec?
<dael> plinss: It's a string not a URL?
<dael> TabAtkins: Yes, it's strictly spec that it's a string.
<dael> TabAtkins: Only controversy is the early absolutizing. Is that okay or shouldwe make it retain it's relativeness and better define when it stops being relative.
<dael> TabAtkins: If you want to manipulate the URL the URL spec requires an absolute value or a URL with a base. It would be harder for authors since it's hard to get the base.
<dael> plinss: Would it make sense to keep it as a relative URL but also expose it as a base?
<dael> TabAtkins: Possibly. In the current CSS if you use the OM APIs if you pull a reltivate URL and write it somewhere else it changes the base.
<dael> plinss: Are we allowing authors to rationalize it as a valid URL if they want to.
<dael> TabAtkins: Do authors want this?
<dael> plinss: Yes
<dael> TabAtkins: I doubt they do. When this occurs style sheets are usually in same folder.
<dael> nainar: If we're being more eager about absolutizing should that be in css as well?
<dael> TabAtkins: Pos.
<dael> birlets: I'm wondering if there's a cloning use case?
<dael> TabAtkins: But would people want to do that?
<dael> TabAtkins: I can come up with a reason, but it's a minority case.
<dael> krit: If you provide the bbase would you still get back relative?
<dael> TabAtkins: You have to know where the split happened. If impl want to preserve I can attach the original URL. But the core easiest to use should be absolute.
<dael> krit: I'm fine with it as a default.
<dael> plinss: I expect there will be use cases, like editor. In my experience any time someone tries to help me with URL they get in the way at some point. If it's distructive that's painful. If there's a way to preserve the original url.
<dael> dbaron: Seems like it's not crazy to have multi accessors.
<dael> TabAtkins: I'm happy to put on an original URL value.
<dbaron> s/multi/multiple/
<dael> plinss: Should that but the URL
<dael> TabAtkins: All the DOM apis do absolute URLs.
<dael> TabAtkins: Prop: We accept this proposal but also expose the original URL.
<dael> dbaron: You could also expose base URL
<nainar> Proposal is comment in https://github.com//issues/716#issuecomment-368659261
<dael> TabAtkins: That would have to be dynamic. Oh, base URL as this instant. I could do that.
<dael> dbaron: I don't know how useful, but if you're exposing a bunch of information.
<dael> Rossen_: Okay.
<dael> Rossen_: Suggestions, objections?
<dael> RESOLVED: We accept this proposal https://github.com//issues/716#issuecomment-368659261 but also expose the original URL and base URL

@annevk
Copy link
Member

annevk commented Apr 9, 2018

What does exposing original URL and base URL mean here? The latter is somewhat underdefined in, e.g., <style>, and for the former I showed some disagreement between browsers already for something like url(http://test:test/) (which is technically not a URL and cannot be parsed as one).

@tabatkins
Copy link
Member Author

"original url" is what's actually specified in url() or whatever. "base url" is whatever the base URL that CSS uses in the context that the value is encountered. <style> shouldn't be underdefined - it's the document's base URL. In any case, browsers all have such a base URL in whatever context a url() can be encountered; if they're inconsistent, that's a problem on CSS's side, and Typed OM won't be exposing anything new.

@annevk
Copy link
Member

annevk commented Apr 10, 2018

It'll be exposing the inconsistencies in more places... And exposing "original url" doesn't seem super useful if it includes values that cannot even be parsed as a URL. Why would we do that?

@tabatkins
Copy link
Member Author

It'll be exposing the inconsistencies in more places...

Freshly exposing a previously-hidden inconsistency is a problem. Merely adding another place that exposes a previously-exposed one is fine; the inconsistency still needs to be fixed, but this adds no new problems. (And the base url used has a very large effect on URLs.)

In general, this spec isn't trying to fix problems in core CSS, it's just providing a new way to inspect and manipulate CSS values.

And exposing "original url" doesn't seem super useful if it includes values that cannot even be parsed as a URL. Why would we do that?

It lets you, if desired, exactly reproduce the string-OM behavior of moving a url value between stylesheets; you just have to construct a fresh CSSURLValue from the original url and new base url. I'm not of the opinion that this is particularly useful, but some in the WG wanted it, and it's not a problem to add it; we're just exposing information that's already obtainable via the string-OM.

@annevk
Copy link
Member

annevk commented Apr 10, 2018

@tabatkins but that's not true, as I demonstrated the string-OM doesn't always expose the "original url".

@annevk
Copy link
Member

annevk commented Apr 10, 2018

(I think you're touching on a problem with this API though. The idea that you don't have to understand "core CSS" or can gloss over how it functions when describing this new thing is misguided. Core CSS has to be in a good condition if you want to start exposing it through a public API.)

@tabatkins
Copy link
Member Author

tabatkins commented Apr 10, 2018

but that's not true, as I demonstrated the string-OM doesn't always expose the "original url".

??? You didn't demonstrate anything of the sort, tho. Check http://software.hixie.ch/utilities/js/live-dom-viewer/saved/5882, which is using your example troublesome URL - all browsers expose the exact original URL when querying the style directly (some with added quotes, some without), and all (except pre-Blink Opera, and apparently modern Chrome) expose nothing at all in the computed style.

In the TypedOM I only want to expose absolute URLs normally, which in this case I guess means that it would have a .url == null or something, because absolutization will fail. Then .originalURL == "http://test:test/" and .baseURL will be whatever the correct base url is.

@annevk
Copy link
Member

annevk commented Apr 10, 2018

Chrome and Safari expose a computed style? And differently so if you omit the trailing slash... It does seem that everyone also preserves the original value though, that's unfortunate, but I guess we might as well keep it then.

@tabatkins
Copy link
Member Author

Right. And the computed value differences are unfortunate, but the string-OM already exposes it, so TypedOM doing so doesn't make things worse (and heck, since I'm speccing it to use URL's absolutizing algorithm, it should generally be more consistent than string-OM). It's just something that needs to be fixed.

@SebastianZ
Copy link

@annevk wrote:

And exposing "original url" doesn't seem super useful if it includes values that cannot even be parsed as a URL.

@tabatkins wrote:

"original url" is what's actually specified in url() or whatever.

In the TypedOM I only want to expose absolute URLs normally, which in this case I guess means that it would have a .url == null or something, because absolutization will fail. Then .originalURL == "http://test:test/" and .baseURL will be whatever the correct base url is.

Because the "original url" may not be a URL at all, it sounds like it should rather be called .originalValue or .specifiedValue or something like that and be part of CSSStyleValue, so it's available for all kinds of values.

Sebastian

@annevk
Copy link
Member

annevk commented Jul 23, 2018

That doesn't sound great to me. I don't think we'd necessarily want to preserve the original input in all cases.

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

No branches or pull requests

8 participants