Fix for #6742: Handle invalid timing function params #6757

marcelgerber · 2014-02-04T20:41:00Z

Todo:

Set values for an empty cubic-bezier() (according to @redmunds, we should use ease-in-out here)
Set values for an invalid cubic-bezier(-5, 0.7, 8, "invalid") (only replace the invalid ones with default values)
Set values for a cubic-bezier(0, 0, 0) with an invalid number of params (only replace the missing/invalid ones with default values)
Set values for an empty steps()
Set values for an invalid steps("invalid", 2) (only replace the invalid ones with default values)
Set values for an steps(1, 2, 3) with an invalid number of params (only replace the missing/invalid ones with default values)
Add unit tests
Display hint to inform the user about the invalid syntax in the editor and that it will be changed (see Fix for #6742: Handle invalid timing function params #6757 (comment) and following comments)
Hide the hint when changes are reflected in the document

marcelgerber · 2014-02-04T20:44:08Z

src/extensions/default/InlineTimingFunctionEditor/TimingFunctionUtils.js

+     * @param {Array} def The default params to replace invalid values with
+     * @return { isNumber: boolean, value: number } 
+     */
+    function getValidBezierParams(match, def) {


I think we can do this easier, but how to?
Any ideas are very appreciated.

Instead of having "easy" and "complex" regexes, rename them to be "strict" and "lax". Then follow the pattern of "strict" versus "lax" used by the other regexes. "lax" is used when initially parsing user data in page and "strict" is used by editor reparsing strings it already knows to be valid (to get offsets for replacing in doc).

For the getValidBezierParams(match, def) function, I think match[1] will contain the parameter string, so you'll need to do something like the following to separate them into an array:

var params = match[1].split(",");

I'll rename the regexes, but we need to do a little bit more repositioning to do that.

Even if we do .split(), we still need for loop to loop through the entries, don't we?
Btw, match[0] contains the parameter string, but the cubic-bezier( prefix is included, too.

Yes, you have to loop through the parameters. Use match[1] because you want only parameters.

redmunds · 2014-02-04T22:18:12Z

src/extensions/default/InlineTimingFunctionEditor/BezierCurveEditor.js

+            } else {
+                return TimingFunctionUtils.getValidBezierParams(match, [ ".42", "0", ".58", "1" ]);
+                // take ease-in-out as default value in case there are no params yet (or they are invalid)
+            }
        } else {


You shouldn't need to do any of this here because this only gets called once we're already in editor, so data should already be valid.

The initial validation gets done in main.js in prepareEditorForProvider() in the call to TimingFunctionUtils.timingFunctionMatch(). The conversion to default parameters should get done on TimingFunctionUtils.timingFunctionMatch().

Also, I think values for ease-in-out should only be used when there are no parameters supplied to cubic-bezier(). For values out-of-range, they should be moved to closest value in range. For example, the range of valid values for x values (first and third parameters) is 0-1, so values less than 0 should be set to 0 and values greater than 1 should be set to 1.

The .valid flag is set if _validateCubicBezierParams() passes. Should I change the name of the flag?

That's a good idea, but we still need to insert some values in case there are non-numeric values (e.g. "invalid").

marcelgerber · 2014-02-05T14:30:46Z

@redmunds Just added another commit with the requested changes included, could you please take a look, too?
I had to add match[1], hope this isn't breaking anything.

redmunds · 2014-02-05T16:56:29Z

No, I don't think the match array should change. All of your changes should be in TimingFunctionUtils. I forgot 1 piece to the puzzle. I hope this explains how this should work.

Starting with these regexes:

    var BEZIER_CURVE_STRICT_REGEX       = /cubic-bezier\(\s*(\S+)\s*,\s*(\S+)\s*,\s*(\S+)\s*,\s*(\S+)\s*\)/,
        BEZIER_CURVE_LAX_REGEX          = /cubic-bezier\((.*)\)/;

First make the original call:

    var match = str.match(BEZIER_CURVE_STRICT_REGEX);

If that fails, then try with BEZIER_CURVE_LAX_REGEX. If that matches, then call getValidBezierParams() to create and/or fix params. This function needs to build a new bezier-curve() function call string -- this is the piece I forgot.

Then run the new string that was created against the strict regex pattern:

    var match = newStr.match(BEZIER_CURVE_STRICT_REGEX);

Then getValidBezierParams() returns either this match or null.

This keeps all processing in TimingFunctionUtils and also keeps the same format for match. Does that make sense?

s# Please enter the commit message for your changes. Lines starting

marcelgerber · 2014-02-05T21:28:55Z

@redmunds Just pushed changes again. I hope this was what you meant, BezierCurveEditor.js isn't changed at all.

marcelgerber · 2014-02-05T21:30:05Z

src/extensions/default/InlineTimingFunctionEditor/TimingFunctionUtils.js

+     */
+    function _getValidBezierParams(match, def) {
+        var param,
+            old_index = match.index, // we need to store the old match.index to re-set the index afterwards


Is there a better way to store and re-set this later?

I don't think this is necessary since the string will be re-matched against BEZIER_CURVE_STRICT_REGEX at the end.

But the index is set to another value because there is no more context (first few chars in row), and the index is used to set the cm selection later on.
Without re-setting, the selection will be on the wrong place (this is the reason why this code is there).

Gotcha. If we update the index, then I think the replacement string will be wrong when we make first update to doc after an edit.

Is that correct? If so, then I see 2 options:

Immediately update doc with updated params. I was hoping to avoid this, but I guess it's not so bad since user can undo.

Keep match object that same but add on a new property which is an array of validatedParams.

Other suggestions?

What do you mean? An update caused by the Inline Editor, or an update while the Inline Editor is open?
The first one seemed to work (with the match.index set like this), even if I did not test that much. Haven't tested the latter yet.

My concern is with the first edit after the params have been inserted/corrected. If that works, then I think it's OK.

Works - just tested.

marcelgerber · 2014-02-06T17:19:33Z

@redmunds I hope this is finally the right one.
Will add other functions (from the todo list above) later or tomorrow.

…arams

redmunds · 2014-02-06T23:27:18Z

src/extensions/default/InlineTimingFunctionEditor/TimingFunctionUtils.js

+        match = str.match(BEZIER_CURVE_VALID_REGEX);
+        if (match && _validateCubicBezierParams(match)) { // cubic-bezier() with valid params
+            return _tagMatch(match, BEZIER);
+        } else {


You don't need this else statement since the if clause returns.

redmunds · 2014-02-06T23:41:26Z

@SAplayer This works pretty great! I made a couple minor comments about the code.

I am able to see the "length" error I was concerned about. Start with cubic-bezier(-0.001, Infinity, 1.001, NaN). The parameters are converted to a valid values. But, only part of the function call is selected in the doc, so when you make an edit, it's not updated correctly and css becomes corrupted.

Works great for initial value of cubic-bezier()!

marcelgerber · 2014-02-07T05:31:47Z

Thanks for the repro steps. I may know where this comes from.
Can we also set a .realLength = oldLength; to use in main.js#L109?

Btw, this may also happen with multiple css values in a line:
transition: cubic-bezier(); margin: 0 auto;
Haven't tried yet.

redmunds · 2014-02-07T16:16:37Z

Can we also set a .realLength = oldLength; to use in main.js#L109?

Adding a new property is good, but the term "real" can be confusing. Maybe .originalLength ?

marcelgerber · 2014-02-07T19:25:27Z

Hey @redmunds, just pushed a few commits to finally fix the nasty selection bugs and implement this feature for steps(), too.
Could you do another review (only the unit tests are missing)?

redmunds · 2014-02-15T18:41:29Z

One way to ~~fix~~ work around it would be to put a delay on the start of the transition.

marcelgerber · 2014-02-15T18:53:00Z

Ah thanks, that works (but is a bit laggy when doing further changes).

redmunds · 2014-02-15T19:02:21Z

src/nls/root/strings.js

@@ -491,6 +491,7 @@ define({
    "INLINE_TIMING_EDITOR_PROGRESSION"          : "Progression",
    "BEZIER_EDITOR_INFO"                        : "<kbd>↑</kbd><kbd>↓</kbd><kbd>←</kbd><kbd>→</kbd> Move selected point<br><kbd class='text'>Shift</kbd> Move by ten units<br><kbd class='text'>Tab</kbd> Switch points",
    "STEPS_EDITOR_INFO"                         : "<kbd>↑</kbd><kbd>↓</kbd> Increase or decrease steps<br><kbd>←</kbd><kbd>→</kbd> 'Start' or 'End'",
+    "INLINE_TIMING_EDITOR_INVALID"              : "The old value <code>{0}</code> is not valid, so the displayed one was changed to <code>{1}</code>. The document will be updated with the first edit.",


One more wording tweak: change "the displayed one" to be a little more descriptive such as "the displayed function".

redmunds · 2014-02-15T19:35:32Z

src/extensions/default/InlineTimingFunctionEditor/BezierCurveEditor.js

@@ -530,6 +530,9 @@ define(function (require, exports, module) {
        // current cubic-bezier() function params
        this._cubicBezierCoords = this._getCubicBezierCoords(bezierCurve);

+        this.hint = $(".hint", this.$element);
+        TimingFunctionUtils.showHideHint(this, bezierCurve.originalLength, bezierCurve.originalString, "cubic-bezier(" + this._cubicBezierCoords.join(", ") + ")");


The usage of originalLength is a bit too magical here. It needs to be easier for the next dev to figure out this code. I'm thinking of something like:

// If function was auto-corrected, then originalString and originalLength hold the original function, // and an informational message need to be shown var show = (bezierCurve.originalLength ? true : false); if (show) { TimingFunctionUtils.showHideHint(this, true, bezierCurve.originalString, "cubic-bezier(" + this._cubicBezierCoords.join(", ") + ")"); } else { TimingFunctionUtils.showHideHint(this, false); }

This also has the benefit of only building and passing the display strings in the correction case (which is not the usual case).

Actually, if you are storing originalString then there's no need to store originalLength, is there? Can't you just use originalString.length?

Will do. Can I do show = !!bezierCurve.originalString.length as well (is this accepted within the coding standard?)
I thought about removing originalLength as well.

It's not necessary to create the show var, and originalString may not exist so maybe:

// If function was auto-corrected, then originalString holds the original function, // and an informational message need to be shown if (bezierCurve.originalString) { TimingFunctionUtils.showHideHint(this, true, bezierCurve.originalString, "cubic-bezier(" + this._cubicBezierCoords.join(", ") + ")"); } else { TimingFunctionUtils.showHideHint(this, false); }

redmunds · 2014-02-15T20:09:19Z

Done with first final review :)

marcelgerber · 2014-02-16T00:23:27Z

Pushed changes, tests still passing.

marcelgerber · 2014-02-16T01:21:32Z

Fixed the animation to not use setTimeout and last 0.6s (plus 0.1s delay, so 0.7s overall) instead of 1.2s.

Maybe @couzteau for the german translation?

marcelgerber · 2014-02-16T17:36:12Z

src/extensions/default/InlineTimingFunctionEditor/TimingFunctionUtils.js

@@ -107,12 +234,44 @@ define(function (require, exports, module) {
    function _validateStepsParams(match) {
        var count = _convertToNumber(match[1]);

-        if (!count.isNumber || count.value <= 0) {
+        if (!count.isNumber || count.value < 1 || Math.floor(count.value) !== count.value) {


Is there a better method to check for an integer?

You could probably use Lo-Dash http://lodash.com/docs#isFinite?

Have you tried this? I think it will accept values like 2.0 as they are still finite...

Just tested it - it's not testing for an integer, but for a (finite) number. So 2.0 or even 2.1 are still valid.

No, I haven't tested it... I just read the documentation I thought it might work. I am not sure why it does that, doesn't seem that useful, but there isn't an isInteger function.

So I guess your method is fine. Here are other methods if you want to check http://stackoverflow.com/questions/3885817/how-to-check-if-a-number-is-float-or-integer

No problem.
It's just checking that the input is not ±Infinity, "", true, false, null or undefined.
But I can't believe it's that difficult in JavaScript to check for an integer - it's a built-in function in most other languages.
I will still use this solution as it should still work for things like 1.0 (some on StackOverflow don't).

I know... But is comming: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Number/isInteger

Too bad is not supported in Brackets yet :(

Ah yes, they are using the same check with floor as I do ;) (we don't need the other checks).
It would be cool if we had NumberUtils in Brackets to do such checks.

redmunds · 2014-02-18T21:22:27Z

@ingorichter or @pthiess - could review the German strings?

redmunds · 2014-02-18T21:36:14Z

This looks good. After the German strings are reviewed, then just need to rebase.

marcelgerber · 2014-02-19T16:03:58Z

@redmunds Here's a rebased version: #6922

Set initial value for cubic-bezier

4ea73b9

marcelgerber mentioned this pull request Feb 4, 2014

[Bezier Curve] Creating a new cubic-bezier with no need for params #6742

Closed

marcelgerber reviewed Feb 4, 2014
View reviewed changes

redmunds self-assigned this Feb 4, 2014

redmunds reviewed Feb 4, 2014
View reviewed changes

Changes requested by redmunds

4cccd3b

Marcel Gerber added 2 commits February 5, 2014 16:18

Allow cubic-beziers with an invalid number of params

5ebd579

Remove debug console.log

c0bc92b

Marcel Gerber added 2 commits February 5, 2014 22:14

Changes requested by redmunds

fe54397

Update to better fit redmunds' expectation

16aadd4

s# Please enter the commit message for your changes. Lines starting

marcelgerber reviewed Feb 5, 2014
View reviewed changes

Minor changes requested by redmunds

4f6c6ec

Marcel Gerber added 2 commits February 6, 2014 18:24

Revert unnecessary indent

355c2c1

Merge remote-tracking branch 'upstream/master' into cubic-bezier-no-p…

8c25aaf

…arams

redmunds reviewed Feb 6, 2014
View reviewed changes

Marcel Gerber added 4 commits February 7, 2014 18:15

Requested changes, fix selection problem

56cc320

Support for steps() with invalid params

8e30cc6

Set second default param for steps()

b3ffb9a

fix a few bugs

814196e

Start animation after a delay

192c1b5

redmunds reviewed Feb 15, 2014
View reviewed changes

Wording tweak

3ca6e8b

redmunds reviewed Feb 15, 2014
View reviewed changes

Review changes

f9926ad

Animation fixes

c314dcc

Review on my own

cd84c85

marcelgerber reviewed Feb 16, 2014
View reviewed changes

Test for an empty originalString in unit tests

45f366d

marcelgerber mentioned this pull request Feb 19, 2014

Rebased: Handle invalid timing function params #6922

Merged

marcelgerber closed this Feb 19, 2014

marcelgerber deleted the cubic-bezier-no-params branch March 1, 2014 17:19

core-ai-bot mentioned this pull request Aug 30, 2021

[CLOSED] Fix for #6742: Handle invalid timing function params brackets-archive/bracketsIssues#6132

Open

9 tasks

Fix for #6742: Handle invalid timing function params #6757

Fix for #6742: Handle invalid timing function params #6757

Conversation

marcelgerber commented Feb 4, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelgerber commented Feb 5, 2014

redmunds commented Feb 5, 2014

marcelgerber commented Feb 5, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcelgerber commented Feb 6, 2014

Choose a reason for hiding this comment

redmunds commented Feb 6, 2014

marcelgerber commented Feb 7, 2014

redmunds commented Feb 7, 2014

marcelgerber commented Feb 7, 2014

redmunds commented Feb 15, 2014

marcelgerber commented Feb 15, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redmunds commented Feb 15, 2014

marcelgerber commented Feb 16, 2014

marcelgerber commented Feb 16, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

redmunds commented Feb 18, 2014

redmunds commented Feb 18, 2014

marcelgerber commented Feb 19, 2014