Bug 1210796 - Part 11: Disable clicking on keyframes because we can’t calculate correct current time since the animation detail handles the animation progress not time. r=pbro draft
authorDaisuke Akatsuka <dakatsuka@mozilla.com>
Tue, 18 Apr 2017 12:15:56 +0900
changeset 564124 8c3c8034d13d3b56519c6c2fa115e822dc185b0c
parent 564123 38bf08f87bb0414c211099d1b4e85092baefd9ae
child 564125 153619c923b9a15576610f1eef813c1651fc0e2d
push id54524
push userbmo:dakatsuka@mozilla.com
push dateTue, 18 Apr 2017 09:24:06 +0000
reviewerspbro
bugs1210796
milestone55.0a1
Bug 1210796 - Part 11: Disable clicking on keyframes because we can’t calculate correct current time since the animation detail handles the animation progress not time. r=pbro MozReview-Commit-ID: JhXNREIyH9X
devtools/client/animationinspector/components/animation-details.js
devtools/client/animationinspector/components/animation-timeline.js
devtools/client/animationinspector/components/keyframes.js
devtools/client/animationinspector/test/browser.ini
devtools/client/animationinspector/test/browser_animation_keyframe_click_to_set_time.js
devtools/client/themes/animationinspector.css
--- a/devtools/client/animationinspector/components/animation-details.js
+++ b/devtools/client/animationinspector/components/animation-details.js
@@ -21,18 +21,16 @@ const L10N =
  * This includes information about timing, easing, keyframes, animated
  * properties.
  *
  * @param {Object} serverTraits The list of server-side capabilities.
  */
 function AnimationDetails(serverTraits) {
   EventEmitter.decorate(this);
 
-  this.onFrameSelected = this.onFrameSelected.bind(this);
-
   this.keyframeComponents = [];
   this.serverTraits = serverTraits;
 }
 
 exports.AnimationDetails = AnimationDetails;
 
 AnimationDetails.prototype = {
   // These are part of frame objects but are not animated properties. This
@@ -48,17 +46,16 @@ AnimationDetails.prototype = {
     this.unrender();
     this.containerEl = null;
     this.serverTraits = null;
     this.progressIndicatorEl = null;
   },
 
   unrender: function () {
     for (let component of this.keyframeComponents) {
-      component.off("frame-selected", this.onFrameSelected);
       component.destroy();
     }
     this.keyframeComponents = [];
 
     while (this.containerEl.firstChild) {
       this.containerEl.firstChild.remove();
     }
   },
@@ -202,21 +199,16 @@ AnimationDetails.prototype = {
     this.dummyAnimation =
       new this.win.Animation(new this.win.KeyframeEffect(null, null, timing), null);
 
     // Useful for tests to know when rendering of all animation detail UIs
     // have been completed.
     this.emit("animation-detail-rendering-completed");
   }),
 
