Closed
Bug 711941
Opened 13 years ago
Closed 9 years ago
Inspector should have a cubic bezier tooltip for css values as appropriate
Categories
(DevTools :: Inspector, defect)
DevTools
Inspector
Tracking
(firefox33 fixed, firefox34 fixed, relnote-firefox 33+)
RESOLVED
FIXED
Firefox 33
People
(Reporter: miker, Unassigned, Mentored)
References
(Blocks 1 open bug)
Details
Attachments
(6 files, 9 obsolete files)
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
Reporter | ||
Comment 1•13 years ago
|
||
The tooltip should be a line graph
Reporter | ||
Comment 2•13 years ago
|
||
Please see meta bug 711947 for more info
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.
Reporter | ||
Comment 5•11 years ago
|
||
Cubic bezier matching regex: /cubic-bezier\(([\d.]+,\s*){3}[\d.]+\)/gi
Reporter | ||
Updated•11 years ago
|
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
Updated•11 years ago
|
Assignee: nobody → pbrosset
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.
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.
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
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.
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.
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.
Comment 12•10 years ago
|
||
Working on part2: integrating the widget into an editor tooltip in the rule-view.
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.
Attachment #8445777 -
Attachment is obsolete: true
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).
Attachment #8445924 -
Attachment is obsolete: true
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.
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.
Depends on: 977063
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.
Attachment #8446518 -
Attachment is obsolete: true
Attachment #8447993 -
Flags: review?(fayearthur)
Comment 18•10 years ago
|
||
New part2: only contains the changes to output-parser.js to parse timing functions out of css properties.
Attachment #8446521 -
Attachment is obsolete: true
Comment 19•10 years ago
|
||
part3 - rule-view swatch and tooltip to edit timing-functions.
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.
Attachment #8447993 -
Flags: review?(fayearthur) → review+
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
Attachment #8447993 -
Attachment is obsolete: true
Attachment #8449360 -
Flags: review+
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
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
Comment 24•10 years ago
|
||
Added an entry in about:license
Attachment #8449360 -
Attachment is obsolete: true
Attachment #8449377 -
Flags: review+
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?
Attachment #8449377 -
Flags: review?(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
Attachment #8449377 -
Flags: review?(gerv) → review+
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
Attachment #8449377 -
Attachment is obsolete: true
Attachment #8450093 -
Flags: review+
Updated•10 years ago
|
Keywords: leave-open
Updated•10 years ago
|
Attachment #8450093 -
Flags: checkin+
Comment 28•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/fe19e5d96d3d
Flags: in-testsuite+
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
Attachment #8447994 -
Attachment is obsolete: true
Attachment #8452979 -
Flags: review?(mratcliffe)
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.
Attachment #8452979 -
Flags: review?(mratcliffe) → review+
Comment 31•10 years ago
|
||
part 2 now pushed to fx-team: https://hg.mozilla.org/integration/fx-team/rev/91ce45738633
Updated•10 years ago
|
Attachment #8452979 -
Flags: checkin+
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
Attachment #8447995 -
Attachment is obsolete: true
Attachment #8456773 -
Flags: review?(mratcliffe)
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.
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.
Attachment #8456773 -
Flags: review?(mratcliffe) → review+
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.
Attachment #8457471 -
Flags: review+
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
Updated•10 years ago
|
Status: NEW → ASSIGNED
Comment 38•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/2a7afe8ef3d7 https://hg.mozilla.org/mozilla-central/rev/0b0b703ed428
Updated•10 years ago
|
Comment 39•10 years ago
|
||
Added to the release note with "Developer tools: Cubic-bezier curves editor" as wording.
relnote-firefox:
--- → 33+
Updated•10 years ago
|
Attachment #8456773 -
Flags: checkin+
Updated•10 years ago
|
Attachment #8457471 -
Flags: checkin+
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.
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.
Assignee: pbrosset → nobody
Mentor: pbrosset
Whiteboard: [computedview][ruleview] → [good next bug][lang=js][computedview][ruleview]
Updated•9 years ago
|
Status: ASSIGNED → NEW
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.
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: leave-open
Resolution: --- → FIXED
Whiteboard: [good next bug][lang=js][computedview][ruleview]
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•