Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Description • 13 years ago

Web developer style tools should have a cubic bezier tooltip for css values as appropriate

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 1 • 13 years ago

The tooltip should be a line graph

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 2 • 13 years ago

Please see meta bug 711947 for more info

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 3 • 12 years ago

Filter on PEGASUS.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 4 • 11 years ago

It should look something like this (screenshot courtesy of Lea Verou).

The code she users for her previews is at https://github.com/LeaVerou/dabblet/blob/master/code/previewers.js

She says that if we use her code (with changes) we should credit her in the file header.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 5 • 11 years ago

Cubic bezier matching regex:
/cubic-bezier\(([\d.]+,\s*){3}[\d.]+\)/gi

	Patrick Brosset <:pbro>
	Comment 6 • 11 years ago

Part of the global devtools tooltip plan: https://gist.github.com/captainbrosset/6681978
I propose that we use this bug to do the same as in bug 889638 : a little swatch could precede the actual timing function in the css code and, when the user clicks on it, a cubic bezier editor could appear in a tooltip. It would allow to both visualize what the curve looks like as well as edit it.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 7 • 11 years ago

(In reply to Patrick Brosset from comment #6)
> Part of the global devtools tooltip plan:
> https://gist.github.com/captainbrosset/6681978
> I propose that we use this bug to do the same as in bug 889638 : a little
> swatch could precede the actual timing function in the css code and, when
> the user clicks on it, a cubic bezier editor could appear in a tooltip. It
> would allow to both visualize what the curve looks like as well as edit it.


Yup, I like that idea.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 8 • 11 years ago

Ideally we will allow the curve to be dragged outside of the chart area as this allows bounce-back effects. Not sure how doable this would be though.

e.g. See http://cubic-bezier.com/#.42,-0.98,.95,1.96

	Patrick Brosset <:pbro>
	Comment 9 • 11 years ago

That should be possible, although we probably don't want to take up too much vertical space, so we might limit it a bit more than what cubic-bezier.com does.

	Patrick Brosset <:pbro>
	Comment 10 • 10 years ago

I've just resumed working on this bug. Currently porting Lea Verrou's cubic-bezier.com code to our devtools. 
It's definitely the best cubic-bezier curve editor tool out there.

	Patrick Brosset <:pbro>
	Comment 11 • 10 years ago

part1 - The cubic-bezier widget itself.
This widget works somewhat like the colorpicker widget (spectrum):
- it has an HTML frame to be loaded into
- it takes a set of coordinates (control points) as argument to draw the curve
- control points are draggable, keyboard movable and the curve can be clicked to move the closest point
- comes complete with tests.

Next step is to create a SwatchBased tooltip for it, so that it can be used in the rule-view.

	Patrick Brosset <:pbro>
	Comment 12 • 10 years ago

Working on part2: integrating the widget into an editor tooltip in the rule-view.

	Patrick Brosset <:pbro>
	Comment 13 • 10 years ago

Added a couple of setters to the widget so the curve can be changed programmatically and added tests for them.

	Patrick Brosset <:pbro>
	Comment 14 • 10 years ago

Added a timing-function preview widget so that when you tweak the curve, you can preview the effect.
Idea for this came from : https://twitter.com/rachelnabors/status/481857379303034880

(Useful even if we have live preview of changes in the rule-view because the animation may not be running at this stage).

	Patrick Brosset <:pbro>
	Comment 15 • 10 years ago

part2 - Integrated the widget as a swatch-based tooltip in the rule-view.
Still rough on the edges for now, but should work.

Next step (apart from fixing bugs and adding tests) is to make a non-editable version for the computed-view.

	Patrick Brosset <:pbro>
	Comment 16 • 10 years ago

Bug 977063 will make use of the new inDOMUtils.cssPropertySupportsType helper to fix cases where words part of free text (like web font names) were interpreted as colors.
The cubic-bezier tooltip also suffers from the same bug if words that look like timing-functions end up in non-timing-function properties.
Bug 977063 will most likely make changes to output-parser.js that I need to wait on before fixing this.

	Patrick Brosset <:pbro>
	Comment 17 • 10 years ago

The part1 cubic-bezier widget patch is ready for review.
Pending try build: https://tbpl.mozilla.org/?tree=Try&rev=b4c86e169ca6

Note that this patch only contains the widget, not the clickable swatch in the rule-view.

	Patrick Brosset <:pbro>
	Comment 18 • 10 years ago

New part2: only contains the changes to output-parser.js to parse timing functions out of css properties.

	Patrick Brosset <:pbro>
	Comment 19 • 10 years ago

part3 - rule-view swatch and tooltip to edit timing-functions.

	Heather Arthur [:harth]
	Comment 20 • 10 years ago

Comment on attachment 8447993 [details] [diff] [review]
bug711941-part1-cubic-bezier-widget v4.patch

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

This looks great. Just a couple things.

::: browser/devtools/shared/widgets/CubicBezierWidget.js
@@ +2,5 @@
> + * 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/. */
> +
> +// Based on www.cubic-bezier.com by Lea Verou
> +// See https://github.com/LeaVerou/cubic-bezier

I'm not sure we can just say this. Did we use significant chunks of code from it? If so it seems like we might need to add the MIT license.

@@ +130,5 @@
> +
> +    settings || (settings = {});
> +
> +    for (let setting in defaultSettings) {
> +      (setting in settings) || (settings[setting] = defaultSettings[setting]);

This modifies the settings object you're passing in. Probably fine since you'll probably be the only one using this API, but I wouldn't expect a function to modify the argument I pass in unless it says it does.

@@ +307,5 @@
> +  _onPointKeyDown: function(event) {
> +    let point = event.target;
> +    let code = event.keyCode;
> +
> +    if(code >= 37 && code <= 40) {

nit, space after if

@@ +345,5 @@
> +
> +    this._updateFromPoints();
> +  },
> +
> +  _updateFromPoints: function() {

Function comment headers would help. Even if it's not full boilerplate, just a brief mention of what the function does.

	Patrick Brosset <:pbro>
	Comment 21 • 10 years ago

Thanks Heather for the review.
I addressed all the points in this new patch.
New try build for good measure: https://tbpl.mozilla.org/?tree=Try&rev=e78b57bde7f6

	Patrick Brosset <:pbro>
	Comment 22 • 10 years ago

CCing :gerv about the MIT license. 

gerv, as you can see in the CubicBezierWidget.js file [1] I've added the MIT license as the code mostly comes from Lea Verou's cubic-bezier's library, it's only been adapted to fit into our codebase better.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8449360&action=diff#a/browser/devtools/shared/widgets/CubicBezierWidget.js_sec2

	Gervase Markham [:gerv]
	Comment 23 • 10 years ago

That sounds fine. If this is shipping in Firefox, you need to update about:license to add a copy of this license at the appropriate alphabetical point.

Gerv

	Patrick Brosset <:pbro>
	Comment 24 • 10 years ago

Added an entry in about:license

	Patrick Brosset <:pbro>
	Comment 25 • 10 years ago

Comment on attachment 8449377 [details] [diff] [review]
bug711941-part1-cubic-bezier-widget v6.patch

Gerv, can you review the changes made to about:license? Or point me to someone who should?

	Gervase Markham [:gerv]
	Comment 26 • 10 years ago

Comment on attachment 8449377 [details] [diff] [review]
bug711941-part1-cubic-bezier-widget v6.patch

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

r=gerv, with the license name changed to "cubic-bezier" (lower-case C) to match the upstream project.

Gerv

	Patrick Brosset <:pbro>
	Comment 27 • 10 years ago

Thanks Gerv.
cubic-bezier changed to lower-case.

Green try: https://tbpl.mozilla.org/?tree=Try&rev=e78b57bde7f6
Pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/fe19e5d96d3d

	Ryan VanderMeulen [:RyanVM]
	Comment 28 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/fe19e5d96d3d

	Patrick Brosset <:pbro>
	Comment 29 • 10 years ago

Mike, this patch only adds cubic-bezier parsing abilities to output-parser.js.
To avoid bugs whereby "linear" in "linear-gradient()" would be parsed as a cubic-bezier timing function we need bug 977063 to be resolved, but in the meantime, just in case this lands before bug 977063, I've added a simple fix with a comment saying the fix should be removed.
Let me know if that's fine?

Pending try build: https://tbpl.mozilla.org/?tree=Try&rev=e0c346dade77

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 30 • 10 years ago

Comment on attachment 8452979 [details] [diff] [review]
bug711941-part2-output-parser-parses-cubic-bezier v2.patch

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

Nice and simple.

r+ assuming a green try.

	Patrick Brosset <:pbro>
	Comment 31 • 10 years ago

part 2 now pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/91ce45738633

	Carsten Book [:Tomcat]
	Comment 32 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/91ce45738633

	Patrick Brosset <:pbro>
	Comment 33 • 10 years ago

part3 - v3

This part styles the bezier-swatch added in the rule-view (thanks to the output-parser changes made in part 2) and makes it clickable.
On click, a new tooltip appears (similar to the color-picker tooltip) and shows the cubic-bezier widget (done in part 1).
It's then possible to modify the timing-function curve in the tooltip to change the value in the rule-view.
ESC and ENTER also work like with the color-picker.

Pending try build: https://tbpl.mozilla.org/?tree=Try&rev=fc5be4d5e3fd

	Patrick Brosset <:pbro>
	Comment 34 • 10 years ago

Note that part 3 doesn't change anything for the computed-view. I intend to make changes to the computed-view (read-only cubic-bezier timing-function tooltip) in a separate patch.

	Mike Ratcliffe [:miker] [:mratcliffe] [:mikeratcliffe] Reporter
	Comment 35 • 10 years ago

Comment on attachment 8456773 [details] [diff] [review]
bug711941-part3-swatch-based-tooltip v3.patch

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

r+ assuming a green try.

::: browser/devtools/styleinspector/test/browser_ruleview_cubicbezier-appears-on-swatch-click.js
@@ +20,5 @@
> +  '<div class="test">Testing the cubic-bezier tooltip!</div>'
> +].join("\n");
> +
> +let test = asyncTest(function*() {
> +  yield addTab("data:text/html,rule view cubic-bezier tooltip test");

Nit: yield addTab("data:text/html;charset=utf-8,rule view cubic-bezier tooltip test");

We have enough false errors in our logs... same for the other tests.

	Patrick Brosset <:pbro>
	Comment 36 • 10 years ago

Thanks Mike for the review. Waiting for a green try build and then landing this part 3.

About utf-8, I decided to do this as a separate patch since I took the opportunity to fix *all* occurrences of data:text/html in devtools/styleinspector tests.
I've R+'d this patch myself since this is only minor test changes.

	Patrick Brosset <:pbro>
	Comment 37 • 10 years ago

Try is green, landing both part3 and part3.1 patches to fx-team:

remote:   https://hg.mozilla.org/integration/fx-team/rev/2a7afe8ef3d7
remote:   https://hg.mozilla.org/integration/fx-team/rev/0b0b703ed428

	Carsten Book [:Tomcat]
	Comment 38 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/2a7afe8ef3d7
https://hg.mozilla.org/mozilla-central/rev/0b0b703ed428

	Sylvestre Ledru [:Sylvestre]
	Comment 39 • 10 years ago

Added to the release note with "Developer tools: Cubic-bezier curves editor" as wording.

	Patrick Brosset <:pbro>
	Comment 40 • 10 years ago

The remaining part still to be done here is a cubic-bezier appear-on-hover tooltip for the computed-view.

The computed-view is read-only, so a tooltip that behaves more like the background image preview tooltip seems like a good fit. This way you'd only see the cubic-bezier curve when you hover over the swatch icon.

Here's what I think should be done:
- Add a isReadOnly option to CubicBezierWidget that, when passed to true, does not initialize any event listeners
- Also add the same option as a param to Tooltip.prototype.setCubicBezierContent so it can be passed to CubicBezierWidget from there
- Change style-inspector-overlays.js so that in _getTooltipType, there's a condition for computed-view and transition-timing-function and animation-timing-function that, when met, will just call setCubicBezierContent with isReadOnly to true.

Something along those lines should work.

	Patrick Brosset <:pbro>
	Comment 41 • 10 years ago

Unassigning as I won't have time to work on this last part right now. Happy to mentor anyone who wants to finish it off. Not a good first bug though, more like a good next bug.

	Patrick Brosset <:pbro>
	Comment 42 • 9 years ago

This has been leave-open for a long while. I think it was marked as such so that we would put the tooltip in the computed-view too.
If we want to do that, let's file a new bug.