-  onFrameSelected: function (e, args) {
-    // Relay the event up, it's needed in parents too.
-    this.emit(e, args);
-  },
-
   renderAnimatedPropertiesHeader: function () {
     // Add animated property header.
     const headerEl = createNode({
       parent: this.containerEl,
       attributes: { "class": "animated-properties-header" }
     });
 
     // Add progress tick container.
@@ -289,17 +281,16 @@ AnimationDetails.prototype = {
       let keyframesComponent = new Keyframes();
       keyframesComponent.init(framesEl);
       keyframesComponent.render({
         keyframes: this.tracks[propertyName],
         propertyName: propertyName,
         animation: this.animation,
         animationType: animationTypes[propertyName]
       });
-      keyframesComponent.on("frame-selected", this.onFrameSelected);
       this.keyframeComponents.push(keyframesComponent);
     }
   },
 
   renderProgressIndicator: function () {
     // The wrapper represents the area which the indicator is displayable.
     const progressIndicatorWrapperEl = createNode({
       parent: this.containerEl,
--- a/devtools/client/animationinspector/components/animation-timeline.js
+++ b/devtools/client/animationinspector/components/animation-timeline.js
@@ -53,17 +53,16 @@ function AnimationsTimeline(inspector, s
 
   this.onAnimationStateChanged = this.onAnimationStateChanged.bind(this);
   this.onScrubberMouseDown = this.onScrubberMouseDown.bind(this);
   this.onScrubberMouseUp = this.onScrubberMouseUp.bind(this);
   this.onScrubberMouseOut = this.onScrubberMouseOut.bind(this);
   this.onScrubberMouseMove = this.onScrubberMouseMove.bind(this);
   this.onAnimationSelected = this.onAnimationSelected.bind(this);
   this.onWindowResize = this.onWindowResize.bind(this);
-  this.onFrameSelected = this.onFrameSelected.bind(this);
   this.onTimelineDataChanged = this.onTimelineDataChanged.bind(this);
 
   EventEmitter.decorate(this);
 }
 
 exports.AnimationsTimeline = AnimationsTimeline;
 
 AnimationsTimeline.prototype = {
@@ -267,17 +266,16 @@ AnimationsTimeline.prototype = {
   unrender: function () {
     for (let animation of this.animations) {
       animation.off("changed", this.onAnimationStateChanged);
     }
     this.stopAnimatingScrubber();
     TimeScale.reset();
     this.destroySubComponents("targetNodes");
     this.destroySubComponents("timeBlocks");
-    this.details.off("frame-selected", this.onFrameSelected);
     this.details.unrender();
     this.animationsEl.innerHTML = "";
     this.off("timeline-data-changed", this.onTimelineDataChanged);
   },
 
   onWindowResize: function () {
     // Don't do anything if the root element has a width of 0
     if (this.rootWrapperEl.offsetWidth === 0) {
@@ -329,22 +327,18 @@ AnimationsTimeline.prototype = {
       this.animationDetailEl.dataset.defaultDisplayStyle;
     yield this.details.render(animation);
     this.onTimelineDataChanged(null, { time: this.currentTime || 0 });
     this.animationAnimationNameEl.textContent = getFormattedAnimationTitle(animation);
     this.emit("animation-selected", animation);
   }),
 
   /**
-   * When a frame gets selected, move the scrubber to the corresponding position
+   * When move the scrubber to the corresponding position
    */
-  onFrameSelected: function (e, {x}) {
-    this.moveScrubberTo(x, true);
-  },
-
   onScrubberMouseDown: function (e) {
     this.moveScrubberTo(e.pageX);
     this.win.addEventListener("mouseup", this.onScrubberMouseUp);
     this.win.addEventListener("mouseout", this.onScrubberMouseOut);
     this.win.addEventListener("mousemove", this.onScrubberMouseMove);
 
     // Prevent text selection while dragging.
     e.preventDefault();
@@ -468,17 +462,16 @@ AnimationsTimeline.prototype = {
       // Draw the animation time block.
       let timeBlock = new AnimationTimeBlock();
       timeBlock.init(timeBlockEl);
       timeBlock.render(animation);
       this.timeBlocks.push(timeBlock);
 
       timeBlock.on("selected", this.onAnimationSelected);
     }
-    this.details.on("frame-selected", this.onFrameSelected);
 
     // Use the document's current time to position the scrubber (if the server
     // doesn't provide it, hide the scrubber entirely).
     // Note that because the currentTime was sent via the protocol, some time
     // may have gone by since then, and so the scrubber might be a bit late.
     if (!documentCurrentTime) {
       this.scrubberEl.style.display = "none";
     } else {
--- a/devtools/client/animationinspector/components/keyframes.js
+++ b/devtools/client/animationinspector/components/keyframes.js
@@ -16,35 +16,31 @@ const {ProgressGraphHelper, appendPathEl
 let LINEAR_GRADIENT_ID_COUNTER = 0;
 
 /**
  * UI component responsible for displaying a list of keyframes.
  * Also, shows a graphical graph for the animation progress of one iteration.
  */
 function Keyframes() {
   EventEmitter.decorate(this);
-  this.onClick = this.onClick.bind(this);
 }
 
 exports.Keyframes = Keyframes;
 
 Keyframes.prototype = {
   init: function (containerEl) {
     this.containerEl = containerEl;
 
     this.keyframesEl = createNode({
       parent: this.containerEl,
       attributes: {"class": "keyframes"}
     });
-
-    this.containerEl.addEventListener("click", this.onClick);
   },
 
   destroy: function () {
-    this.containerEl.removeEventListener("click", this.onClick);
     this.keyframesEl.remove();
     this.containerEl = this.keyframesEl = this.animation = null;
   },
 
   render: function ({keyframes, propertyName, animation, animationType}) {
     this.keyframes = keyframes;
     this.propertyName = propertyName;
     this.animation = animation;
@@ -94,32 +90,16 @@ Keyframes.prototype = {
           "class": "frame",
           "style": `left:${frame.offset * 100}%;`,
           "data-offset": frame.offset,
           "data-property": propertyName,
           "title": frame.value
         }
       });
     }
-  },
-
-  onClick: function (e) {
-    // If the click happened on a frame, tell our parent about it.
-    if (!e.target.classList.contains("frame")) {
-      return;
-    }
-
-    e.stopPropagation();
-    this.emit("frame-selected", {
-      animation: this.animation,
-      propertyName: this.propertyName,
-      offset: parseFloat(e.target.dataset.offset),
-      value: e.target.getAttribute("title"),
-      x: e.target.offsetLeft + e.target.closest(".frames").offsetLeft
-    });
   }
 };
 
 /**
  * Render a graph representing the progress of the animation over one iteration.
  * @param {Element} parentEl - Parent element of this appended path element.
  * @param {Number} duration - Duration of one iteration.
  * @param {Number} minSegmentDuration - Minimum segment duration.
--- a/devtools/client/animationinspector/test/browser.ini
+++ b/devtools/client/animationinspector/test/browser.ini
@@ -22,17 +22,16 @@ support-files =
   !/devtools/client/shared/test/test-actor-registry.js
   !/devtools/client/shared/test/test-actor.js
 
 [browser_animation_animated_properties_displayed.js]
 [browser_animation_click_selects_animation.js]
 [browser_animation_controller_exposes_document_currentTime.js]
 skip-if = os == "linux" && !debug # Bug 1234567
 [browser_animation_empty_on_invalid_nodes.js]
-[browser_animation_keyframe_click_to_set_time.js]
 [browser_animation_keyframe_markers.js]
 [browser_animation_mutations_with_same_names.js]
 [browser_animation_panel_exists.js]
 [browser_animation_participate_in_inspector_update.js]
 [browser_animation_playerFronts_are_refreshed.js]
 [browser_animation_playerWidgets_appear_on_panel_init.js]
 [browser_animation_playerWidgets_target_nodes.js]
 [browser_animation_pseudo_elements.js]
deleted file mode 100644
--- a/devtools/client/animationinspector/test/browser_animation_keyframe_click_to_set_time.js
+++ /dev/null
@@ -1,49 +0,0 @@
-/* vim: set ts=2 et sw=2 tw=80: */
-/* Any copyright is dedicated to the Public Domain.
- http://creativecommons.org/publicdomain/zero/1.0/ */
-
-"use strict";
-
-// Test that animated properties' keyframes can be clicked, and that doing so
-// sets the current time in the timeline.
-
-add_task(function* () {
-  yield addTab(URL_ROOT + "doc_keyframes.html");
-  let {panel} = yield openAnimationInspector();
-  let timeline = panel.animationsTimelineComponent;
-  let {scrubberEl} = timeline;
-
-  // XXX: The scrollbar is placed in the timeline in such a way that it causes
-  // the animations to be slightly offset with the header when it appears.
-  // So for now, let's hide the scrollbar. Bug 1229340 should fix this.
-  timeline.animationsEl.style.overflow = "hidden";
-
-  info("Click on the first keyframe of the first animated property");
-  yield clickKeyframe(panel, "background-color", 0);
-
-  info("Make sure the scrubber stopped moving and is at the right position");
-  yield assertScrubberMoving(panel, false);
-  checkScrubberPos(scrubberEl, 0);
-
-  info("Click on a keyframe in the middle");
-  yield clickKeyframe(panel, "transform", 2);
-
-  info("Make sure the scrubber is at the right position");
-  checkScrubberPos(scrubberEl, 50);
-});
-
-function* clickKeyframe(panel, property, index) {
-  let keyframeComponent = getKeyframeComponent(panel, property);
-  let keyframeEl = getKeyframeEl(panel, property, index);
-
-  let onSelect = keyframeComponent.once("frame-selected");
-  EventUtils.sendMouseEvent({type: "click"}, keyframeEl,
-                            keyframeEl.ownerDocument.defaultView);
-  yield onSelect;
-}
-
-function checkScrubberPos(scrubberEl, pos) {
-  let newPos = Math.round(parseFloat(scrubberEl.style.left));
-  let expectedPos = Math.round(pos);
-  is(newPos, expectedPos, `The scrubber is at ${pos}%`);
-}
--- a/devtools/client/themes/animationinspector.css
+++ b/devtools/client/themes/animationinspector.css
@@ -619,17 +619,16 @@ body {
 }
 
 .keyframes .frame {
   position: absolute;
   top: 50%;
   width: 0;
   height: 0;
   background-color: inherit;
-  cursor: pointer;
 }
 
 .keyframes .frame::before {
   content: "";
   display: block;
   transform:
     translateX(calc(var(--keyframes-marker-size) * -.5))
     /* The extra pixel on the Y axis is so that markers are centered on the