(In reply to Frédéric Wang from comment #20) > (In reply to Zach Hoffman [:zrhoffman] from comment #17) > > Sorry for not responding sooner. It was crashing in at-property-animation.html when trying to check if a custom property is a shorthand <https://phabricator.services.mozilla.com/D191059?vs=on&id=793059#C7129453NL498> by passing the property's `mID` to `nsCSSProps::IsShorthand` (which fails on an assert in `nsCSSProps::IsShorthand` <https://searchfox.org/mozilla-central/rev/78a2c17cc80680a5a82446e4ce7c45a73b935383/layout/style/nsCSSProps.h#70-71> , because that assert did not consider `eCSSPropertyExtra_variable` a valid variant. > > Just to comment on this, instead of tweaking the assert to accept `eCSSPropertyExtra_variable`, my approach was to modify the callers of `nsCSSProps::IsShorthand` to handle `eCSSPropertyExtra_variable` separately (e.g. my change to the MOZ_ASSERT in `nsTransitionManager::ConsiderInitiatingTransition`). If you instead choose to just fix the assert so it can handle `eCSSPropertyExtra_variable`, some of the changes I made to avoid calling `nsCSSProps::IsShorthand` with `eCSSPropertyExtra_variable` can probably be removed. > > Also, technically CSS variables are all longhand properties, so the method should probably return false. But `eCSSPropertyExtra_variable` is the last value at https://searchfox.org/mozilla-central/source/layout/style/nsCSSPropertyID.h.in#35 so I believe your current version returns true. So calling `nsCSSProps::IsShorthand` with `eCSSPropertyExtra_variable` could make things wrong (for example the assert in ConsiderInitiatingTransition I just mentioned would cause a crash when we make custom properties transitionable). Thanks, `nsCSSProps::IsShorthand` is fixed now WRT behavior for `eCSSPropertyExtra_variable` in [D191059](https://phabricator.services.mozilla.com/D191059), though we have yet to remove unnecessary `nsCSSProps::IsShorthand()` calls and avoid `eCSSPropertyExtra_variable` being passed to it. (In reply to Frédéric Wang from comment #23) > Hi Zach. Thanks again for taking over the work on this. Now that the crash is fixed and the patches essentially accepted, I'm curious how that improves the WPT tests and what's happening with the testcase of 1402219? As seen in the WPT metadata changes in [D190758](https://phabricator.services.mozilla.com/D190758#change-zC5Cxr6SHutZ), we get 4 tests passing with the changeset as-is (with a couple more passes not yet marked in `css/css-animations/animation-base-response-001.html` and `css/css-animations/keyframes-unrelated-custom-property.html`), though none in Interop 2023 yet. I think the main reason for not getting more passes so far is that most of the Properties and Values animation WPTs check assert that custom properties have a particular value, rather than using reftests, so probably bug 1811897 will need to be fixes in order to get most of them passing. Unfortunately, the changeset also introduces a large number of regressions, as well as several crashes (see try run \<https://treeherder.mozilla.org/push-health/push?repo=try&revision=3433c4cc3ba33031d0f32551185f67f1f64ca6bc&tab=tests&testGroup=pr&selectedTest=csscss-animationsanimation-base-response-001html&selectedTaskId=&selectedJobName=\>), which will probably need to be resolved before the changeset can land.
Bug 1846516 Comment 24 Edit History
Note: The actual edited comment in the bug view page will always show the original commenter’s name and original timestamp.
(In reply to Frédéric Wang from comment #20) > (In reply to Zach Hoffman [:zrhoffman] from comment #17) > > Sorry for not responding sooner. It was crashing in at-property-animation.html when trying to check if a custom property is a shorthand <https://phabricator.services.mozilla.com/D191059?vs=on&id=793059#C7129453NL498> by passing the property's `mID` to `nsCSSProps::IsShorthand` (which fails on an assert in `nsCSSProps::IsShorthand` <https://searchfox.org/mozilla-central/rev/78a2c17cc80680a5a82446e4ce7c45a73b935383/layout/style/nsCSSProps.h#70-71> , because that assert did not consider `eCSSPropertyExtra_variable` a valid variant. > > Just to comment on this, instead of tweaking the assert to accept `eCSSPropertyExtra_variable`, my approach was to modify the callers of `nsCSSProps::IsShorthand` to handle `eCSSPropertyExtra_variable` separately (e.g. my change to the MOZ_ASSERT in `nsTransitionManager::ConsiderInitiatingTransition`). If you instead choose to just fix the assert so it can handle `eCSSPropertyExtra_variable`, some of the changes I made to avoid calling `nsCSSProps::IsShorthand` with `eCSSPropertyExtra_variable` can probably be removed. > > Also, technically CSS variables are all longhand properties, so the method should probably return false. But `eCSSPropertyExtra_variable` is the last value at https://searchfox.org/mozilla-central/source/layout/style/nsCSSPropertyID.h.in#35 so I believe your current version returns true. So calling `nsCSSProps::IsShorthand` with `eCSSPropertyExtra_variable` could make things wrong (for example the assert in ConsiderInitiatingTransition I just mentioned would cause a crash when we make custom properties transitionable). Thanks, `nsCSSProps::IsShorthand` is fixed now WRT behavior for `eCSSPropertyExtra_variable` in [D191059](https://phabricator.services.mozilla.com/D191059), though we have yet to remove unnecessary `nsCSSProps::IsShorthand()` calls and avoid `eCSSPropertyExtra_variable` being passed to it. (In reply to Frédéric Wang from comment #23) > Hi Zach. Thanks again for taking over the work on this. Now that the crash is fixed and the patches essentially accepted, I'm curious how that improves the WPT tests and what's happening with the testcase of 1402219? As seen in the WPT metadata changes in [D190758](https://phabricator.services.mozilla.com/D190758#change-zC5Cxr6SHutZ), we get 4 tests passing with the changeset as-is (with a couple more passes not yet marked in `css/css-animations/animation-base-response-001.html` and `css/css-animations/keyframes-unrelated-custom-property.html`), though none in Interop 2023 yet. I think the main reason for not getting more passes so far is that most of the Properties and Values animation WPTs check assert that custom properties have a particular value, rather than using reftests, so probably bug 1811897 will need to be fixes in order to get most of them passing. Unfortunately, the changeset also introduces a large number of regressions, as well as several crashes (see try run \<[[https://treeherder.mozilla.org/push-health/push?repo=try&revision=3433c4cc3ba33031d0f32551185f67f1f64ca6bc&tab=tests&testGroup=pr&selectedTest=csscss-animationsanimation-base-response-001html&selectedTaskId=&selectedJobName=](https://treeherder.mozilla.org/push-health/push?repo=try&revision=3433c4cc3ba33031d0f32551185f67f1f64ca6bc&tab=tests&testGroup=pr&selectedTest=csscss-animationsanimation-base-response-001html&selectedTaskId=&selectedJobName=)>), which will probably need to be resolved before the changeset can land.
(In reply to Frédéric Wang from comment #20) > (In reply to Zach Hoffman [:zrhoffman] from comment #17) > > Sorry for not responding sooner. It was crashing in at-property-animation.html when trying to check if a custom property is a shorthand <https://phabricator.services.mozilla.com/D191059?vs=on&id=793059#C7129453NL498> by passing the property's `mID` to `nsCSSProps::IsShorthand` (which fails on an assert in `nsCSSProps::IsShorthand` <https://searchfox.org/mozilla-central/rev/78a2c17cc80680a5a82446e4ce7c45a73b935383/layout/style/nsCSSProps.h#70-71> , because that assert did not consider `eCSSPropertyExtra_variable` a valid variant. > > Just to comment on this, instead of tweaking the assert to accept `eCSSPropertyExtra_variable`, my approach was to modify the callers of `nsCSSProps::IsShorthand` to handle `eCSSPropertyExtra_variable` separately (e.g. my change to the MOZ_ASSERT in `nsTransitionManager::ConsiderInitiatingTransition`). If you instead choose to just fix the assert so it can handle `eCSSPropertyExtra_variable`, some of the changes I made to avoid calling `nsCSSProps::IsShorthand` with `eCSSPropertyExtra_variable` can probably be removed. > > Also, technically CSS variables are all longhand properties, so the method should probably return false. But `eCSSPropertyExtra_variable` is the last value at https://searchfox.org/mozilla-central/source/layout/style/nsCSSPropertyID.h.in#35 so I believe your current version returns true. So calling `nsCSSProps::IsShorthand` with `eCSSPropertyExtra_variable` could make things wrong (for example the assert in ConsiderInitiatingTransition I just mentioned would cause a crash when we make custom properties transitionable). Thanks, `nsCSSProps::IsShorthand` is fixed now WRT behavior for `eCSSPropertyExtra_variable` in [D191059](https://phabricator.services.mozilla.com/D191059), though we have yet to remove unnecessary `nsCSSProps::IsShorthand()` calls and avoid `eCSSPropertyExtra_variable` being passed to it. (In reply to Frédéric Wang from comment #23) > Hi Zach. Thanks again for taking over the work on this. Now that the crash is fixed and the patches essentially accepted, I'm curious how that improves the WPT tests and what's happening with the testcase of 1402219? As seen in the WPT metadata changes in [D190758](https://phabricator.services.mozilla.com/D190758#change-zC5Cxr6SHutZ), we get 4 tests passing with the changeset as-is (with a couple more passes not yet marked in `css/css-animations/animation-base-response-001.html` and `css/css-animations/keyframes-unrelated-custom-property.html`), though none in Interop 2023 yet. I think the main reason for not getting more passes so far is that most of the Properties and Values animation WPTs check assert that custom properties have a particular value, rather than using reftests, so probably bug 1811897 will need to be fixes in order to get most of them passing. Unfortunately, the changeset also introduces a large number of regressions, as well as several crashes (see try run <[https://treeherder.mozilla.org/push-health/push?repo=try&revision=3433c4cc3ba33031d0f32551185f67f1f64ca6bc&tab=tests&testGroup=pr&selectedTest=csscss-animationsanimation-base-response-001html&selectedTaskId=&selectedJobName=](https://treeherder.mozilla.org/push-health/push?repo=try&revision=3433c4cc3ba33031d0f32551185f67f1f64ca6bc&tab=tests&testGroup=pr&selectedTest=csscss-animationsanimation-base-response-001html&selectedTaskId=&selectedJobName=)>), which will probably need to be resolved before the changeset can land.