1134568 - Add a list of predefined easing functions to the cubic-bezier tooltip

Status: RESOLVED FIXED

(DevTools :: Inspector, defect)

Description

[Patrick Brosset <:pbro>]

Attachment: timing-functions-adobe-edge-animate.gif

There are many animation/transition timing functions that could be helpful to developers if they were readily available in a list, when using the cubic-bezier tooltip in the inspector.

Apart from the built-in named functions like linear, ease-in, ease-out, ease-in-out, there are many functions that people commonly use but don't have built-in names: bounce, elastic, sine, ...

Adobe Edge Animation provides a nice UI for this, I've attached a GIF that shows it.

Comment 1

[Patrick Brosset <:pbro>]

Ana Tudor suggested this site: http://easings.net/
That's a nice list of easing functions right there.

Comment 2

[John Giannakos [:jtgi]]

I've started working on this.
Progress: http://i.imgur.com/8Eej9xM.gif

Comment 3

[Brian Grinstead [:bgrins]]

Looks great!  I've assigned the bug to you.

Comment 4

[John Giannakos [:jtgi]]

Attachment: Initial Cubic-Bezier Presets Mock.

Any thoughts on the attached design? Its just a PSD at the moment, but should be achievable with enough svg twiddling I believe.

Some thoughts:
- There's a bit of extra space above and below the graph to make sure dragging the handles is still comfortable.
- # of presets, if we start to go over we could either opt for scroll bars vertical or horizontal, or some carousel style arrows if we want to get fancy.
- We can include 'linear' as the first preset in any given category and remove the need to have a top level category with 1 preset in it.
- The design attempts to mimic the rest of the dev tools where a precedent exists. The dark theme would simply follow existing inverted conventions.

Questions:
1. Is it okay to assume all interaction with this panel will result in a cubic-bezier setting? ie. Even if you selected `linear` the animation set would still be of type cubic-bezier?
2. Is loading this many svgs okay? Haven't built them yet and don't have a great idea of perf implications, if any. Are static assets jpg, png, etc an option?
3. Too large? There's no other widgets this big –not sure if there are some max-size requirements or otherwise. The size as it stands now is mostly constrained on attempting to match font-sizes, margins, padding with other parts of the devtool chrome as well as size of the graph with enough room to move handles.
4. Too many presets? Some of the differences like quartic, quintic are kinda negligible. Might be fine with less.

Comment 5

[Patrick Brosset <:pbro>]

Attachment: easeInOutBack.png

(In reply to John Giannakos [:jtgi] from comment #4)
> Created attachment 8567482 [details]
> Initial Cubic-Bezier Presets Mock.
> 
> Any thoughts on the attached design? Its just a PSD at the moment, but
> should be achievable with enough svg twiddling I believe.
That looks great! I like the categories (tabs) and the little previews.
I'm not so sure we need the header part though, since it's easy to know what setting is selected in the list below. Also because the more room we can keep to drag the handles, the better.
In fact, already today, with nothing else in the tooltip than the graph, it's hard to set certain functions (see the attached screenshot where I've reproduced the easeInOutBack setting, the handles are out of the tooltip area).

> Some thoughts:
> - There's a bit of extra space above and below the graph to make sure
> dragging the handles is still comfortable.
See previous comment. Perhaps we can make the graph smaller?
> - # of presets, if we start to go over we could either opt for scroll bars
> vertical or horizontal, or some carousel style arrows if we want to get
> fancy.
There are probably a tone of UX approaches we could take to have more presets in there if we need too (there could be only one preview svg area, and hovering over preset names would preview the corresponding curve there, or we could also get rid of all the preview svg areas, and just preview on hover on the main graph, in a different color than the current curve). I'm happy to start with a small number of presets and keep your current design proposal.
Brian can probably also give his feedback here.
> - We can include 'linear' as the first preset in any given category and
> remove the need to have a top level category with 1 preset in it.
Yes!
> - The design attempts to mimic the rest of the dev tools where a precedent
> exists. The dark theme would simply follow existing inverted conventions.
Perfect.

> Questions:
> 1. Is it okay to assume all interaction with this panel will result in a
> cubic-bezier setting? ie. Even if you selected `linear` the animation set
> would still be of type cubic-bezier?
There's an advantage to keeping the few built-in CSS function names I think (linear, ease, ease-in, ease-out, ease-in-out): it helps users learn that those names exist and can be used in stylesheets. So I'd like it if you'd select one of those in the list, that would enter the corresponding name in the property value below.
> 2. Is loading this many svgs okay? Haven't built them yet and don't have a
> great idea of perf implications, if any. Are static assets jpg, png, etc an
> option?
Static images are an option of course, but we have to consider @2x images too, and dark theme... this can quickly become a nightmare. SVG (or canvas for that matter) would be a much better fit. With SVG, if I'm not wrong, this is just a single <path> element, I don't see any perf implications there.
In fact, one other option would be to simply instantiate new BezierCanvas widgets to render the previews, see:
http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/CubicBezierWidget.js#209
> 3. Too large? There's no other widgets this big –not sure if there are some
> max-size requirements or otherwise. The size as it stands now is mostly
> constrained on attempting to match font-sizes, margins, padding with other
> parts of the devtool chrome as well as size of the graph with enough room to
> move handles.
I would say developers tend to have big screens, but that's just an assumption and we have no numbers to back this out. In fact, we just landed a change to our telemetry system that will tell us our users' screen sizes, but right now we don't know.  I would say, if the tooltip still fits on a 13'' display, we're probably ok. The screenshot you attached looks fine too me. I wouldn't worry too much about it being bigger than the other widgets in devtools. 
As I said earlier, another option is to make the graph a bit smaller (tooltip thinner, but maybe as high, to leave space for dragging the handles), then make the categories text only, and preview on hover in the main graph. So we have options if needed.
> 4. Too many presets? Some of the differences like quartic, quintic are kinda
> negligible. Might be fine with less.
Yeah, let's start with a small list, it's better to add more later.
Can you provide the list of presets you intend to add?

Comment 6

[Patrick Brosset <:pbro>]

Attachment: cubic-bezier-tooltip-chrome.gif

This just landed in chrome :)

Comment 7

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #5)
> > Some thoughts:
> > - There's a bit of extra space above and below the graph to make sure
> > dragging the handles is still comfortable.
> See previous comment. Perhaps we can make the graph smaller?
> > - # of presets, if we start to go over we could either opt for scroll bars
> > vertical or horizontal, or some carousel style arrows if we want to get
> > fancy.
> There are probably a tone of UX approaches we could take to have more
> presets in there if we need too (there could be only one preview svg area,
> and hovering over preset names would preview the corresponding curve there,
> or we could also get rid of all the preview svg areas, and just preview on
> hover on the main graph, in a different color than the current curve). I'm
> happy to start with a small number of presets and keep your current design
> proposal.
> Brian can probably also give his feedback here.

