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

[scroll-animations-1] JS API for view-timeline-inset #7748

Closed
flackr opened this issue Sep 15, 2022 · 15 comments
Closed

[scroll-animations-1] JS API for view-timeline-inset #7748

flackr opened this issue Sep 15, 2022 · 15 comments
Labels
Closed Accepted by CSSWG Resolution Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. scroll-animations-1 Current Work

Comments

@flackr
Copy link
Contributor

flackr commented Sep 15, 2022

In CSS we have view-timeline-inset to allow reducing the portion of the scroll area which would be considered visible, however the JS ViewTimeline constructor does not have the ability to set the same property.

I propose extending the ViewTimelineOptions dictionary with a property that allows setting the one or two valued inset. The equivalent property in IntersectionObserver is a DOMString rootMargin which is a space separated list of offsets. We could do the same here, but call it inset (to be consistent with the CSS property), or we could accept an array of CSSNumeric values.

Option 1: DOMString

dictionary ViewTimelineOptions {
  Element subject;
  ScrollAxis axis = "block";
  DOMString inset = "0px 0px";
};

Option 2: CSSNumeric

dictionary ViewTimelineOptions {
  Element subject;
  ScrollAxis axis = "block";
  sequence<CSSNumericValue> inset = [CSS.px(0), CSS.px(0)];
};

I think there's a slight advantage to accepting the string syntax in that:

  • It allows specifying the auto value from view-timeline-inset, and
  • You can pass the computed value of view-timeline-inset, e.g. new ViewTimeline({subject: el, inset: getComputedStyle(subject).viewTimelineInset})

However, passing and parsing a string seems slightly inconsistent with the direction of other JS API's where we've decided to go in the direction of richer types, e.g. #7589.

@fantasai
Copy link
Collaborator

Do we want to allow both versions of input? Is that a thing we can do?

@birtles
Copy link
Contributor

birtles commented Oct 31, 2022

Allowing both definitely seems attractive. Should ViewTimeline also expose these values as (readonly?) attributes? Perhaps using the typed version?

@flackr
Copy link
Contributor Author

flackr commented Oct 31, 2022

Allowing both definitely seems attractive.

Allowing both sounds reasonable to me.

Should ViewTimeline also expose these values as (readonly?) attributes? Perhaps using the typed version?

Yes, I think it should.

@fantasai
Copy link
Collaborator

Should the attributes be sequence<CSSNumericValue> inset or CSSNumericValue startInset; CSSNumericValue endInset;? The latter seems a bit more obvious.

@flackr
Copy link
Contributor Author

flackr commented Oct 31, 2022

Good question, when providing CSSNumericValues the latter seems more obvious, but if we want to accept strings as well I think it's better to have one property so you can pass the computed value string from view-timeline-inset directly without having to split it as in my example above:

new ViewTimeline({
  subject: el,
  inset: getComputedStyle(subject).viewTimelineInset})

Whereas with startInset and endInset as separate properties you'd presumably have to split it like so:

const insets = getComputedStyle(subject).viewTimelineInset.split(' ');
new ViewTimeline({
  subject: el,
  startInset: insets[0],
  endInset: insets[1] || insets[0] /* Use first inset for endInset if no second inset supplied */
});

@fantasai
Copy link
Collaborator

@flackr Oh, I didn't mean for the input -- that makes sense to me as-is. But there was a suggestion to add an inset attribute to the ViewTimeline object itself. Which does bring up a few interesting questions:

  • should the inset be incorporated into the startOffset / endOffset readouts? Or should those represent raw object bounds only?
  • should the inset be a separate attribute? If so, as one sequence attribute inset or as startInset and endInset? (Personally, I think the latter is more understandable, even if the input object uses a sequence.)

My original thought was that startOffset and endOffset would be the effective start/end of the timeline, and incorporate any inset calculations.

@LeaVerou
Copy link
Member

LeaVerou commented Nov 2, 2022

Do we want to allow both versions of input? Is that a thing we can do?

Yup, and I think accepting both is the way to go.

@flackr
Copy link
Contributor Author

flackr commented Nov 2, 2022

@fantasai sorry for the confusion, makes sense!

  • should the inset be incorporated into the startOffset / endOffset readouts? Or should those represent raw object bounds only?

I agree with your assessment, that startOffset and endOffset should probably incorporate inset calculations.

  • should the inset be a separate attribute? If so, as one sequence attribute inset or as startInset and endInset? (Personally, I think the latter is more understandable, even if the input object uses a sequence.)

I think you could make a similar argument that by exposing insets it makes it more ergonomic to pass one view timeline's insets to another, e.g.

