Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2895913002:
blink/css: Read animation keyframe rules in UA sheets for shadow DOM. (Closed)
Descriptionblink/css: Read animation keyframe rules in UA sheets for shadow DOM.
The keyframe rules declared in UA stylesheets parsed by the
CSSDefaultStyleSheets are not available in StyleResolver when querying
for these rules.
Ideally these rules should be added to the Document's
ScopedStyleResolver, with the visibility for rules for the shadow DOM
limited to UA provided rules. This change is only a temporary fix.
BUG=724975
Patch Set 1 #
Messages
Total messages: 15 (3 generated)
khushalsagar@chromium.org changed reviewers: + kochi@chromium.org
rune@opera.com changed reviewers: + rune@opera.com
Animations (with keyframes) are named. Won't adding them to the UA stylesheet pollute the namespace for author animations? If this is for animating UA shadows, they should probably be scoped to the UA shadow in question. Note that keyframes in a shadow scope also applies to the shadows host element. Could you provide some more context?
On 2017/05/22 11:54:42, rune wrote: > Animations (with keyframes) are named. Won't adding them to the UA stylesheet > pollute the namespace for author animations? From what I can tell, keyframes from UA stylesheets are not being added to the ScopedStyleResolvers at all, only the ones from author stylesheets. And this patch shouldn't change that behaviour. The change here is that now only the UA shadow elements will have access to animations in UA stylesheets. They still can not access author animations. And elements outside the UA shadow DOM will still not have access to the UA stylesheet animations. Do you think this would still be a concern? > > If this is for animating UA shadows, they should probably be scoped to the UA > shadow in question. Note that keyframes in a shadow scope also applies to the > shadows host element. > > Could you provide some more context? The change that is trying to add animation to UA shadow is here: https://codereview.chromium.org/2898543002/diff/60001/third_party/WebKit/Sour.... Look at "-internal-media-controls-promo-throbbing". Is there an alternate way to achieve this?
As I suggested in private chat (and rune also suggested in the issue tracker), could you try another CL to add a stylesheet with the keyframes rule to each video UA shadow?
On 2017/05/24 10:42:01, kochi wrote: > As I suggested in private chat (and rune also suggested > in the issue tracker), could you try another CL to > add a stylesheet with the keyframes rule to each video > UA shadow? That won't work out-of-the-box atm, though, because of: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/resol... We need to either accept the current patch in some form, or put some substantial effort into it.
On 2017/05/24 11:32:33, rune wrote: > On 2017/05/24 10:42:01, kochi wrote: > > As I suggested in private chat (and rune also suggested > > in the issue tracker), could you try another CL to > > add a stylesheet with the keyframes rule to each video > > UA shadow? > > That won't work out-of-the-box atm, though, because of: > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/resol... > > We need to either accept the current patch in some form, or put some substantial > effort into it. Uhm, sorry, yes putting the key-frames in a scoped sheet would work since ScopedStyleResolver is retrieved off the UA shadow scope for FindKeyframesRules(). That's also a bit strange to separate animation declaration and keyframes rule in UA sheet and scoped sheet. Long/medium term, I think we should try move all to a scoped sheet.
On 2017/05/24 11:45:56, rune wrote: > On 2017/05/24 11:32:33, rune wrote: > > On 2017/05/24 10:42:01, kochi wrote: > > > As I suggested in private chat (and rune also suggested > > > in the issue tracker), could you try another CL to > > > add a stylesheet with the keyframes rule to each video > > > UA shadow? > > > > That won't work out-of-the-box atm, though, because of: > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/resol... > > > > We need to either accept the current patch in some form, or put some > substantial > > effort into it. > > Uhm, sorry, yes putting the key-frames in a scoped sheet would work since > ScopedStyleResolver is retrieved off the UA shadow scope for > FindKeyframesRules(). That's also a bit strange to separate animation > declaration and keyframes rule in UA sheet and scoped sheet. Long/medium term, I > think we should try move all to a scoped sheet. Thanks rune@ and kochi@. I was able to get it working by adding a style element with the keyframe rule and the style for the element to animate in the video element's shadow DOM. You can take a look at that here: https://codereview.chromium.org/2898543002/diff/80001/third_party/WebKit/Sour....
On 2017/05/25 02:01:23, Khushal wrote: > On 2017/05/24 11:45:56, rune wrote: > > On 2017/05/24 11:32:33, rune wrote: > > > On 2017/05/24 10:42:01, kochi wrote: > > > > As I suggested in private chat (and rune also suggested > > > > in the issue tracker), could you try another CL to > > > > add a stylesheet with the keyframes rule to each video > > > > UA shadow? > > > > > > That won't work out-of-the-box atm, though, because of: > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/resol... > > > > > > We need to either accept the current patch in some form, or put some > > substantial > > > effort into it. > > > > Uhm, sorry, yes putting the key-frames in a scoped sheet would work since > > ScopedStyleResolver is retrieved off the UA shadow scope for > > FindKeyframesRules(). That's also a bit strange to separate animation > > declaration and keyframes rule in UA sheet and scoped sheet. Long/medium term, > I > > think we should try move all to a scoped sheet. > > Thanks rune@ and kochi@. I was able to get it working by adding a style element > with the keyframe rule and the style for the element to animate in the video > element's shadow DOM. You can take a look at that here: > https://codereview.chromium.org/2898543002/diff/80001/third_party/WebKit/Sour.... Cool, thanks! rune@ - the odd part of it is that ::-webkit-* pseudo element can be styled as usual element via CSS, and using UA shadow for the pseudo element is an implementation detail. So people wonder why @keyframes doesn't apply to such pseudo elements. In that standpoint, this change may not be so bad as I initially thought, though code-wise I think the other CL (putting @keyframes in per-shadow <style>) looks much saner for me for now. For UA styles, we may move all styles for those pseudo elements into per-shadow scoped stylesheets, but matching author rules using ::-webkit-* against such UA shadows is very hard to deprecate in a short term. So even after we move all styles for pseudo elements in per-shadow stylesheet, I think the C++ code for matching such rules will not get any cleaner. (as far as I understand, but I could be wrong - Rune knows much better).
On 2017/05/25 05:43:33, kochi wrote: > On 2017/05/25 02:01:23, Khushal wrote: > > On 2017/05/24 11:45:56, rune wrote: > > > On 2017/05/24 11:32:33, rune wrote: > > > > On 2017/05/24 10:42:01, kochi wrote: > > > > > As I suggested in private chat (and rune also suggested > > > > > in the issue tracker), could you try another CL to > > > > > add a stylesheet with the keyframes rule to each video > > > > > UA shadow? > > > > > > > > That won't work out-of-the-box atm, though, because of: > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/resol... > > > > > > > > We need to either accept the current patch in some form, or put some > > > substantial > > > > effort into it. > > > > > > Uhm, sorry, yes putting the key-frames in a scoped sheet would work since > > > ScopedStyleResolver is retrieved off the UA shadow scope for > > > FindKeyframesRules(). That's also a bit strange to separate animation > > > declaration and keyframes rule in UA sheet and scoped sheet. Long/medium > term, > > I > > > think we should try move all to a scoped sheet. > > > > Thanks rune@ and kochi@. I was able to get it working by adding a style > element > > with the keyframe rule and the style for the element to animate in the video > > element's shadow DOM. You can take a look at that here: > > > https://codereview.chromium.org/2898543002/diff/80001/third_party/WebKit/Sour.... > > Cool, thanks! > > rune@ - the odd part of it is that ::-webkit-* pseudo element can be styled > as usual element via CSS, and using UA shadow for the pseudo element is an > implementation detail. So people wonder why @keyframes doesn't apply to > such pseudo elements. > > In that standpoint, this change may not be so bad as I initially thought, > though code-wise I think the other CL (putting @keyframes in per-shadow > <style>) looks much saner for me for now. > > For UA styles, we may move all styles for those pseudo elements into > per-shadow scoped stylesheets, but matching author rules using ::-webkit-* > against such UA shadows is very hard to deprecate in a short term. > So even after we move all styles for pseudo elements in per-shadow stylesheet, > I think the C++ code for matching such rules will not get any cleaner. > (as far as I understand, but I could be wrong - Rune knows much better). Public holiday in Norway today. Will comment tomorrow.
On 2017/05/25 18:43:26, rune wrote: > On 2017/05/25 05:43:33, kochi wrote: > > On 2017/05/25 02:01:23, Khushal wrote: > > > On 2017/05/24 11:45:56, rune wrote: > > > > On 2017/05/24 11:32:33, rune wrote: > > > > > On 2017/05/24 10:42:01, kochi wrote: > > > > > > As I suggested in private chat (and rune also suggested > > > > > > in the issue tracker), could you try another CL to > > > > > > add a stylesheet with the keyframes rule to each video > > > > > > UA shadow? > > > > > > > > > > That won't work out-of-the-box atm, though, because of: > > > > > > > > > > > > > > > > > > > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/resol... > > > > > > > > > > We need to either accept the current patch in some form, or put some > > > > substantial > > > > > effort into it. > > > > > > > > Uhm, sorry, yes putting the key-frames in a scoped sheet would work since > > > > ScopedStyleResolver is retrieved off the UA shadow scope for > > > > FindKeyframesRules(). That's also a bit strange to separate animation > > > > declaration and keyframes rule in UA sheet and scoped sheet. Long/medium > > term, > > > I > > > > think we should try move all to a scoped sheet. > > > > > > Thanks rune@ and kochi@. I was able to get it working by adding a style > > element > > > with the keyframe rule and the style for the element to animate in the video > > > element's shadow DOM. You can take a look at that here: > > > > > > https://codereview.chromium.org/2898543002/diff/80001/third_party/WebKit/Sour.... > > > > Cool, thanks! > > > > rune@ - the odd part of it is that ::-webkit-* pseudo element can be styled > > as usual element via CSS, and using UA shadow for the pseudo element is an > > implementation detail. So people wonder why @keyframes doesn't apply to > > such pseudo elements. > > > > In that standpoint, this change may not be so bad as I initially thought, > > though code-wise I think the other CL (putting @keyframes in per-shadow > > <style>) looks much saner for me for now. > > > > For UA styles, we may move all styles for those pseudo elements into > > per-shadow scoped stylesheets, but matching author rules using ::-webkit-* > > against such UA shadows is very hard to deprecate in a short term. > > So even after we move all styles for pseudo elements in per-shadow stylesheet, > > I think the C++ code for matching such rules will not get any cleaner. > > (as far as I understand, but I could be wrong - Rune knows much better). > > Public holiday in Norway today. Will comment tomorrow. No worries - have a nice holiday!
On 2017/05/25 05:43:33, kochi wrote: > https://codereview.chromium.org/2898543002/diff/80001/third_party/WebKit/Sour.... > > Cool, thanks! > > rune@ - the odd part of it is that ::-webkit-* pseudo element can be styled > as usual element via CSS, and using UA shadow for the pseudo element is an > implementation detail. So people wonder why @keyframes doesn't apply to > such pseudo elements. Do you mean that you can do this from an author sheet: ::-webkit-* { animation: my-anim } But it won't pick up this from the same author sheet?: @keyframes my-anim { } > In that standpoint, this change may not be so bad as I initially thought, > though code-wise I think the other CL (putting @keyframes in per-shadow > <style>) looks much saner for me for now. I'm worried https://codereview.chromium.org/2898543002/diff/80001/third_party/WebKit/Sour... causes DCHECK fails in https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/resol... > For UA styles, we may move all styles for those pseudo elements into > per-shadow scoped stylesheets, but matching author rules using ::-webkit-* > against such UA shadows is very hard to deprecate in a short term. Yes. I think limiting the ::-webkit-* syntax to the author-piercing and not use it for the UA styling of the UA shadow is a step in the right direction. > So even after we move all styles for pseudo elements in per-shadow stylesheet, > I think the C++ code for matching such rules will not get any cleaner. > (as far as I understand, but I could be wrong - Rune knows much better). You mean because we still need to pull in the ::-webkit-* piercing rules from the outer scope?
On 2017/05/26 14:27:53, rune wrote: > On 2017/05/25 05:43:33, kochi wrote: > > > > https://codereview.chromium.org/2898543002/diff/80001/third_party/WebKit/Sour.... > > > > Cool, thanks! > > > > rune@ - the odd part of it is that ::-webkit-* pseudo element can be styled > > as usual element via CSS, and using UA shadow for the pseudo element is an > > implementation detail. So people wonder why @keyframes doesn't apply to > > such pseudo elements. > > Do you mean that you can do this from an author sheet: > > ::-webkit-* { > animation: my-anim > } > > But it won't pick up this from the same author sheet?: > > @keyframes my-anim { > } Right, the reason that "my-anim" isn't picked up is just Blink's implementation detail that such pseudo element is implemented via UA shadow. For some other pseudo elements (e.g. ::after), animation can be applicable. > > In that standpoint, this change may not be so bad as I initially thought, > > though code-wise I think the other CL (putting @keyframes in per-shadow > > <style>) looks much saner for me for now. > > I'm worried > https://codereview.chromium.org/2898543002/diff/80001/third_party/WebKit/Sour... > causes DCHECK fails in > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/css/resol... Aha, we need more work :) > > For UA styles, we may move all styles for those pseudo elements into > > per-shadow scoped stylesheets, but matching author rules using ::-webkit-* > > against such UA shadows is very hard to deprecate in a short term. > > Yes. I think limiting the ::-webkit-* syntax to the author-piercing and not use > it for the UA styling of the UA shadow is a step in the right direction. > > > So even after we move all styles for pseudo elements in per-shadow stylesheet, > > I think the C++ code for matching such rules will not get any cleaner. > > (as far as I understand, but I could be wrong - Rune knows much better). > > You mean because we still need to pull in the ::-webkit-* piercing rules from > the outer scope? Yes, for author sheets.
Description was changed from ========== blink/css: Read animation keyframe rules in UA sheets for shadow DOM. The keyframe rules declared in UA stylesheets parsed by the CSSDefaultStyleSheets are not available in StyleResolver when querying for these rules. Ideally these rules should be added to the Document's ScopedStyleResolver, with the visibility for rules for the shadow DOM limited to UA provided rules. This change is only a temporary fix. BUG=724975 ========== to ========== blink/css: Read animation keyframe rules in UA sheets for shadow DOM. The keyframe rules declared in UA stylesheets parsed by the CSSDefaultStyleSheets are not available in StyleResolver when querying for these rules. Ideally these rules should be added to the Document's ScopedStyleResolver, with the visibility for rules for the shadow DOM limited to UA provided rules. This change is only a temporary fix. BUG=724975 ========== |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
