Bug 1018269: Handle Shadow DOM in keyframes lookup. r=heycam
authorEmilio Cobos Álvarez <emilio@crisal.io>
Tue, 01 May 2018 07:09:12 +0200
changeset 472564 69109fe28dc09b17132ba2a793202f9fda66d5f4
parent 472563 d6ab3049b56061284e9e0e10410588766c7245d9
child 472565 587a7161ba5c84abce9c9fe1e39d8c50054c2f36
push id1728
push userjlund@mozilla.com
push dateMon, 18 Jun 2018 21:12:27 +0000
treeherdermozilla-release@c296fde26f5f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersheycam
bugs1018269
milestone61.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1018269: Handle Shadow DOM in keyframes lookup. r=heycam MozReview-Commit-ID: AeUmsOHOUYR
layout/style/ServoBindingList.h
layout/style/ServoStyleSet.cpp
layout/style/ServoStyleSet.h
layout/style/nsAnimationManager.cpp
servo/components/style/stylist.rs
servo/ports/geckolib/glue.rs
testing/web-platform/meta/MANIFEST.json
testing/web-platform/tests/css/css-scoping/keyframes-001.html
testing/web-platform/tests/css/css-scoping/keyframes-002.html
--- a/layout/style/ServoBindingList.h
+++ b/layout/style/ServoBindingList.h
@@ -130,16 +130,17 @@ SERVO_BINDING_FUNC(Servo_StyleSet_FlushS
 SERVO_BINDING_FUNC(Servo_StyleSet_SetAuthorStyleDisabled, void,
                    RawServoStyleSetBorrowed set,
                    bool author_style_disabled)
 SERVO_BINDING_FUNC(Servo_StyleSet_NoteStyleSheetsChanged, void,
                    RawServoStyleSetBorrowed set,
                    mozilla::OriginFlags changed_origins)
 SERVO_BINDING_FUNC(Servo_StyleSet_GetKeyframesForName, bool,
                    RawServoStyleSetBorrowed set,
+                   RawGeckoElementBorrowed element,
                    nsAtom* name,
                    nsTimingFunctionBorrowed timing_function,
                    RawGeckoKeyframeListBorrowedMut keyframe_list)
 SERVO_BINDING_FUNC(Servo_StyleSet_GetFontFaceRules, void,
                    RawServoStyleSetBorrowed set,
                    RawGeckoFontFaceRuleListBorrowedMut list)
 SERVO_BINDING_FUNC(Servo_StyleSet_GetCounterStyleRule,
                    const RawServoCounterStyleRule*,
--- a/layout/style/ServoStyleSet.cpp
+++ b/layout/style/ServoStyleSet.cpp
@@ -1176,24 +1176,24 @@ ServoStyleSet::AssertTreeIsClean()
   DocumentStyleRootIterator iter(mDocument);
   while (Element* root = iter.GetNextStyleRoot()) {
     Servo_AssertTreeIsClean(root);
   }
 }
 #endif
 
 bool
-ServoStyleSet::GetKeyframesForName(nsAtom* aName,
+ServoStyleSet::GetKeyframesForName(const Element& aElement,
+                                   nsAtom* aName,
                                    const nsTimingFunction& aTimingFunction,
                                    nsTArray<Keyframe>& aKeyframes)
 {
-  // TODO(emilio): This may need to look at the element itself for handling
-  // @keyframes properly in Shadow DOM.
   MOZ_ASSERT(!StylistNeedsUpdate());
   return Servo_StyleSet_GetKeyframesForName(mRawSet.get(),
+                                            &aElement,
                                             aName,
                                             &aTimingFunction,
                                             &aKeyframes);
 }
 
 nsTArray<ComputedKeyframeValues>
 ServoStyleSet::GetComputedKeyframeValuesFor(
   const nsTArray<Keyframe>& aKeyframes,
--- a/layout/style/ServoStyleSet.h
+++ b/layout/style/ServoStyleSet.h
@@ -336,17 +336,18 @@ public:
    * Resolve style for the given element, and return it as a
    * ComputedStyle.
    *
    * FIXME(emilio): Is there a point in this after bug 1367904?
    */
   inline already_AddRefed<ComputedStyle>
     ResolveServoStyle(dom::Element* aElement);
 
-  bool GetKeyframesForName(nsAtom* aName,
+  bool GetKeyframesForName(const dom::Element& aElement,
+                           nsAtom* aName,
                            const nsTimingFunction& aTimingFunction,
                            nsTArray<Keyframe>& aKeyframes);
 
   nsTArray<ComputedKeyframeValues>
   GetComputedKeyframeValuesFor(const nsTArray<Keyframe>& aKeyframes,
                                dom::Element* aElement,
                                const mozilla::ComputedStyle* aStyle);
 
--- a/layout/style/nsAnimationManager.cpp
+++ b/layout/style/nsAnimationManager.cpp
@@ -352,24 +352,26 @@ PopExistingAnimation(const nsAtom* aName
 class MOZ_STACK_CLASS ServoCSSAnimationBuilder final {
 public:
   explicit ServoCSSAnimationBuilder(const ComputedStyle* aComputedStyle)
     : mComputedStyle(aComputedStyle)
   {
     MOZ_ASSERT(aComputedStyle);
   }
 
-  bool BuildKeyframes(nsPresContext* aPresContext,
+  bool BuildKeyframes(const Element& aElement,
+                      nsPresContext* aPresContext,
                       nsAtom* aName,
                       const nsTimingFunction& aTimingFunction,
                       nsTArray<Keyframe>& aKeyframes)
   {
     ServoStyleSet* styleSet = aPresContext->StyleSet();
     MOZ_ASSERT(styleSet);
-    return styleSet->GetKeyframesForName(aName,
+    return styleSet->GetKeyframesForName(aElement,
+                                         aName,
                                          aTimingFunction,
                                          aKeyframes);
   }
   void SetKeyframes(KeyframeEffectReadOnly& aEffect,
                     nsTArray<Keyframe>&& aKeyframes)
   {
     aEffect.SetKeyframes(Move(aKeyframes), mComputedStyle);
   }
@@ -478,17 +480,18 @@ BuildAnimation(nsPresContext* aPresConte
                uint32_t animIdx,
                ServoCSSAnimationBuilder& aBuilder,
                nsAnimationManager::CSSAnimationCollection* aCollection)
 {
   MOZ_ASSERT(aPresContext);
 
   nsAtom* animationName = aStyleDisplay.GetAnimationName(animIdx);
   nsTArray<Keyframe> keyframes;
-  if (!aBuilder.BuildKeyframes(aPresContext,
+  if (!aBuilder.BuildKeyframes(*aTarget.mElement,
+                               aPresContext,
                                animationName,
                                aStyleDisplay.GetAnimationTimingFunction(animIdx),
                                keyframes)) {
     return nullptr;
   }
 
   TimingParams timing =
     TimingParamsFromCSSParams(aStyleDisplay.GetAnimationDuration(animIdx),
--- a/servo/components/style/stylist.rs
+++ b/servo/components/style/stylist.rs
@@ -1416,24 +1416,62 @@ impl Stylist {
             CaseSensitivity::CaseSensitive => {},
         }
 
         let hash = id.get_hash();
         self.any_applicable_rule_data(element, |data| data.mapped_ids.might_contain_hash(hash))
     }
 
     /// Returns the registered `@keyframes` animation for the specified name.
-    ///
-    /// FIXME(emilio): This needs to account for the element rules.
     #[inline]
-    pub fn get_animation(&self, name: &Atom) -> Option<&KeyframesAnimation> {
-        self.cascade_data
-            .iter_origins()
-            .filter_map(|(d, _)| d.animations.get(name))
-            .next()
+    pub fn get_animation<'a, E>(
+        &'a self,
+        name: &Atom,
+        element: E,
+    ) -> Option<&'a KeyframesAnimation>
+    where
+        E: TElement + 'a,
+    {
+        macro_rules! try_find_in {
+            ($data:expr) => {
+                if let Some(animation) = $data.animations.get(name) {
+                    return Some(animation);
+                }
+            }
+        }
+
+        // NOTE(emilio): We implement basically what Blink does for this case,
+        // which is [1] as of this writing.
+        //
+        // See [2] for the spec discussion about what to do about this. WebKit's
+        // behavior makes a bit more sense off-hand, but it's way more complex
+        // to implement, and it makes value computation having to thread around
+        // the cascade level, which is not great. Also, it breaks if you inherit
+        // animation-name from an element in a different tree.
+        //
+        // See [3] for the bug to implement whatever gets resolved, and related
+        // bugs for a bit more context.
+        //
+        // [1]: https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/css/resolver/style_resolver.cc?l=1267&rcl=90f9f8680ebb4a87d177f3b0833372ae4e0c88d8
+        // [2]: https://github.com/w3c/csswg-drafts/issues/1995
+        // [3]: https://bugzil.la/1458189
+        if let Some(shadow) = element.shadow_root() {
+            try_find_in!(shadow.style_data());
+        }
+
+        if let Some(shadow) = element.containing_shadow() {
+            try_find_in!(shadow.style_data());
+        } else {
+            try_find_in!(self.cascade_data.author);
+        }
+
+        try_find_in!(self.cascade_data.user);
+        try_find_in!(self.cascade_data.user_agent.cascade_data);
+
+        None
     }
 
     /// Computes the match results of a given element against the set of
     /// revalidation selectors.
     pub fn match_revalidation_selectors<E, F>(
         &self,
         element: E,
         bloom: Option<&BloomFilter>,
--- a/servo/ports/geckolib/glue.rs
+++ b/servo/ports/geckolib/glue.rs
@@ -4763,27 +4763,28 @@ fn fill_in_missing_keyframe_values(
             }
         }
     }
 }
 
 #[no_mangle]
 pub unsafe extern "C" fn Servo_StyleSet_GetKeyframesForName(
     raw_data: RawServoStyleSetBorrowed,
+    element: RawGeckoElementBorrowed,
     name: *mut nsAtom,
     inherited_timing_function: nsTimingFunctionBorrowed,
     keyframes: RawGeckoKeyframeListBorrowedMut,
 ) -> bool {
-    debug_assert!(keyframes.len() == 0,
-                  "keyframes should be initially empty");
-
+    debug_assert!(keyframes.len() == 0, "keyframes should be initially empty");
+
+    let element = GeckoElement(element);
     let data = PerDocumentStyleData::from_ffi(raw_data).borrow();
     let name = Atom::from_raw(name);
 
-    let animation = match data.stylist.get_animation(&name) {
+    let animation = match data.stylist.get_animation(&name, element) {
         Some(animation) => animation,
         None => return false,
     };
 
     let global_style_data = &*GLOBAL_STYLE_DATA;
     let guard = global_style_data.shared_lock.read();
 
     let mut properties_set_at_current_offset = LonghandIdSet::new();
--- a/testing/web-platform/meta/MANIFEST.json
+++ b/testing/web-platform/meta/MANIFEST.json
@@ -316307,16 +316307,28 @@
     ]
    ],
    "css/css-scoping/host-functional-descendant-invalidation.html": [
     [
      "/css/css-scoping/host-functional-descendant-invalidation.html",
      {}
     ]
    ],
+   "css/css-scoping/keyframes-001.html": [
+    [
+     "/css/css-scoping/keyframes-001.html",
+     {}
+    ]
+   ],
+   "css/css-scoping/keyframes-002.html": [
+    [
+     "/css/css-scoping/keyframes-002.html",
+     {}
+    ]
+   ],
    "css/css-scoping/shadow-cascade-order-001.html": [
     [
      "/css/css-scoping/shadow-cascade-order-001.html",
      {}
     ]
    ],
    "css/css-scoping/slotted-invalidation.html": [
     [
@@ -512332,16 +512344,24 @@
   "css/css-scoping/host-nested-001.html": [
    "a0b74d2e6bf24e9142904a925f95e969d206db20",
    "reftest"
   ],
   "css/css-scoping/host-slotted-001.html": [
    "918bd04b95a276c6035383f2fe4dcfe4274bceeb",
    "reftest"
   ],
+  "css/css-scoping/keyframes-001.html": [
+   "f0cda2f0f3d4093c7a24d283416c4fead20e597f",
+   "testharness"
+  ],
+  "css/css-scoping/keyframes-002.html": [
+   "748ba6ae7c4f34770652c3269b92ff004e71e542",
+   "testharness"
+  ],
   "css/css-scoping/reference/green-box.html": [
    "a736f68dc602c0fccab56ec5cc6234cb3298c88d",
    "support"
   ],
   "css/css-scoping/shadow-assign-dynamic-001.html": [
    "c57e0fd5aa5be63e1cadf65a4e382798c5e05ec4",
    "reftest"
   ],
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/css-scoping/keyframes-001.html
@@ -0,0 +1,42 @@
+<!doctype html>
+<title>CSS Test: @keyframes applies in the shadow tree.</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<link rel="help" href="https://drafts.csswg.org/css-scoping/#selectors-data-model">
+<!--
+  Beware of https://github.com/w3c/csswg-drafts/issues/1995 potentially, but
+  unlikely, changing the expected result of this test.
+-->
+<style>
+#in-document {
+  width: 100px;
+  height: 100px;
+  background: blue;
+  animation: myanim 10s infinite;
+}
+</style>
+<div id="host"><div id="in-document"></div></div>
+<script>
+test(function() {
+  host.attachShadow({ mode: "open" }).innerHTML = `
+    <style>
+      @keyframes myanim {
+        from { background: red; }
+        to { background: green; }
+      }
+      #in-shadow {
+        width: 100px;
+        height: 100px;
+        background: blue;
+        animation: myanim 10s infinite;
+      }
+    </style>
+    <slot></slot>
+    <div id="in-shadow"></div>
+  `;
+
+  assert_equals(document.getElementById('in-document').getAnimations().length, 0);
+  assert_equals(host.shadowRoot.getElementById('in-shadow').getAnimations().length, 1);
+}, "@keyframes applies in the shadow tree")
+</script>
new file mode 100644
--- /dev/null
+++ b/testing/web-platform/tests/css/css-scoping/keyframes-002.html
@@ -0,0 +1,34 @@
+<!doctype html>
+<title>CSS Test: @keyframes from the document don't apply in the shadow tree.</title>
+<script src="/resources/testharness.js"></script>
+<script src="/resources/testharnessreport.js"></script>
+<link rel="author" title="Emilio Cobos Álvarez" href="mailto:emilio@crisal.io">
+<link rel="help" href="https://drafts.csswg.org/css-scoping/#selectors-data-model">
+<!--
+  Beware of https://github.com/w3c/csswg-drafts/issues/1995 potentially, but
+  unlikely, changing the expected result of this test.
+-->
+<style>
+@keyframes myanim {
+  from { background: red; }
+  to { background: red; }
+}
+</style>
+<div id="host"></div>
+<script>
+test(function() {
+  host.attachShadow({ mode: "open" }).innerHTML = `
+    <style>
+      #in-shadow {
+        width: 100px;
+        height: 100px;
+        background: blue;
+        animation: myanim 10s infinite;
+      }
+    </style>
+    <div id="in-shadow"></div>
+  `;
+
+  assert_equals(host.shadowRoot.getElementById('in-shadow').getAnimations().length, 0);
+}, "@keyframes from the document don't apply in the shadow tree");
+</script>