let timeline2 = new ViewTimeline({
  subject: el,
  insets: timeline1.insets
});

This is also similar in spirit to how intersectionObserver exposes the rootMargin with a syntax similar to the inut: https://developer.mozilla.org/en-US/docs/Web/API/IntersectionObserver/rootMargin

Though I think that's a fairly weak reason to do it and would be fine with it being separated into startInset and endInset since as you say this is more understandable.

@fantasai
Copy link
Collaborator

fantasai commented Nov 7, 2022

OK, I think we can split this issue down into three sub-points:

Resolution 1: Proposed to accept both string and TypedOM 2-value sequence inset within ViewTimeline Options. @flackr would you mind posting the WebIDL for this? :)

Resolution 2: Proposed that existing ViewTimeline.startOffset and .endOffset attributes incorporate inset calculations (so that they continue to accurately represent the start/end scroll offsets of the ViewTimeline within the scroll container).

Resolution 3: Choose one of

  • Add inset two-value sequence to ViewTimeline
  • Add startInset and endInset to ViewTimeline
  • Don't add inset attributes to ViewTimeline yet, just store them internally

(I'm leaning towards the third.)

@flackr
Copy link
Contributor Author

flackr commented Nov 8, 2022

Thanks @fantasai, I agree on all three points.

Resolution 1: Proposed to accept both string and TypedOM 2-value sequence inset within ViewTimeline Options. @flackr would you mind posting the WebIDL for this? :)

I think this would be:

dictionary ViewTimelineOptions {
  Element subject;
  ScrollAxis axis = "block";
  DOMString or sequence<CSSNumericValue> inset = [CSS.px(0), CSS.px(0)];
};