Going to forward the UX request onto Stephen.  See Comment 4 for more information.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #6)
> Created attachment 8567884 [details]
> cubic-bezier-tooltip-chrome.gif
> 
> This just landed in chrome :)

I like that, although I don't know how it would scale with the longer list of options.

Comment 8

[John Giannakos [:jtgi]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #5)
> I'm not so sure we need the header part though, since it's easy to know what
> setting is selected in the list below. Also because the more room we can
> keep to drag the handles, the better.

Yeah, good point re: header and vertical space. I'll wait for Stephens follow up on this. Since vertical space is a bit of an issue, it might be smart to have a more columnar layout. The listing could be left/right of the graph if need be.

> one other option would be to simply instantiate new BezierCanvas
> widgets to render the previews, see:
> http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/
> widgets/CubicBezierWidget.js#209

Good idea. This is the file I've been working in where I built the prototype above.

> Can you provide the list of presets you intend to add?

I intend to add every 2nd function that is available at http://easings.net.

I can see a couple categorization schemes.

# Option 1: Categorize based on function type

3 categories: Ease In, Ease Out, Ease In Out

Ease In
  Linear (linear)
  Default (ease-in)
  Quadratic (cubic-bezier)
  Quartic (cubic-bezier)
  Exponential (cubic-bezier)
  Circular (cubic-bezier)
  Back (Backward?) (cubic-bezier)

Ease Out
  Linear (linear)
  Default (ease-out)
  Quadratic (cubic-bezier)
  Quartic (cubic-bezier)
  Exponential (cubic-bezier)
  Circular (cubic-bezier)
  Back (Backward?) (cubic-bezier)

Ease In Out
  Linear (linear)
  Default (ease-in-out)
  Ease (ease) <- [1]
  Quadratic (cubic-bezier)
  Quartic (cubic-bezier)
  Exponential (cubic-bezier)
  Circular (cubic-bezier)
  Back (Backward?) (cubic-bezier)

[1] Ease is a bit hard to place. Its somewhere in between an ease-in-out and and ease-out. I placed it here.

Thoughts: In this scheme, 'Linear' can be a bit redundant, 'Default' perhaps isn't too clear, and 'Ease' is somewhat lost in the mix. Overall though, I think this approach is reasonable.

# Option 2: Same as #1 but add 'Defaults' category; Remove `Linear` and `Default` from each category.

Defaults
  Linear (linear)
  Ease (ease)
  Ease In (ease-in)
  Ease Out (ease-out)
  Ease In Out (ease-in-out)

Ease In
  ...

...

Thoughts: This is not too bad. A little strange maybe to have "Ease In, Out, In Out" as an item and a category but this could serve to better drive awareness of built in functions and remove redundancy.

# Option 3: Categorize based on preset type

Standard
  Linear (linear)
  Ease (ease)
  Ease In (ease-in)
  Ease Out (ease-out)
  Ease In Out (ease-in-out)

Cubic Bezier
  Ease In Quadratic
  ..
  Ease Out Quadratic
  ..
  Ease In Out Quadratic
  ..

Thoughts: This is perhaps a bit cleaner in terms of organization (no repeated linear, clearer what is a css default), but the cubic bezier list can grow long. It's probably not going to be great given the current mock I have but maybe better suited to a listing.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #6)
> This just landed in chrome :)
Nice. It would be easy to add presets to that and is quite compact. Does become a little hard to know what you have and haven't seen though. Previews are nice in that you can quickly scan things and know what you have and have not tried.

p.s. Would be sweet to get some markdown support on this.

Comment 9

[John Giannakos [:jtgi]]

Also, I'm not sure how the other `step` related functions should fit into this, if at all.

Comment 10

[John Giannakos [:jtgi]]

Attachment: add_presets_to_cubic_bezier.patch

Attaching what I've done to date before I get too far in with hopes on early feedback on code style, placement and the works.

The patch contains:
 - Sample definition of presets as per easings.net + defaults.
 - Logic for toggling a preset category and their corresponding presets.
 - Rendering coordinates to display when a preset is selected.

Thank you~

Comment 11

[Patrick Brosset <:pbro>]

