Skip to content

[css-transitions-1] Make discrete properties transition at 50%; exclude them from 'all'. #4441#8520

Merged
fantasai merged 1 commit intow3c:mainfrom
fantasai:transition-discrete
Mar 3, 2023
Merged

[css-transitions-1] Make discrete properties transition at 50%; exclude them from 'all'. #4441#8520
fantasai merged 1 commit intow3c:mainfrom
fantasai:transition-discrete

Conversation

@fantasai
Copy link
Copy Markdown
Collaborator

@fantasai fantasai commented Mar 3, 2023

No description provided.

@fantasai fantasai requested a review from dbaron March 3, 2023 03:32
dbaron
dbaron reviewed Mar 3, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@dbaron dbaron left a comment

Choose a reason for hiding this comment

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

I think this is mostly good, although there are a number of things that are awkward about doing this as a delta spec and it's not completely clear how diff-like to make some of the edits.

I have a few comments below.

In addition to those comments, I think you should specifically patch all occurences of "transitionable" in the "Starting of transitions" section, which does need to be patched to make this part of the detailed algorithm. I think that may need a term (I have mixed feelings about whether to re-use "transitionable") that considers whether the relevant part of transition-property was all or not.

@fantasai fantasai force-pushed the transition-discrete branch from 468fa68 to c48cd62 Compare March 3, 2023 20:10
@fantasai
Copy link
Copy Markdown
Collaborator Author

fantasai commented Mar 3, 2023

I think you should specifically patch all occurences of "transitionable" in the "Starting of transitions" section, which does need to be patched to make this part of the detailed algorithm

I'm patching the definition of "transitionable" here; I don't see that, with that change, anything in "Starting of transitions" needs edits?

@dbaron
Copy link
Copy Markdown
Member

dbaron commented Mar 3, 2023

I'm patching the definition of "transitionable" here; I don't see that, with that change, anything in "Starting of transitions" needs edits?

With your patch to "transitionable", the detailed algorithm in "Starting of transitions" will start transitions for discretely-animatable properties when all is specified for transition-property, which is incorrect.

@fantasai
Copy link
Copy Markdown
Collaborator Author

fantasai commented Mar 3, 2023

With your patch to "transitionable", the detailed algorithm in "Starting of transitions" will start transitions for discretely-animatable properties when all is specified for transition-property, which is incorrect.

I don't see where it says that. It doesn't mention the all keyword in that section at all, afaict. And the section we're editing here now says very clearly that discretely-animatable properties are not included in all. So I don't see how you're arriving at this conclusion.

@fantasai fantasai force-pushed the transition-discrete branch from c48cd62 to c109e1b Compare March 3, 2023 21:28
@dbaron
Copy link
Copy Markdown
Member

dbaron commented Mar 3, 2023

I don't see where it says that. It doesn't mention the all keyword in that section at all, afaict. And the section we're editing here now says very clearly that discretely-animatable properties are not included in all. So I don't see how you're arriving at this conclusion.

OK, after rereading, I agree.

dbaron
dbaron approved these changes Mar 3, 2023
View reviewed changes
Copy link
Copy Markdown
Member

@dbaron dbaron left a comment

Choose a reason for hiding this comment

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

OK, looks good. Thanks.

@fantasai fantasai merged commit 69f4ac7 into w3c:main Mar 3, 2023
@fantasai fantasai deleted the transition-discrete branch March 3, 2023 22:29
@dbaron dbaron mentioned this pull request Mar 9, 2023
Merged
@josepharhar josepharhar mentioned this pull request Mar 15, 2023
Closed
1 task
@fantasai fantasai mentioned this pull request Mar 23, 2023
Closed
@josepharhar josepharhar mentioned this pull request Mar 29, 2023
Closed
1 task
@dholbert
Copy link
Copy Markdown
Member

dholbert commented Apr 24, 2023

I have an editorial nit with the new text here which I filed as #8761

@dholbert dholbert mentioned this pull request May 5, 2023
Closed
@dholbert
Copy link
Copy Markdown
Member

dholbert commented May 5, 2023

I also have one question about various tests that were updated to test this change but I think are mistaken; see web-platform-tests/wpt#39871

(Alternately, maybe the tests are right and the spec-text just needs clarification?)

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

Reviewers

@dbaron dbaron dbaron approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@fantasai @dbaron @dholbert