(I'm leaning towards the third.)

I'm also leaning towards the third.

@flackr
Copy link
Contributor Author

flackr commented Nov 8, 2022

@tabatkins I think you had reason to not support string values in these contexts (from #4352 (comment)).

If we do this, it would probably be good to remove support for supplying a string like "42px", that way we can infer that strings are keywords.

@tabatkins
Copy link
Member

Yeah, specifically I mean there not parsing a string as a CSS value, instead just letting strings be keyword values. That would imply that "42px" would be the ident with value "42px", written as \34 2px in CSS syntax. While we're not universal with it, we often do this sort of "the string is used literally" behavior precisely so you don't have nested escapes, with a CSS escape put in a JS string and thus needing to be JS-escaped as well. It's also slightly better perf to not invoke the parser unnecessarily.

@atanassov atanassov added this to Agenda+ in November 30 2022 Nov 16, 2022
@flackr
Copy link
Contributor Author

flackr commented Nov 16, 2022

Thanks @tabatkins, I can see the value in keeping the parsing and need for escaping simple. So for this proposal, would you prefer that DOMString only support the keyword "auto" and developers have to use CSSNumericValue, e.g. CSS.px(42), for non-auto values?

Should getComputedStyle(elem).viewTimelineInset return CSSNumericValues? If not, is there an easy way to explicitly parse a string into CSSNumericValues?

@css-meeting-bot
Copy link
Member

The CSS Working Group just discussed JS API for view-timeline-inset, and agreed to the following:

  • RESOLVED: Insets accept either a string containing CSS, or a sequence of TypedOM values.
  • RESOLVED: Accept fantasai's proposal for resolution 2
  • RESOLVED: Defer adding inset properties to the timeline object
The full IRC log of that discussion <TabAtkins> Topic: JS API for view-timeline-inset
<TabAtkins> github: https://github.com//issues/7748#issuecomment-1306097703
<TabAtkins> flackr: Proposal was to allow insets in the ViewTimelineOptions object as either a string or a pair of CSSNumericValues
<TabAtkins> flackr: But I recalled on other issues there were parsing concerns for things other than idents
<TabAtkins> flackr: So i pinged Tab on this issue
<TabAtkins> flackr: So yes we should add this attr, but should it support strings?
<fantasai> scribe+
<fantasai> TabAtkins: While we aren't 100% consistent, we try to avoid doing CSS parsing on small instances of CSS values
<fantasai> ... in APIs like this
<fantasai> ... because in the rare cases where you need to do an escape, it needs to be double-escaped
<fantasai> ... The general rule for identifiers is that we don't parse stirngs [missed]
<fantasai> ... You can express any string as an ident if it's properly escaped
<fantasai> ... [something else]
<fantasai> ... So I proposed not allowing string-based values here, and just sticking with the TypedOM objects
<fantasai> ... They're easy to work with
<fantasai> flackr: should still allow string for auto
<fantasai> TabAtkins: especially because we need to allow identifiers, we'd need to do string parsing in identifiers
<fantasai> ... if you wanted a name with a weird thing, would need to double-escape
<fantasai> ... would have string just be the literal value of the string
<fantasai> ... which means we can't mix keywords with other values in strings
<Rossen_> ack fantasai
<TabAtkins> fantasai: I think the arg for wanting to allow string-based is that you can pipe getComputedStyle() right back into it
<TabAtkins> fantasai: and just easier to write as a literal "2px" rather than CSS.px(2)
<TabAtkins> fantasai: re ident escaping concerns, there's no custom idents in this API so escapes are never necessary anyway
<Rossen_> q?
<flackr> q+
<TabAtkins> flackr: I'm a little curious - if we want to use typedom objects going forward, is there easy ways to get that?
<fantasai> TabAtkins: There's a TypedOM for computed style
<fantasai> ... not all values have that defined, but for simple things it should work
<fantasai> ... for more complex ones, will just get a string
<TabAtkins> flackr: so maybe that alleviates some simple concerns
<TabAtkins> fantasai: Well what's the computed style of -inset? It's one or two values, so what will we get out of typed om computed style?
<TabAtkins> flackr: this is a new property, we can define it to be what we want for the input
<TabAtkins> fantasai: So what would that be?
<TabAtkins> flackr: A sequence of numeric or identifer values
<Rossen_> ack flackr
<dbaron> https://developer.mozilla.org/en-US/docs/Web/API/CSS_Typed_OM_API#browser_compatibility
<TabAtkins> flackr: So I propose we onlya ccept the typedom representation, and specify the computed typed om for the property to give the same values, to support passthru
<TabAtkins> dbaron: is the API not gonna work right if the impl wants to do this without typed om?
<TabAtkins> dbaron: I ask because typedom is implemented right now in chromium but not gecko or webkit
<TabAtkins> flackr: sounds like it would be an issue
<iank_> I believe webkit has an implemtnation just not shipped yet
<TabAtkins> fantasai: What's the downside of stringbased?
<TabAtkins> fantasai: If we have both you can use whichever is easiest
<ydaniv_> +1 for both
<TabAtkins> TabAtkins: my only concern is impl complexity, and slightly being more consistent about parsing css vs taking strings literally, but i can go either way
<fantasai> Proposed to accept both string and TypedOM 2-value sequence inset within ViewTimeline Options
<TabAtkins> PROPOSED RESOLUTION: Offsets accept either a string or a sequence of typedom values
<fantasai> s/Offsets/Insets/
<TabAtkins> chrishtr: Do we need to specify how the string is parsed?
<TabAtkins> flackr: Matches the CSS property
<TabAtkins> RESOLVED: Insets accept either a string containing CSS, or a sequence of TypedOM values.
<fantasai> Proposed that existing ViewTimeline.startOffset and .endOffset attributes incorporate inset calculations (so that they continue to accurately represent the start/end scroll offsets of the ViewTimeline within the scroll container).
<flackr> +1
<ydaniv_> +1
<bramus> +1
<TabAtkins> RESOLVED: Accept fantasai's proposal for resolution 2
<Rossen_> s/Accept fantasai's proposal for resolution 2/Existing ViewTimeline.startOffset and .endOffset attributes incorporate inset calculations/
<TabAtkins> fantasai: final, do we expose the viewtimelineoptions on the timeline itself? doesn't seem to be use-cases. But we *could* add an inset or startInset/endInset to the timeline if we wanted to.
<flackr> +1 to deferring for now
<fantasai> s/viewtimelineoptions/inset values/
<flackr> (this is resolution 3 from https://github.com//issues/7748#issuecomment-1306097703 )
<TabAtkins> fantasai: not sure if it needs to be done or not, so unless someone thinks it's important we recommend resolving not to do it for now
<TabAtkins> RESOLVED: Defer adding inset properties to the timeline object

@bakura10
Copy link

bakura10 commented Oct 19, 2023

Hi !

I am experimenting this and while the CSS version seems to allow passing CSS variables, I could not make it work for the JS version. I can of course use getComputedStyle, but if the value change (responsive) then the value becomes incorrect. What is the recommended way for that?

This works:

const timeline = new ViewTimeline({
      subject: this,
      axis: 'block',
      inset: '71px 0px'
    });

This does not:

const timeline = new ViewTimeline({
      subject: this,
      axis: 'block',
      inset: 'var(--header-height) 0px'
    });

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Closed Accepted by CSSWG Resolution Commenter Satisfied Commenter has indicated satisfaction with the resolution / edits. scroll-animations-1 Current Work
Development

No branches or pull requests

8 participants