If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

Inspector should have a cubic bezier tooltip for css values as appropriate

RESOLVED FIXED in Firefox 33

Status

()

Firefox
Developer Tools: Inspector
RESOLVED FIXED
6 years ago
2 years ago

People

(Reporter: miker, Unassigned, Mentored)

Tracking

(Blocks: 1 bug)

unspecified
Firefox 33
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox33 fixed, firefox34 fixed, relnote-firefox 33+)

Details

Attachments

(6 attachments, 9 obsolete attachments)

118.84 KB, image/png
Details
59.97 KB, image/png
Details
43.03 KB, patch
pbro
: review+
pbro
: checkin+
Details | Diff | Splinter Review
9.62 KB, patch
miker
: review+
pbro
: checkin+
Details | Diff | Splinter Review
41.04 KB, patch
miker
: review+
pbro
: checkin+
Details | Diff | Splinter Review
59.11 KB, patch
pbro
: review+
pbro
: checkin+
Details | Diff | Splinter Review
Web developer style tools should have a cubic bezier tooltip for css values as appropriate
The tooltip should be a line graph
Blocks: 711947
Please see meta bug 711947 for more info
Filter on PEGASUS.
Whiteboard: [computedview][ruleview]
Created attachment 712181 [details]
Cubic Bezier (easing) Tooltip

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.
Cubic bezier matching regex:
/cubic-bezier\(([\d.]+,\s*){3}[\d.]+\)/gi
Component: Developer Tools: Style Editor → Developer Tools: Inspector
Summary: Web developer style tools should have a cubic bezier tooltip for css values as appropriate → Inspector should have a cubic bezier tooltip for css values as appropriate
Assignee: nobody → pbrosset
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.
(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.
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
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.
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.
Created attachment 8445777 [details] [diff] [review]
bug711941-part1-cubic-bezier-widget v1.patch

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.
Created attachment 8445830 [details]
wip screenshot

Working on part2: integrating the widget into an editor tooltip in the rule-view.
Created attachment 8445924 [details] [diff] [review]
bug711941-part1-cubic-bezier-widget v2.patch

Added a couple of setters to the widget so the curve can be changed programmatically and added tests for them.
Attachment #8445777 - Attachment is obsolete: true
Created attachment 8446518 [details] [diff] [review]
bug711941-part1-cubic-bezier-widget v3.patch

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).
Attachment #8445924 - Attachment is obsolete: true
Created attachment 8446521 [details] [diff] [review]
bug711941-part2-swatch-based-tooltip v1.patch

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.
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.
Depends on: 977063
Created attachment 8447993 [details] [diff] [review]
bug711941-part1-cubic-bezier-widget v4.patch

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.
Attachment #8446518 - Attachment is obsolete: true
Attachment #8447993 - Flags: review?(fayearthur)
Created attachment 8447994 [details] [diff] [review]
bug711941-part2-output-parser-parses-cubic-bezier v1.patch

New part2: only contains the changes to output-parser.js to parse timing functions out of css properties.
Attachment #8446521 - Attachment is obsolete: true
Created attachment 8447995 [details] [diff] [review]
bug711941-part3-swatch-based-tooltip v2.patch

part3 - rule-view swatch and tooltip to edit timing-functions.
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.
Attachment #8447993 - Flags: review?(fayearthur) → review+
Created attachment 8449360 [details] [diff] [review]
bug711941-part1-cubic-bezier-widget v5.patch

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
Attachment #8447993 - Attachment is obsolete: true
Attachment #8449360 - Flags: review+
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
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
Created attachment 8449377 [details] [diff] [review]
bug711941-part1-cubic-bezier-widget v6.patch

Added an entry in about:license
Attachment #8449360 - Attachment is obsolete: true
Attachment #8449377 - Flags: review+
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?
Attachment #8449377 - Flags: review?(gerv)
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
Attachment #8449377 - Flags: review?(gerv) → review+
Created attachment 8450093 [details] [diff] [review]
bug711941-part1-cubic-bezier-widget v7.patch

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
Attachment #8449377 - Attachment is obsolete: true
Attachment #8450093 - Flags: review+
Keywords: leave-open
Attachment #8450093 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/fe19e5d96d3d
Flags: in-testsuite+
Created attachment 8452979 [details] [diff] [review]
bug711941-part2-output-parser-parses-cubic-bezier v2.patch

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
Attachment #8447994 - Attachment is obsolete: true
Attachment #8452979 - Flags: review?(mratcliffe)
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.
Attachment #8452979 - Flags: review?(mratcliffe) → review+
part 2 now pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/91ce45738633
Attachment #8452979 - Flags: checkin+
https://hg.mozilla.org/mozilla-central/rev/91ce45738633
Created attachment 8456773 [details] [diff] [review]
bug711941-part3-swatch-based-tooltip v3.patch

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
Attachment #8447995 - Attachment is obsolete: true
Attachment #8456773 - Flags: review?(mratcliffe)
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.
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.
Attachment #8456773 - Flags: review?(mratcliffe) → review+
Created attachment 8457471 [details] [diff] [review]
bug711941-part3.1-test-tabs-utf8-charset v1.patch

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.
Attachment #8457471 - Flags: review+
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
Status: NEW → ASSIGNED
https://hg.mozilla.org/mozilla-central/rev/2a7afe8ef3d7
https://hg.mozilla.org/mozilla-central/rev/0b0b703ed428
status-firefox33: --- → fixed
status-firefox34: --- → fixed
Target Milestone: --- → Firefox 33
Added to the release note with "Developer tools: Cubic-bezier curves editor" as wording.
relnote-firefox: --- → 33+
Attachment #8456773 - Flags: checkin+
Attachment #8457471 - Flags: checkin+
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.
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.
Assignee: pbrosset → nobody
Mentor: pbrosset@mozilla.com
Whiteboard: [computedview][ruleview] → [good next bug][lang=js][computedview][ruleview]

Updated

3 years ago
Status: ASSIGNED → NEW
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.
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Keywords: leave-open
Resolution: --- → FIXED
Whiteboard: [good next bug][lang=js][computedview][ruleview]
Blocks: 1194146
You need to log in before you can comment on or make changes to this bug.