(In reply to John Giannakos [:jtgi] from comment #8)
> Yeah, good point re: header and vertical space. I'll wait for Stephens
> follow up on this. Since vertical space is a bit of an issue, it might be
> smart to have a more columnar layout. The listing could be left/right of the
> graph if need be.
That sounds great.

> # Option 1: Categorize based on function type
> # Option 2: Same as #1 but add 'Defaults' category; Remove `Linear` and
> `Default` from each category.
> # Option 3: Categorize based on preset type
Option 1 makes the most sense to me, but I would take 'linear' out of the categories and make it an empty category (it would show at the same level as the other categories, but selecting it would apply the linear function, not display the list of functions). But that's just my opinion. #2 also sounds reasonable.
I'll let you choose between those 2.

> p.s. Would be sweet to get some markdown support on this.
This will be possible in BZ 6.0 (we're currently running 4.2 something)

Comment 12

[Patrick Brosset <:pbro>]

(In reply to John Giannakos [:jtgi] from comment #9)
> Also, I'm not sure how the other `step` related functions should fit into
> this, if at all.
The steps functions should not be part of this widget, at least not right now. We need a way to visually represent them first. This would be very useful (I doubt everyone knows how they work by heart) but will be better treated in another bug.

Comment 13

[Patrick Brosset <:pbro>]

Comment on attachment 8568172 [details] [diff] [review]
add_presets_to_cubic_bezier.patch

Review of attachment 8568172 [details] [diff] [review]:
-----------------------------------------------------------------

Definitely a great start!
I have made some minor comments in the code below.

I have also applied the patch locally and gave it a try, here are some comments, in no particular order:
- the tooltip has the same size as before for me, so you have to scroll to get to the categories,
- a 'pointer' cursor would help knowing that you can click on the presets,
- it's kind of hard to see when you change category, the category name becomes highlighted, which is good, but since the list of presets is exactly the same, it's easy to miss that something changed. Having the previews like chrome would help there,
- when a preset is selected, it should also be highlighted like the category name, right now, the background color is a paler blue,
- a columnar layout, as you proposed, would indeed work better I believe,
- if you select a preset, and then change the function by dragging the handles, then the category and preset should appear unselected (removing the highlight color should be enough).

You're on the right track. Thanks for working on this.

::: browser/devtools/shared/widgets/CubicBezierWidget.js
@@ +198,5 @@
>   * sent a CubicBezier object
>   */
>  function CubicBezierWidget(parent, coordinates=PREDEFINED["ease-in-out"]) {
>    this.parent = parent;
> +  let {curve, p1, p2, presetPane} = this._initMarkup();

I doesn't look like presetPane is returned by _initMarkup.

@@ +228,5 @@
>    this.timingPreview = new TimingFunctionPreviewWidget(parent);
>  
> +  //TODO: What's the best way to update the display when an
> +  //item has been clicked in the preset widget? EventEmitter?
> +  //call it directly? Probably not passing the whole ref.

EventEmitter seems to me like the best solution. This helps decoupling and reusability.

@@ +439,5 @@
>    }
>  };
>  
> +//TODO: Is there some default widgets I should be using? I saw
> +//      the generic lists but couldn't find any other examples.

What do you mean by default widgets? This looks ok to me.

@@ +441,5 @@
>  
> +//TODO: Is there some default widgets I should be using? I saw
> +//      the generic lists but couldn't find any other examples.
> +//TODO: Is there no CSS sass like variables? What if ff changes
> +//      theme color later? Update all of these things?

Our light/dark themes are based on css variables. You can see them in light-theme.css and dark-theme.css in the :root selector.
As long as theme-switching.js is included in the parent window (and it already is), then the switch will work automatically.
So if you need to define colors in your CSS file, make sure you reuse the corresponding variables when possible.

@@ +444,5 @@
> +//TODO: Is there no CSS sass like variables? What if ff changes
> +//      theme color later? Update all of these things?
> +//TODO: How can I make this tooltip larger/wider, seems fixed~~~
> +//TODO: Anything I should be concerned about in regards to screen res
> +//      and resizing?

We should make sure the UI you create works both on retina and non-retina pixel density screens.

@@ +445,5 @@
> +//      theme color later? Update all of these things?
> +//TODO: How can I make this tooltip larger/wider, seems fixed~~~
> +//TODO: Anything I should be concerned about in regards to screen res
> +//      and resizing?
> +//TODO: This should probably be in another file.

Yes, you can create a new file in the widgets folder that only has the presets.

@@ +446,5 @@
> +//TODO: How can I make this tooltip larger/wider, seems fixed~~~
> +//TODO: Anything I should be concerned about in regards to screen res
> +//      and resizing?
> +//TODO: This should probably be in another file.
> +//TODO: l18n for these strings? How?

We don't really need l10n for these strings. They are technical words, and some of them are actually CSS keywords.

@@ +493,5 @@
> + * and binding basic event handlers. Maybe something simpler that
> + * can be done.
> + */
> +//TODO: Only need `cubicBezierDisplay` for calling set coordinates(), 
> +//      maybe some better way to share this method. Events?

Yes, you can get rid of the cubicBezierDisplay parameter if you make this new widget "observable":

EventEmitter.decorate(this);
...
this.emit("new-coordinates", coordinates);

Or something like this.

@@ +547,5 @@
> +
> +    let allCategoriesNodeList = presetPane.querySelectorAll(".category");
> +    let allPresetsNodeList = presetPane.querySelectorAll(".preset");
> +
> +    //Just say no to nodeLists.

This isn't needed if you use for..of loops.
for..of loops can iterate on nodeLists.
If you prefer to use .forEach, you could also do this instead:
let allCategories = [...allCategoriesNodeList];

@@ +566,5 @@
> +    category.id = categoryLabel;
> +    category.classList.add("category");
> +
> +    let categoryDisplayLabel = this._normalizeCategoryLabel(categoryLabel);
> +    category.appendChild(doc.createTextNode(categoryDisplayLabel));

category.textContent = categoryDisplayLabel;
should work too.

@@ +608,5 @@
> +  _normalizePresetLabel: function(categoryLabel, presetLabel) {
> +    return presetLabel.replace(categoryLabel + "-", "");
> +  },
> +
> +  _initEvents: function() {

Can you also define a _removeEvents method that removes all the added event listeners, and call it in destroy?

@@ +609,5 @@
> +    return presetLabel.replace(categoryLabel + "-", "");
> +  },
> +
> +  _initEvents: function() {
> +    this.categories.forEach(category => {

That's where you can use the for..of loops on nodeLists.

@@ +618,5 @@
> +      preset.addEventListener("click", this._onPresetClick);
> +    });
> +  },
> +
> +  _onPresetClick: function(event) { 

nit: trailing whitespace

@@ +619,5 @@
> +    });
> +  },
> +
> +  _onPresetClick: function(event) { 
> +    this.display.coordinates = event.target.coordinates;

Emit an event here instead.

@@ +623,5 @@
> +    this.display.coordinates = event.target.coordinates;
> +    this.activePreset = event.target.id;
> +  },
> +
> +  _onCategoryClick: function(event) { 

nit: trailing whitespace

@@ +634,5 @@
> +    this._activePresetList = presetList;
> +  },
> +
> +  set activeCategory(categoryId) {
> +    let category = this.presetPane.querySelector("#" + categoryId);

Why pass the ID only here? Passing the DOM node directly would avoid having to look it up again later.

@@ +645,5 @@
> +  get activeCategory() {
> +    return this._activeCategory;
> +  },
> +
> +  set activePreset(presetId) {

Why pass the ID only here? Passing the DOM node directly would avoid having to look it up again later.

@@ +782,5 @@
>  
>    return false;
>  }
> +
> +function swapClassName(className, from, to) {

Please add a jsdoc comment for this function.

@@ +783,5 @@
>    return false;
>  }
> +
> +function swapClassName(className, from, to) {
> +  if(from !== null) {

nit: we usually insert a space between if and (

@@ +787,5 @@
> +  if(from !== null) {
> +    from.classList.remove(className);
> +  }
> +
> +  if(to !== null) {

ditto

::: browser/devtools/shared/widgets/cubic-bezier.css
@@ +4,5 @@
>  
>  /* Based on Lea Verou www.cubic-bezier.com
>     See https://github.com/LeaVerou/cubic-bezier */
>  
> +/* =====================

nit: We usually don't have these kinds of big separator comments in the devtools codebase.
A simple /* ... */ comment line should be enough.

@@ +14,2 @@
>  .coordinate-plane {
> +  position:relative;

nit: please add a space after the :

@@ +153,5 @@
> +/* ================================
> + *
> + *  PRESET WIDGET
> + *
> + * ===============================*/

Global comments about the preset widget css code:
- no need for bug separatorcomment,
- please add an empty space after : in properties,
- please try and avoid hard coding colors if their equivalent exist in dark/light-theme.css

Comment 14

[John Giannakos [:jtgi]]

Patrick, thanks for the feedback! 

I should have been clear that this was not intended to be a UI design in response to our discussion above but just an implementation of the key behaviours we are after.

I will respond to feedback and resubmit.

Comment 15

[John Giannakos [:jtgi]]

Couple things:

1. I can't seem to be able to adjust the width or height of the tooltip. Only the width or height of the tooltip's contents, which creates horizontal or vertical scrollbars. I presume this is set somewhere else but my efforts to find this somewhere else have all failed. Any ideas?

2. Is there an example of using the light/dark CSS variables? I wasn't able to find one.

Thank you.

Comment 16

[Patrick Brosset <:pbro>]

(In reply to John Giannakos [:jtgi] from comment #15)
> Couple things:
> 
> 1. I can't seem to be able to adjust the width or height of the tooltip.
> Only the width or height of the tooltip's contents, which creates horizontal
> or vertical scrollbars. I presume this is set somewhere else but my efforts
> to find this somewhere else have all failed. Any ideas?
The only place the tooltip dimension is set is here: http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/Tooltip.js#794 and it's done by setting the size of the iframe inside.
If that doesn't work for some reason, you can also try to call this.setSize(w,h): http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/widgets/Tooltip.js#271

> 2. Is there an example of using the light/dark CSS variables? I wasn't able
> to find one.
There is a few example in this css file: http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/animationinspector.css#12
Search for var( in the file.

Comment 17

[Brian Grinstead [:bgrins]]

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #16)
> > 2. Is there an example of using the light/dark CSS variables? I wasn't able
> > to find one.
> There is a few example in this css file:
> http://mxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/
> animationinspector.css#12
> Search for var( in the file.

And the available variables are listed here: https://dxr.mozilla.org/mozilla-central/source/browser/themes/shared/devtools/light-theme.css#9

Comment 18

[John Giannakos [:jtgi]]

Attachment: cubic-bezier-presets-candidate-1

Update: I've got an updated patch I'm starting to polish up in response to @pbrosset's code review and our design discussion above.

This implementation is a side by side layout to maximize that vertical space. We can make it smaller yet, if we'd like to.

To keep the preset panel a reasonable width I've just gone with 3 top level categories. We could make it wider, make it a drop down, accordion, or others if we'd like more categories in the future, or if opposed to this.

If the presets overflow the height of the tooltip, the categories and everything else will stay fixed and the preset previews will get a scroll bar.

As for remaining work, just some small behavior and some alignment tasks to work through.

Let me know if you have any thoughts / criticisms with this approach.

Comment 19

[John Giannakos [:jtgi]]

Attachment: add_presets_to_cubic_bezier.patch

I've gone ahead and implemented the remaining tasks.

In this patch:
- Canvas preset previews for of each preset available.
- Basic scrollable panel for presets.
- Proper use of CSS vars (sans one rule). Works in both light+dark theme.
- If user has defined a animation-timing-function that we have a preset defined for (linear, ease, cubic-bezier(*)), the tool tip will display that onOpen and set state accordingly.
- If user chooses a HTML predefined preset (linear, ease-in) from the menu that rule will then be written back to the variables view as oppose to `cubic-bezier` before.
- Code organization and formatting as per @pbrosset's feedback.

Testing: 
- I have only done manual testing on a Macbook Pro Retina, not sure how to go about testing on other machines. I'll try and set up a build environment on my Win8.1 machine.

Comment 20

[John Giannakos [:jtgi]]

Attachment: Added missing CubicBezierPresets.js

Added missing CubicBezierPresets.js

Comment 21

[Patrick Brosset <:pbro>]

(In reply to John Giannakos [:jtgi] from comment #18)
> This implementation is a side by side layout to maximize that vertical
> space. We can make it smaller yet, if we'd like to.
> 
> To keep the preset panel a reasonable width I've just gone with 3 top level
> categories. We could make it wider, make it a drop down, accordion, or
> others if we'd like more categories in the future, or if opposed to this.
> 
> If the presets overflow the height of the tooltip, the categories and
> everything else will stay fixed and the preset previews will get a scroll
> bar.
That looks great to me! I like it a lot.
I was off last week, but will get to the code review early this week (you removed the F? flag when attaching the second version of the patch, I guess this was accidental).

Comment 22

[John Giannakos [:jtgi]]

Thanks Patrick. Yes, I forgot to put the F? flag, actually I wasn't sure it was necessary in addition to the needinfo at the bottom of the attachment. Will do for next time.

In other news, I found a regression today. It's easy to repro.
1. Open nightly.
2. Open the bezier tooltip.
3. Click anywhere in the coordinate plane or drag handles before choosing any type of preset on the left.
Expected: updated graph.
Actual: (uncaught exception: Wrong coordinate at 0(2.24)), handles detach.

After clicking any preset, the problem never reoccurs, even if closed and reopened.

I'll fix this. I assume a small oversight somewhere in my first-time init of the canvas.

Comment 23

[John Giannakos [:jtgi]]

Attachment: add_presets_to_cubic_bezier.patch

Apologies for the multiple updates but I've updated the patch with a fix for the problem mentioned above.

If interested:
Problem was the canvas' `curveBoundingBox` is initialized to [0, 0, 0, 0] and not updated until a drag event occurs where getBoundingClientRect() is called. Since `onCurveClick` calculates its offset based on the curveBoundingBox's x-position and since the canvas' x-position in the new layout is no longer 0 but rather ~50%, the left offset was incorrect. This was never a problem before because the canvas' x-position was in fact at the left most coordinate (0).

Solution was just to call `getBoundingClientRect()` in `onCurveClick` to get fresh positions, like what is done in `onPointMouseDown`.

P.S. I'm still figuring out bugzilla convention re: making things obsolete, what flags to use and how often I should post here so please let me know if I am doing anything odd.

Comment 24

[Patrick Brosset <:pbro>]

(In reply to John Giannakos [:jtgi] from comment #23)
> P.S. I'm still figuring out bugzilla convention re: making things obsolete,
> what flags to use and how often I should post here so please let me know if
> I am doing anything odd.
No you're fine :)
You should upload a new patch as soon as you either need help, or feedback, or a formal code review (once you think the patch is done). When doing so, you should mark the old version of the patch as obsolete (which is what you did so, great).

Comment 25

[Patrick Brosset <:pbro>]

Comment on attachment 8575023 [details] [diff] [review]
add_presets_to_cubic_bezier.patch

Review of attachment 8575023 [details] [diff] [review]:
-----------------------------------------------------------------

This looks great. I haven't yet applied the patch locally and played with the tooltip, but I'll do this next. 
I've looked at the code though, and it's easy to read and simple. I also really liked that screenshot you uploaded recently, so we're getting there!
I guess the next big step will be making sure the existing tests still pass (they will surely need some adapting), and adding new tests.

::: browser/devtools/shared/widgets/CubicBezierPresets.js
@@ +1,1 @@
> +/*

This new file needs the usual js file header we normally have:

/* This Source Code Form is subject to the terms of the Mozilla Public
 * License, v. 2.0. If a copy of the MPL was not distributed with this
 * file, You can obtain one at http://mozilla.org/MPL/2.0/. */

"use strict";

@@ +1,2 @@
> +/*
> + * Set of preset definitions for use with CubicBezierWidget 

nit: trailing white space.

@@ +9,5 @@
> +  "ease-in": [0.42, 0, 1, 1],
> +  "ease-out": [0, 0, 0.58, 1],
> +  "ease-in-out": [0.42, 0, 0.58, 1]
> +};
> +

nit: just one empty line is needed.

::: browser/devtools/shared/widgets/CubicBezierWidget.js
@@ +66,5 @@
>      return this.coordinates.slice(2);
>    },
>  
>    toString: function() {
> +    //Check first if current coords are html predefined

Maybe rephrase:
// Check first if current coords are one of the predefined CSS easing functions.

@@ +70,5 @@
> +    //Check first if current coords are html predefined
> +    let predefName = Object.keys(PREDEFINED)
> +                           .find(key => coordsAreEqual(PREDEFINED[key], this.coordinates));
> +
> +    return predefName ? predefName : 'cubic-bezier(' + this.coordinates + ')';

Or perhaps:
return predefName || 'cubic-bezier(' + this.coordinates + ')';

Btw, thanks for this change, that's a nice thing to add.

@@ +149,5 @@
>      for (let setting in settings) {
>        defaultSettings[setting] = settings[setting];
>      }
>  
> +    //Fix canvas refresh problem: 

nit: trailing whitespace, also, we usually insert a whitespace after //, before the actual comment string.
Also, can you explain what problem you're trying to solve with the code below?
That URL seems to mention performance problems, did you measure the perf improvement this change brings?
I'm not against this at all, just want to make sure there's a real benefit for doing it. And if there is, I think these 4 lines need more commenting.

@@ +472,5 @@
> + *
> + * Emits "new-coordinate" event along with the coordinates 
> + * whenever a preset is selected.
> + */
> +function CubicBezierPresetWidget(parent) {

Several formatting nits for this new class (for better consistency with the rest of the codebase):
- there are trailing whitespaces here and there,
- we usually don't insert an empty line after the prototype object definition,
- just one empty line between functions is enough,
- need an whitespace between // and the comment itself for line comments,
- please use let instead of var,
- we normally insert a whitespace between statements and the corresponding (. You missed it a couple of times if( while(

@@ +593,5 @@
> +
> +    curve.setAttribute("height", 55);
> +    curve.setAttribute("width", 55);
> +
> +    preset.bezierCanvas = new BezierCanvas(curve, bezier, [0.15, 0]);

Nice that you reused the BezierCanvas class here.

@@ +645,5 @@
> +    let maxDepth = 3;
> +    let node = event.target;
> +
> +    //event delegation may require a search upwards for the top level node
> +    while(!node.classList.contains("preset") && maxDepth > 0) {

Am I missing something? Why do you need this when you added exactly one click event listener per preset element? event.target should always be a preset element.

::: browser/devtools/shared/widgets/cubic-bezier.css
@@ +1,4 @@
>  /* This Source Code Form is subject to the terms of the Mozilla Public
>   * License, v. 2.0. If a copy of the MPL was not distributed with this
>   * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
>  

nit: A few trailing whitespaces in this file too.

Comment 26

[Patrick Brosset <:pbro>]

Some impressions after having applied the patch locally:
1) this is great! love it!
2) the <ENTER> and <ESC> keybindings still work as before, great.
3) I think we'll need some kind of keyboard support for the tooltip: arrow keys to move through presets, and maybe another to move through categories, but let's keep this for another bug (can you file a new bug for this, that depends on this one?)
4) the background gradient is looking weird for me (I'll attach a screen capture)
5) can you set width/height 100% and overflow hidden on the html,body selector to avoid any kind of scrollbars? Depending on the curve there may be a horizontal scrollbar that appears because the preview animation makes the circle move too far to the right (I'll attach a screen capture too).

Comment 27

[Patrick Brosset <:pbro>]

Attachment: cubic-bezier-tooltip.gif

Shows the background and the scrollbar problems.

Comment 28

[John Giannakos [:jtgi]]

Comment on attachment 8575023 [details] [diff] [review]
add_presets_to_cubic_bezier.patch

Review of attachment 8575023 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/shared/widgets/CubicBezierWidget.js
@@ +149,5 @@
>      for (let setting in settings) {
>        defaultSettings[setting] = settings[setting];
>      }
>  
> +    //Fix canvas refresh problem: 

Yeah, my comment + link is partially misleading. This isn't actually for sake of performance. It is the only way I could get the canvas to render without leaving behind artifacts. I'm really not sure what is causing this and why this specific combination works. I had expected clearRect(...) or `canvas.width = canvas.width` to clear these artifacts, but no luck. In fact, if any of these 4 lines are removed it either doesn't render, or I get artifacts. 

I'm quite inexperienced with canvases and I would prefer not to employ this fix but I'm not sure what else to try, I've spent a considerable amount of time debugging this. I presume it has something to do with the change in markup surrounding the canvas as it is not a problem in the current implementation.

I've attached screenshots as a follow up demonstrating the problem. Perhaps you've seen this before?

@@ +593,5 @@
> +
> +    curve.setAttribute("height", 55);
> +    curve.setAttribute("width", 55);
> +
> +    preset.bezierCanvas = new BezierCanvas(curve, bezier, [0.15, 0]);

Yeah, this worked out really great. Simplified things.

@@ +645,5 @@
> +    let maxDepth = 3;
> +    let node = event.target;
> +
> +    //event delegation may require a search upwards for the top level node
> +    while(!node.classList.contains("preset") && maxDepth > 0) {

The preset element has the following html

preset <- holds reference to coordinates
  preview
  label

I attach an event listener to `preset` which also enables any of its children to pick up click events. `event.target` will point to the DOM Node clicked even if it is a child of `preset`. Since I store the coordinates in `preset` as a part of `_initMarkup` I have to traverse upwards to find it.

Comment 29

[John Giannakos [:jtgi]]

Re: my comments above ^, I thought this would appear in a thread, hopefully you can infer the context.

(In reply to Patrick Brosset [:pbrosset] [:patrick] from comment #26)
> Some impressions after having applied the patch locally:
> 1) this is great! love it!

Awesome. I think it feels quite good overall. The previews are certainly a nice ux improvement in my opinion.

> 2) the <ENTER> and <ESC> keybindings still work as before, great.
> 3) I think we'll need some kind of keyboard support for the tooltip: arrow
> keys to move through presets, and maybe another to move through categories,
> but let's keep this for another bug (can you file a new bug for this, that
> depends on this one?)

Done: #1142255

> 4) the background gradient is looking weird for me (I'll attach a screen
> capture)

I can't repro this on my dev machine so I'm unable to confirm, but I believe in the latest patch below this is fixed. I removed the em based widths and set clearer boundaries on the repeating-linear-gradient. Is there an standard way to test this across Firefox versions and platforms?

Comment 30

[John Giannakos [:jtgi]]

Attachment: add_presets_to_cubic_bezier.patch

Patch updated based on feedback.

Summary of open issues from my perspective:

1. Have canvas refresh fix approved or dig into some other potential fix (line ~156). See next post for images of the problem.
2. Testing. I see `/fx-team/browser/devtools/shared/test/*-cubic-bezier-*. I'll update that and add new tests where necessary; is there anywhere else or other type of testing I should do?

Comment 31

[John Giannakos [:jtgi]]

Attachment: Canvas refresh problem

Attached is a demonstration of the canvas leaving artifacts if not cleared as is in the patch. 

The implementation for each screenshot can be seen immediately right of the graph.

Comment 32

[Patrick Brosset <:pbro>]

(In reply to John Giannakos [:jtgi] from comment #28)
> Comment on attachment 8575023 [details] [diff] [review]
> add_presets_to_cubic_bezier.patch
> 
> Review of attachment 8575023 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: browser/devtools/shared/widgets/CubicBezierWidget.js
> @@ +149,5 @@
> >      for (let setting in settings) {
> >        defaultSettings[setting] = settings[setting];
> >      }
> >  
> > +    //Fix canvas refresh problem: 
> 
> Yeah, my comment + link is partially misleading. This isn't actually for
> sake of performance. It is the only way I could get the canvas to render
> without leaving behind artifacts. I'm really not sure what is causing this
> and why this specific combination works. I had expected clearRect(...) or
> `canvas.width = canvas.width` to clear these artifacts, but no luck. In
> fact, if any of these 4 lines are removed it either doesn't render, or I get
> artifacts. 
> 
> I'm quite inexperienced with canvases and I would prefer not to employ this
> fix but I'm not sure what else to try, I've spent a considerable amount of
> time debugging this. I presume it has something to do with the change in
> markup surrounding the canvas as it is not a problem in the current
> implementation.
> I've attached screenshots as a follow up demonstrating the problem. Perhaps
> you've seen this before?
Thanks for the explanation and the screenshot attached in comment 31. So, the problem comes from the fact that the canvas context is scaled and translated in the BezierCanvas constructor. That's because it makes it easier to draw the various shapes afterwards. But it also means that now, the clearRect command applies only to the scaled and translated area, that's why just clearing from 0, 0 to width/height doesn't clear everything.
So in that respect, your fix is correct, only I would remove the URL from the comment and change the comment itself to something like:

    // Clear the canvas, making sure to clear the whole area by resetting the
    // transform first.
    this.ctx.save();
    this.ctx.setTransform(1, 0, 0, 1, 0, 0);
    this.ctx.clearRect(0, 0, this.canvas.width, this.canvas.height);
    this.ctx.restore();

> @@ +645,5 @@
> > +    let maxDepth = 3;
> > +    let node = event.target;
> > +
> > +    //event delegation may require a search upwards for the top level node
> > +    while(!node.classList.contains("preset") && maxDepth > 0) {
> 
> The preset element has the following html
> 
> preset <- holds reference to coordinates
>   preview
>   label
> 
> I attach an event listener to `preset` which also enables any of its
> children to pick up click events. `event.target` will point to the DOM Node
> clicked even if it is a child of `preset`. Since I store the coordinates in
> `preset` as a part of `_initMarkup` I have to traverse upwards to find it.
Sorry, my bad. But then you can use currentTarget instead:

  _onPresetClick: function(event) {
    let node = event.currentTarget;
    this.emit("new-coordinates", node.coordinates);
    this.activePreset = node;
  },

  _onCategoryClick: function(event) {
    this.activeCategory = event.currentTarget;
  },

Comment 33

[Patrick Brosset <:pbro>]

(In reply to John Giannakos [:jtgi] from comment #29)
> > 4) the background gradient is looking weird for me (I'll attach a screen
> > capture)
> 
> I can't repro this on my dev machine so I'm unable to confirm, but I believe
> in the latest patch below this is fixed. I removed the em based widths and
> set clearer boundaries on the repeating-linear-gradient. Is there an
> standard way to test this across Firefox versions and platforms?
I can't repro with the new patch either, all good.

Comment 34

[Patrick Brosset <:pbro>]

Comment on attachment 8576369 [details]
Canvas refresh problem

Clearing the feedback flag, please see my earlier comment. Your fix is fine, just modify the comment.

Comment 35

[Patrick Brosset <:pbro>]

Comment on attachment 8576358 [details] [diff] [review]
add_presets_to_cubic_bezier.patch

Review of attachment 8576358 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me.
Only tests are missing now. If you manage to make the current tests pass again, that's a good start. Then, please create a few new ones. Better create many small tests than 1 big that does everything. So you should start with one that just opens the tooltip and checks that the categories and presets are actually just displayed. Then one that switches categories and checks that presets are displayed. Etc... until you write more complex ones that actually click on presets and check that the preview refreshes and that the property value in the rule-view changes too.
And you were right, the browser_ruleview_cubicbezier-* tests are what you should be looking at.
Some information about tests can be found here: https://wiki.mozilla.org/DevTools/Hacking#Running_the_Developer_Tools_Tests

::: browser/devtools/shared/widgets/CubicBezierWidget.js
@@ +151,5 @@
> +    // Fix canvas refresh problem.
> +    // Without the following 4 lines, dragging of handles
> +    // will leave artifacts on the canvas.
> +    // More info: http://simonsarris.com/blog/346-how-
> +    // you-clear-your-canvas-matters

As said earlier, please modify this comment.

@@ +622,5 @@
> +
> +    for (let preset of this.presets) {
> +      preset.addEventListener("click", this._onPresetClick);
> +    }
> +

nit: empty line

@@ +636,5 @@
> +    }
> +  },
> +
> +  /*
> +   * When a preset is selected, event delegation requires searching

As commented earlier, you don't need to worry about going back up to the parent, use currentTarget instead.

Comment 36

[John Giannakos [:jtgi]]

Attachment: add_presets_to_cubic_bezier.patch

Apologies for the slow turn around; school has been relentless.

Updated patch contains:
- Changes as per previous review feedback.
- Updated tests.
- New integration tests as per feedback.

Comment 37

[John Giannakos [:jtgi]]

Attachment: add_presets_to_cubic_bezier.patch

Removed some leftover whitespace.

Comment 38

[John Giannakos [:jtgi]]

Comment on attachment 8581464 [details] [diff] [review]
add_presets_to_cubic_bezier.patch

Review of attachment 8581464 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/devtools/shared/test/browser_cubic-bezier-02.js
@@ +14,5 @@
>  
>  add_task(function*() {
>    yield promiseTab("about:blank");
>    let [host, win, doc] = yield createHost("bottom", TEST_URI);
> +  doc.body.setAttribute("style", "position: fixed");

Without this, boundingClientRect.top has negative top (-14) and visually, the cubic bezier widget is clipped under the bottom frame.
This only occurs in the test env, debugged it for some time but not sure why it happens. 
I need it to be zero'd to calculate expected offsets correctly.

@@ +42,5 @@
>    let onUpdated = widget.once("updated");
>  
>    info("Generating a mousedown/move/up on P1");
>    widget._onPointMouseDown({target: widget.p1});
> +  doc.onmousemove({pageX: offsets.left, pageY: offsets.graphTop});

The hardcoded values don't work so well anymore for a couple reasons:
1. The canvas no longer begins at pageX 0. Because of the presets, it now begins at the horizontal halfway point in the tooltip.
2. The canvas no longer uses padding of 25%. This produced some interesting floats that complicated the nice simple expected values you had.

@@ +107,4 @@
>    info("Moving P1 to the left");
> +  let newOffset = parseInt(widget.p1.style.left) - singleStep;
> +  let x = widget.bezierCanvas.
> +          offsetsToCoordinates({style: {left: newOffset}})[0];

This was also due to the fact that the bezier canvas uses a padding of 30% as oppose to the previous 25%. It made these floats far less clean and tougher to work with. That's why I resorted to calculating the values with offsetsToCoords

Comment 39

[Patrick Brosset <:pbro>]

Comment on attachment 8581464 [details] [diff] [review]
add_presets_to_cubic_bezier.patch

Review of attachment 8581464 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks. Those new tests look good.
Can I suggest that in future bugs you try and split your work in several patches? For instance here, it would help review if the test changes were in a different patch.
I just a few nit comments (whitespaces, comments, ...) but the patch looks fine for me otherwise, R+
I pushed it to try to verify that tests still pass fine https://treeherder.mozilla.org/#/jobs?repo=try&revision=af2e6bbb7c2d 
(they all passed fine for me locally, so not expecting anything too bad).

Can you update the patch one last time as per my comments and then feel free to mark it R+ yourself when you upload the new version.
Once try is finished and is green, we'll land the patch. Thanks John!

::: browser/devtools/shared/test/browser_cubic-bezier-01.js
@@ +7,5 @@
>  // Tests that the CubicBezierWidget generates content in a given parent node
>  
>  const TEST_URI = "chrome://browser/content/devtools/cubic-bezier-frame.xhtml";
>  const {CubicBezierWidget} = devtools.require("devtools/shared/widgets/CubicBezierWidget");
> +const {PREDEFINED, PRESETS, DEFAULT_PRESET_CATEGORY} = require("devtools/shared/widgets/CubicBezierPresets");

It doesn't look like you actually need to require this module, as none of the exported variables are being used here.

::: browser/devtools/shared/test/browser_cubic-bezier-02.js
@@ +6,5 @@
>  
>  // Tests the CubicBezierWidget events
>  
>  const TEST_URI = "chrome://browser/content/devtools/cubic-bezier-frame.xhtml";
> +const {CubicBezierWidget} = 

nit: trailing whitespace.

@@ +11,2 @@
>    devtools.require("devtools/shared/widgets/CubicBezierWidget");
> +const {PREDEFINED, PRESETS, DEFAULT_PRESET_CATEGORY} = 

nit: trailing whitespace.

Also, PRESETS and DEFAULT_PRESET_CATEGORY aren't being used in this test, no need to import these variables here.

@@ +14,5 @@
>  
>  add_task(function*() {
>    yield promiseTab("about:blank");
>    let [host, win, doc] = yield createHost("bottom", TEST_URI);
> +  doc.body.setAttribute("style", "position: fixed");

It's odd, but I'm ok with fixing the position in the test. Can you just copy your explanation as a comment above this line?

@@ +20,5 @@
>    let container = doc.querySelector("#container");
>    let w = new CubicBezierWidget(container, PREDEFINED.linear);
>  
> +  let rect = w.curve.getBoundingClientRect();
> +  rect.padding = w.bezierCanvas.padding[0];

Why store padding as a property of rect, it doesn't seem to be used anywhere else than in the current scope. You could just make it a local variable `let padding`

@@ +107,4 @@
>    info("Moving P1 to the left");
> +  let newOffset = parseInt(widget.p1.style.left) - singleStep;
> +  let x = widget.bezierCanvas.
> +          offsetsToCoordinates({style: {left: newOffset}})[0];

Sounds good to me.

::: browser/devtools/shared/test/browser_cubic-bezier-03.js
@@ +6,5 @@
>  
>  // Tests that coordinates can be changed programatically in the CubicBezierWidget
>  
>  const TEST_URI = "chrome://browser/content/devtools/cubic-bezier-frame.xhtml";
> +const {CubicBezierWidget} = 

nit: trailing whitespace.

@@ +11,2 @@
>    devtools.require("devtools/shared/widgets/CubicBezierWidget");
> +const {PREDEFINED, PRESETS, DEFAULT_PRESET_CATEGORY} = 

nit: trailing whitespace.
Also, if this test only uses PREDEFINED, no need to import all 3 symbols then.
const {PREDEFINED} = require...

::: browser/devtools/shared/test/browser_cubic-bezier-04.js
@@ +6,5 @@
> +
> +// Tests that the CubicBezierPresetWidget generates markup.
> +
> +const TEST_URI = "chrome://browser/content/devtools/cubic-bezier-frame.xhtml";
> +const {CubicBezierPresetWidget} = 

nit trailing whitespace.

@@ +8,5 @@
> +
> +const TEST_URI = "chrome://browser/content/devtools/cubic-bezier-frame.xhtml";
> +const {CubicBezierPresetWidget} = 
> +  devtools.require("devtools/shared/widgets/CubicBezierWidget");
> +const {PREDEFINED, PRESETS, DEFAULT_PRESET_CATEGORY} = 

nit trailing whitespace.

@@ +25,5 @@
> +
> +  ok(container.querySelector("#preset-categories"),
> +     "The preset categories have been added");
> +  let categories = container.querySelectorAll(".category");
> +  is(categories.length, Object.keys(PRESETS).length, 

nit trailing whitespace.

@@ +38,5 @@
> +  Object.keys(PRESETS).forEach(category => {
> +    Object.keys(PRESETS[category]).forEach(presetLabel => {
> +      let preset = container.querySelector("#" + presetLabel);
> +      ok(preset, `${presetLabel} has been added`);
> +      ok(preset.querySelector("canvas"), 

nit trailing whitespace.

::: browser/devtools/shared/test/browser_cubic-bezier-05.js
@@ +6,5 @@
> +
> +// Tests that the CubicBezierPresetWidget cycles menus
> +
> +const TEST_URI = "chrome://browser/content/devtools/cubic-bezier-frame.xhtml";
> +const {CubicBezierPresetWidget} = 

nit trailing whitespace.

@@ +8,5 @@
> +
> +const TEST_URI = "chrome://browser/content/devtools/cubic-bezier-frame.xhtml";
> +const {CubicBezierPresetWidget} = 
> +  devtools.require("devtools/shared/widgets/CubicBezierWidget");
> +const {PREDEFINED, PRESETS, DEFAULT_PRESET_CATEGORY} = 

nit trailing whitespace.

@@ +23,5 @@
> +
> +  w.refreshMenu([0, 0, 0, 0]);
> +  is(w.activeCategory, container.querySelector(`#${DEFAULT_PRESET_CATEGORY}`),
> +    "Check the default category is selected if none chosen yet");
> +  is(w._activePreset, null, "Check no preset is active");

Comments in assertions shouldn't really say what you're doing, but rather what you're expecting:
is(w._activePreset, null, "There is no selected category");
Same comment for other assertions in this file and other test files.

::: browser/devtools/shared/test/browser_cubic-bezier-06.js
@@ +7,5 @@
> +
> +// Tests the integration between CubicBezierWidget and CubicBezierPresets
> +
> +const TEST_URI = "chrome://browser/content/devtools/cubic-bezier-frame.xhtml";
> +const {CubicBezierWidget} = 

nit trailing whitespace.

@@ +9,5 @@
> +
> +const TEST_URI = "chrome://browser/content/devtools/cubic-bezier-frame.xhtml";
> +const {CubicBezierWidget} = 
> +  devtools.require("devtools/shared/widgets/CubicBezierWidget");
> +const {PREDEFINED, PRESETS, DEFAULT_PRESET_CATEGORY} = 

nit trailing whitespace.

@@ +17,5 @@
> +  yield promiseTab("about:blank");
> +  let [host, win, doc] = yield createHost("bottom", TEST_URI);
> +
> +  let container = doc.querySelector("#container");
> +  let w = new CubicBezierWidget(container, 

nit trailing whitespace.

@@ +38,5 @@
> +
> +function* adjustingBezierUpdatesPreset(widget, win, doc, rect) {
> +  info("Checking that changing the bezier refreshes the preset menu");
> +
> +  is(widget.presets.activeCategory, 

nit trailing whitespace.

@@ +39,5 @@
> +function* adjustingBezierUpdatesPreset(widget, win, doc, rect) {
> +  info("Checking that changing the bezier refreshes the preset menu");
> +
> +  is(widget.presets.activeCategory, 
> +     doc.querySelector("#ease-in"), 

nit trailing whitespace.

@@ +42,5 @@
> +  is(widget.presets.activeCategory, 
> +     doc.querySelector("#ease-in"), 
> +     "Check preset category is as initialized");
> +
> +  is(widget.presets._activePreset, 

nit trailing whitespace.

@@ +43,5 @@
> +     doc.querySelector("#ease-in"), 
> +     "Check preset category is as initialized");
> +
> +  is(widget.presets._activePreset, 
> +     doc.querySelector("#ease-in-sine"), 

nit trailing whitespace.

@@ +51,5 @@
> +  widget._onPointMouseDown({target: widget.p1});
> +  doc.onmousemove({pageX: rect.left, pageY: rect.graphTop});
> +  doc.onmouseup();
> +
> +  is(widget.presets.activeCategory, 

nit trailing whitespace.

@@ +52,5 @@
> +  doc.onmousemove({pageX: rect.left, pageY: rect.graphTop});
> +  doc.onmouseup();
> +
> +  is(widget.presets.activeCategory, 
> +     doc.querySelector("#ease-in"), 

nit trailing whitespace.

@@ +55,5 @@
> +  is(widget.presets.activeCategory, 
> +     doc.querySelector("#ease-in"), 
> +     "Check preset category is still last active category");
> +
> +  is(widget.presets._activePreset, null, 

nit trailing whitespace.

Comment 40

[John Giannakos [:jtgi]]

Attachment: add_presets_to_cubic_bezier.patch

Updated in response to code review.

Patrick, thanks for all your help. Super excited to see this land and to take on something new.

Cheers!

Comment 41

[Patrick Brosset <:pbro>]

Try is nice and green: https://treeherder.mozilla.org/#/jobs?repo=try&revision=af2e6bbb7c2d (the rare failures aren't related).
Let's ask for this to be checked-in.

Comment 42

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/1e547a8615b4

Comment 43

[Brian Grinstead [:bgrins]]

Clearing needinfo for UX since this landed

Comment 44

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1e547a8615b4

Comment 45

[Tim Nguyen :ntim]

Release Note Request (optional, but appreciated)
[Why is this notable]: Great new feature
[Suggested wording]: Inspector : Cubic bezier tooltip now shows presets
[Links (documentation, blog post, etc)]:

Comment 46

[John Giannakos [:jtgi]]

(In reply to Tim Nguyen [:ntim] from comment #45)
> Release Note Request (optional, but appreciated)
> [Why is this notable]: 
Inspector : Cubic bezier tooltip now shows a gallery of timing-function presets for use with CSS animations.

> [Links (documentation, blog post, etc)]: 
http://jtgi.me/adding-presets-to-firefox-dev-tools/ 
(not sure what the expectation in terms of content at the link is for this. Please discard if not suitable.)

Comment 47

[John Giannakos [:jtgi]]

^

Comment 48

[Patrick Brosset <:pbro>]

Thanks for the clarifications John, that sounds fine.

Comment 49

[Liz Henry (:lizzard) (relman/hg->git project)]

Adding this to release notes for 39 aurora. Thanks for including the blog post!