Last Comment Bug 722691 - Ability to increase/decrease values in rule view using arrow keys
: Ability to increase/decrease values in rule view using arrow keys
Status: RESOLVED FIXED
[fixed-in-fx-team]
:
Product: Firefox
Classification: Client Software
Component: Developer Tools: Inspector (show other bugs)
: unspecified
: x86 Mac OS X
: P2 normal with 2 votes (vote)
: Firefox 19
Assigned To: Matthew Wein [:K-9]
:
Mentors:
: 799703 (view as bug list)
Depends on:
Blocks: 742504
  Show dependency treegraph
 
Reported: 2012-01-31 06:57 PST by Kevin Dangoor
Modified: 2014-02-07 14:05 PST (History)
17 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
partial patch for bug 722691 (2.39 KB, patch)
2012-07-14 11:32 PDT, Matthew Wein [:K-9]
no flags Details | Diff | Splinter Review
patch for bug 722691 (5.41 KB, patch)
2012-07-14 21:53 PDT, Matthew Wein [:K-9]
no flags Details | Diff | Splinter Review
patch v2 (5.12 KB, patch)
2012-07-16 17:20 PDT, Matthew Wein [:K-9]
no flags Details | Diff | Splinter Review
patch v3 (21.77 KB, patch)
2012-07-22 22:43 PDT, Matthew Wein [:K-9]
no flags Details | Diff | Splinter Review
patch v4 (21.73 KB, patch)
2012-11-03 14:49 PDT, Matthew Wein [:K-9]
no flags Details | Diff | Splinter Review
patch v5 (22.41 KB, patch)
2012-11-04 12:06 PST, Matthew Wein [:K-9]
no flags Details | Diff | Splinter Review
patch v5.1 (22.79 KB, patch)
2012-11-04 17:49 PST, Matthew Wein [:K-9]
fayearthur: review+
Details | Diff | Splinter Review
patch v6 (30.88 KB, patch)
2012-11-09 22:34 PST, Matthew Wein [:K-9]
no flags Details | Diff | Splinter Review
patch v6.1 (39.80 KB, patch)
2012-11-09 23:04 PST, Matthew Wein [:K-9]
fayearthur: feedback+
Details | Diff | Splinter Review
patch v6.2 (39.12 KB, patch)
2012-11-16 14:55 PST, Matthew Wein [:K-9]
no flags Details | Diff | Splinter Review

Description Kevin Dangoor 2012-01-31 06:57:47 PST
I've gotten feedback in blog comments and on Twitter from people who would like to be able to increase/decrease values in the rule view by using the arrow keys.
Comment 1 Kevin Dangoor 2012-03-14 12:46:07 PDT
This is both frequently requested *and* frequently seen in user studies.
Comment 2 Thierry Régagnon 2012-05-08 11:49:35 PDT
Can I suggest to copy Chrome Web Inspector behavior ? (which also works in Safari)

Up key : increase by 1
Down key : decrease by 1

SHIFT + Up/Down key : Increase/decrease by 10
ALT + Up/Down key : Increase/decrease by 0.1


In Opera Dragonfly and IE Web Developer Tools. Only Up/Down keys increment/decrease by 1.
Comment 3 Thierry Régagnon 2012-05-08 12:14:22 PDT
Just discovered that Shift-PageUp/PageDown in Chrome Web Inspector, increment/decrease by 100.

You can read what they support here:
https://developers.google.com/chrome-developer-tools/docs/elements-styles#styles_edit
Comment 4 Dave Camp (:dcamp) 2012-05-08 12:41:35 PDT
Cedric, any progress on this bug?
Comment 5 Paul Rouget [:paul] 2012-06-07 08:42:38 PDT
Cedric, are you still working on that?
Comment 6 Kevin Dangoor 2012-06-22 07:54:00 PDT
Ping! Someone just asked about this in a comment on the devtools blog. This is definitely a top request.
Comment 7 Dave Camp (:dcamp) 2012-07-10 12:04:03 PDT
Heather, can you take a look at this?
Comment 8 Matthew Wein [:K-9] 2012-07-12 17:05:52 PDT
Hey Heather, if you don't think you have time to work on this, I would like to give a shot at implementing it this weekend because this is a feature I really want to see in the inspector.  Thanks, Matt
Comment 9 Heather Arthur [:harth] 2012-07-13 15:31:10 PDT
(In reply to Matthew Wein from comment #8)
> Hey Heather, if you don't think you have time to work on this, I would like
> to give a shot at implementing it this weekend because this is a feature I
> really want to see in the inspector.  Thanks, Matt

go for it!
Comment 10 Matthew Wein [:K-9] 2012-07-14 11:32:46 PDT
Created attachment 642255 [details] [diff] [review]
partial patch for bug 722691

So far I have the patch working for px, em, and %, with shortcuts for +/- 0.1 (alt + up/down) and +/- 10 (shift + up/down).

It currently doesn't work yet for shorthand property rules, and I'm also having trouble handling the floating point error when adding +/- 0.1.

Please let me know if you know of a better way to do any of this, or if you need me to explain any of it.  Thanks, Matt
Comment 11 Matthew Wein [:K-9] 2012-07-14 11:39:47 PDT
Comment on attachment 642255 [details] [diff] [review]
partial patch for bug 722691

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

::: browser/devtools/styleinspector/CssRuleView.jsm
@@ +2109,5 @@
> +          units = "em";
> +        } else if (input.value.indexOf("%") != -1) {
> +          units = "%";
> +        } else if (input.value.indexOf("%") != -1) {
> +          units = "%";

Sorry I forgot to remove this duplicate check for the units.
Comment 12 Matthew Wein [:K-9] 2012-07-14 21:53:37 PDT
Created attachment 642336 [details] [diff] [review]
patch for bug 722691

This patch covers the following:

Up key : increase by 1
Down key : decrease by 1
SHIFT + Up/Down key : Increase/decrease by 10
ALT + Up/Down key : Increase/decrease by 0.1
Reapply CSS when value is changed.

I decided to use Firebug's implementation to update the value which adds support for shorthand properties.  The code that I wrote myself is the code added to _onKeyPress and _apply. I added a parameter to _apply() to allow the CSS to force apply when the value is changed.

This patch does not implement increasing/decreasing hex colors.
Comment 13 Paul Rouget [:paul] 2012-07-16 08:29:15 PDT
(In reply to Matthew Wein from comment #12)
> This patch covers the following:
> […]
> Reapply CSS when value is changed.

If you do that, can you make sure this works in a generic way? See bug 755797.
Comment 14 Matthew Wein [:K-9] 2012-07-16 17:20:48 PDT
Created attachment 642801 [details] [diff] [review]
patch v2

(In reply to Paul Rouget [:paul] from comment #13)
> (In reply to Matthew Wein from comment #12)
> > This patch covers the following:
> > […]
> > Reapply CSS when value is changed.
> 
> If you do that, can you make sure this works in a generic way? See bug
> 755797.

Sure no problem.
I Moved the code for forcing the css to apply out of this bug, and I'll submit it as a patch for bug 755797 when it's finished.
Comment 15 Heather Arthur [:harth] 2012-07-18 13:21:59 PDT
Comment on attachment 642801 [details] [diff] [review]
patch v2

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

Thanks for taking this up. This looks good and works great.

A couple issues:
1) After selecting something like "8px 5px", incrementing will only increment the first value, so we should change the selection to select only the first value.
2) If you have your cursor inside a floating point number, after the decimal (like "98.7|%"), only the number after the decimal is incremented, the whole number should be selected and incremented by a whole number.
3) If your cursor is in a decimal number and you small increment: "98.7" turns into "98.7.1".

Style-wise, we'll want to stay consistent with the rest of the file, so 2-space indentation, wrap long lines at 80 chars, braces around one-line if statements,  opening brace on same line as if/else statement, and no spaces around arguments. 

You'll also want to add a test.

Overall fantastic, thanks for the patch!

::: browser/devtools/styleinspector/CssRuleView.jsm
@@ +2047,5 @@
>      this.input.style.width = width + "px";
>    },
>  
>    /**
> +   * Increase/decrease values in rule view

Add a note about the increment parameter, see the "_onMenuUpdate()" comment for an example.

Also note the return value, but if you're not going to be using the return value anywhere, don't return anything.

@@ +2065,5 @@
> +    
> +    return true;
> +  },
> +
> +  _incrementExpr: function InplaceEditor_incrementExpr( expr, increment, info )

Add a boilerplate comment describing the function and params.

@@ +2073,5 @@
> +        return null;
> +
> +    var m = /\d+(\.\d+)?/.exec(expr);
> +    var digitPost = expr.substr(m.index+m[0].length);
> +    var newValue = Math.round((num-increment)*1000)/1000; // avoid rounding errors

I'd put spaces in between the math operators.  I read this as a var called "num-increment".

@@ +2078,5 @@
> +
> +    if (info && "minValue" in info)
> +        newValue = Math.max(newValue, info.minValue);
> +    if (info && "maxValue" in info)
> +        newValue = Math.min(newValue, info.maxValue);

As noted below, doesn't seem like an info object ever actually gets passed in, so take these out.

@@ +2082,5 @@
> +        newValue = Math.min(newValue, info.maxValue);
> +
> +    newValue = newValue.toString();
> +
> +    // Preserve trailing zeroes of small increments.

Hm, I don't think this is necessary. And maybe not even preferable? I know I would prefer to see "44px" instead of "44.0px", might want to see what people prefer here.

@@ +2098,5 @@
> +    }
> +    return newValue + digitPost;
> +  },
> +
> +  _doIncrementValue: function InplaceEditor_doIncrementValue( value, increment, offset, offsetEnd, info)

Add a boilerplate comment for function.

It doesn't appear that this function is ever called with an "info" argument. If it's not used, take it out and uses of it.

@@ +2103,5 @@
> +  {
> +    // Try to find a number around the cursor to increment.
> +    var start, end;
> +    if (/^-?[0-9.]/.test(value.substring(offset, offsetEnd)) &&
> +        !(/\d/.test(value.charAt(offset-1) + value.charAt(offsetEnd))))

I don't think you need to special-case this. The code below (in your else statement) should be general enough to handle this case as well.

@@ +2116,5 @@
> +    {
> +        // Parse periods as belonging to the number only if we are in a known number
> +        // context. (This makes incrementing the 1 in 'image1.gif' work.)
> +        var pattern = "[" + (info ? "0-9." : "0-9") + "]*";
> +

I think this is what's causing the problems with incrementing with a decimal, info is never defined.
Comment 16 Matthew Wein [:K-9] 2012-07-22 22:43:28 PDT
Created attachment 644841 [details] [diff] [review]
patch v3

(In reply to Heather Arthur [:harth] from comment #15)
> A couple issues:
> 1) After selecting something like "8px 5px", incrementing will only
> increment the first value, so we should change the selection to select only
> the first value.
> 2) If you have your cursor inside a floating point number, after the decimal
> (like "98.7|%"), only the number after the decimal is incremented, the whole
> number should be selected and incremented by a whole number.
> 3) If your cursor is in a decimal number and you small increment: "98.7"
> turns into "98.7.1".

Thanks Heather! These bugs should all be fixed now in the new patch.

> Style-wise, we'll want to stay consistent with the rest of the file, so
> 2-space indentation, wrap long lines at 80 chars, braces around one-line if
> statements,  opening brace on same line as if/else statement, and no spaces
> around arguments. 

The code should be formatted now, with the exception of one line that contains RegEx which is longer than 80 characters.  I also added boilerplate comments to describe the functions and their parameters.
 
> You'll also want to add a test.
>

I wrote up a test (this is my first time writing one) which goes through 78 test cases that I could think of.  Let me know if you think I should add any others.

> Overall fantastic, thanks for the patch!

Thanks!!

> Also note the return value, but if you're not going to be using the return
> value anywhere, don't return anything.

I ended up deciding to use the return value from _incrementValue in _onKeyPress like so:
+    if (increment) {
+      if ( this._incrementValue( increment ) ) {
+        this._updateSize();
+        prevent = true;
+      }
+    }

I also added | this._updateSize() | so that the input field will adjust it's size when the value is incremented. 

> I'd put spaces in between the math operators.  I read this as a var called
> "num-increment".

Yeah I definitely agree with this, I updated the new patch to add the spaces around these occurrences.

> > +    // Preserve trailing zeroes of small increments.
> 

I also agree with you on this, too.  I don't really like how "0px" changes to "0.1px", but  "0.1px" changes to "0.0px". It seems inconsistent so removed it from this patch, but it would be nice to know what other people think about this.   

Thanks,
Matt
Comment 17 [:marius] 2012-07-22 22:49:41 PDT
I've been following this bug cause I'm so excited to see this fixed so here's my 2 cents on your last comment: it would be nice if 0.1px changes to 0px instead of 0.0px. Conceptually, if I see 0.0px I might assume the arrows increment by 0.1px instead of 1px.

Thanks for taking this on, Matt!
Comment 18 Heather Arthur [:harth] 2012-07-24 10:34:55 PDT
Comment on attachment 644841 [details] [diff] [review]
patch v3

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

Thanks for the update. The test looks good, but we do want to avoid setTimeout()s so let me know if you need help getting rid of that part. I have some suggestions and questions inlined.

In a couple places, we're switching the behavior of the keys based on the context. For example, when pressing up/down on an alpha value we increment by 0.01 instead of 1. This kind of inconsistency could be a usability issue, let's ask around.

::: browser/devtools/styleinspector/CssRuleView.jsm
@@ +2049,5 @@
>  
>    /**
> +   * Increase/decrease values in rule view
> +   *
> +   * @param increment The amount to increase/decrease the property value

Also add a @return note here describing the return value. See the function "_maybeAddRule()" for an example. Same for the rest of the functions.

@@ +2056,5 @@
> +  {
> +    var value = this.input.value;
> +    var offset = this.input.selectionStart;
> +    var offsetEnd = this.input.selectionEnd;
> +    var newValue = this._doIncrementValue(value,increment,offset,offsetEnd);

My bad! I really wasn't clear about this when I said not to put spacing around args. Still put spaces after the commas, just not around the argument list, so: `incrementValue(value, increment, offset, offsetEnd)`.

@@ +2057,5 @@
> +    var value = this.input.value;
> +    var offset = this.input.selectionStart;
> +    var offsetEnd = this.input.selectionEnd;
> +    var newValue = this._doIncrementValue(value,increment,offset,offsetEnd);
> +    if (!newValue) {

This will also reject '0', which is a valid value.

@@ +2067,5 @@
> +    return true;
> +  },
> +
> +  /**
> +   * Calculate the new property value for type == "int"

"int" implies a whole number, and I think this function allows floating points, so I'd rename the type.

@@ +2073,5 @@
> +   * @param expr The property value
> +   * @param increment The amount to increase/decrease the expr
> +   * @param info Object with info about the property value
> +   */
> +  _incrementExpr: function InplaceEditor_incrementExpr(expr,increment,info)

Sorry again, spaces after the commas, same for other functions/function calls. In general you can take a look at other functions in the file for a guide.

I'm a little unclear about what 'expr' means throughout the patch. Is it the raw number to increment? If it's an abbreviation for "expression", then that means something different for me.

@@ +2080,5 @@
> +    if (isNaN(num)) {
> +      return null;
> +    }
> +    var m = /\d+(\.\d+)?/.exec(expr);
> +    var digitPost = expr.substr(m.index + m[0].length);

Shortcut: `var digitPost = m[0]`.

I'm confused about what digitPost is, maybe add a comment?

@@ +2118,5 @@
> +      if (newValue !== null) {
> +        completion = newValue;
> +        selection = [0, completion.length];
> +      }
> +    } else if (type === "rgb" && expr.charAt(0) === "#") {

Looks like you could use a separate type for "hex".

@@ +2120,5 @@
> +        selection = [0, completion.length];
> +      }
> +    } else if (type === "rgb" && expr.charAt(0) === "#") {
> +      var offsetIntoExpr = offset - range.start;
> +      var offsetEndIntoExpr = offsetEnd - range.start;

Variable names are a bit long here, maybe 'exprOffset'?

@@ +2142,5 @@
> +        else if (type === "rgb") { // rgb color
> +          info.minValue = 0;
> +          info.maxValue = 255;
> +          if (Math.abs(increment) < 1)
> +            increment = (increment < 0 ? -1 : 1);

Braces around one-line ifs.

@@ +2149,5 @@
> +          info.maxValue = 100;
> +
> +          // If the selection is at the end of a percentage sign, select
> +          // the previous number. This would have been less hacky if
> +          // _parseCSSValue parsed functions recursively.

What do you mean by "parsing functions"?

@@ +2160,5 @@
> +    }
> +
> +    if (completion === null) {
> +      return;
> +    }

Hard to tell what completion is, maybe a short comment?

@@ +2183,5 @@
> +  {
> +    const reSplitCSS = /(url\("?[^"\)]+"?\)?)|(rgba?\([^)]*\)?)|(hsla?\([^)]*\?)|(#[\dA-Fa-f]+)|(-?\d+(\.\d+)?(%|[a-z]{1,4})?)|"([^"]*)"?|'([^']*)'?|([^,\s\/!\(\)]+)|(!(.*)?)/;
> +    var start = 0;
> +    var m;
> +    while (true) {

I'd do something like `while((m = reSplitCSS.exec(value)) && ...)` if you can. Makes it easier to see what will break the loop.

Add a comment explaining what the loop is doing, it's hard to tell at first look.

@@ +2185,5 @@
> +    var start = 0;
> +    var m;
> +    while (true) {
> +      m = reSplitCSS.exec(value);
> +      if (m && m.index+m[0].length < offset) {

Spaces between operators like +.

@@ +2210,5 @@
> +    } else if (m[5]) {
> +      type = "int";
> +    }
> +
> +    var cssValue = {value: m[0], start: start+m.index,

Format this object so each property is on a new line, as it is elsewhere in the patch. and spaces around the +.

@@ +2213,5 @@
> +
> +    var cssValue = {value: m[0], start: start+m.index,
> +                    end: start+m.index+m[0].length, type: type};
> +
> +    if (!type && m[8] && m[8].indexOf("gradient") != -1) {

Comment about what case this is catching.

@@ +2215,5 @@
> +                    end: start+m.index+m[0].length, type: type};
> +
> +    if (!type && m[8] && m[8].indexOf("gradient") != -1) {
> +      var arg = value.substr(m[0].length)
> +      arg = arg.match(/\((?:(?:[^\(\)]*)|(?:\(.*?\)))+\)/);

Comment about what this regex is matching because it's quite complicated.

@@ +2235,5 @@
> +   * @param offset Starting index of value
> +   * @param offsetEnd Ending index of value
> +   * @param info Object with details about the value
> +   */
> +  _doIncrementValueInternal: function(value,increment,offset,offsetEnd,info)

Since this function is for colors only, maybe incColor() (to match incHexColor()) is a more descriptive name.

@@ +2244,5 @@
> +      !(/\d/.test(value.charAt(offset-1) + value.charAt(offsetEnd)))) {
> +      // We have a number selected, possibly with a suffix, and we are
> +      // not in the disallowed case of just part of a known number being
> +      // selected. Use that number.
> +

Mentioned this in previous review. It really seems like the case in your else statement is generic enough to handle this case as well and give the correct values for 'start' and 'end'.

@@ +2250,5 @@
> +      end = offsetEnd;
> +    } else {
> +      // Parse periods as belonging to the number only if we are in a
> +      // known number context. (This makes incrementing the 1 in
> +      // 'image1.gif' work.)

According to the function header, this function only gets color values, so why would a url be passed in?

@@ +2284,5 @@
> +        return {
> +          value: first + mid + last,
> +          start: start,
> +          //Added split(' ')[0] to select only the first value when
> +          //multiple are selected

Rather than inline this comment in the middle of the object, maybe put if before the object with a `var end =` statement.

@@ +2298,5 @@
> +   * @param value The property value
> +   * @param increment The amount to increase/decrease the value
> +   * @param offset Starting index of value
> +   * @param offsetEnd Ending index of value
> +   * @param info Object with info about the property value

This function doesn't actually take a fifth argument it looks like.

@@ +2300,5 @@
> +   * @param offset Starting index of value
> +   * @param offsetEnd Ending index of value
> +   * @param info Object with info about the property value
> +   */
> +  _incHexColor: function InplaceEditor_incHexColor(expr, amt, offset, offsetEnd)

Mismatch - comment says second argument is "increment".

@@ +2371,5 @@
> +      }
> +      // Make the incremented part upper-case if the original value can be
> +      // seen as such (this should happen even for, say, #444444, because
> +      // upper-case hex-colors are the default). Otherwise, the lower-case
> +      // result from .toString is used.

Don't think you need this comment, the code looks self-explanatory enough.

@@ +2379,5 @@
> +
> +      expr = expr.substr(0,pos) + mid + expr.substr(pos+2);
> +    }
> +
> +    return {value: "#" + expr, selection: [offset+1, offsetEnd+1]};

Each property on a new line like other objects in the patch.

@@ +2421,5 @@
> +    let increment = 0;
> +
> +    if (aEvent.keyCode === Ci.nsIDOMKeyEvent.DOM_VK_UP
> +       || aEvent.keyCode === Ci.nsIDOMKeyEvent.DOM_VK_PAGE_UP ) {
> +      increment = -1;

Seems like it makes more sense for increment to be positive 1 here, and -1 when decreasing the number.

@@ +2428,5 @@
> +      increment = 1;
> +    }
> +
> +    if (aEvent.shiftKey && !aEvent.altKey) {
> +      if ( aEvent.keyCode === Ci.nsIDOMKeyEvent.DOM_VK_PAGE_UP

No space after "("

@@ +2439,5 @@
> +      increment *= smallIncrement;
> +    }
> +
> +    if (increment) {
> +      if ( this._incrementValue( increment ) ) {

Combine into one statement: `if (increment && this._incrementValue(increment))`.

::: browser/devtools/styleinspector/test/browser_bug722691.js
@@ +28,5 @@
> +  paused = false;
> +  // Remove function from the execution testQueue to be invoked after
> +  // we are finished with the current execution testQueue.
> +  setTimeout(runTest);
> +}

Explain somewhere what this pausing is doing and why we need it. It looks suspect though, try to avoid using setTimeout() as it can intermittently fail on slower platforms. Ping me if you need help getting rid of it.

@@ +94,5 @@
> +      7: { pageUp: true, shift: true, start: "0px", end: "100px", selectAll: true },
> +      8: { pageDown: true, shift: true, start: "0px", end: "-100px", selectAll: true,
> +           resume: true }
> +    });
> +    EventUtils.synthesizeMouse(marginPropEditor.valueSpan, 1, 1, {}, ruleDialog);

Add a comment here to explain what the '1's mean.
Comment 19 Heather Arthur [:harth] 2012-07-24 11:46:50 PDT
Brian, can you give us some advice on a interaction issue?

We're using the arrow keys to increment/decrement values like "5px" in the rule view. Up/down to increment by 1, alt-up/alt-down to increment by 0.1, shift-up/shift-down to increment by 10.

Depending on the context, sometimes you're more likely to increment a value by 1 (like dimensions) and sometimes you're more likely to want to increment it by a different amount (like with an alpha value, it only ranges from 0 to 1, so you usually try out increments of 0.1 or 0.01).

Matthew's patch will change the increment tied to up/down depending on the number you're trying to increment. I'm wondering if we should go this route or fix the increments for each key, so up/down always increments by 1, etc.
Comment 20 Paul Rouget [:paul] 2012-07-24 12:20:08 PDT
Can we have some example?

Let's say I am in these situations (where | is the cursor):

#74A0B1|

#74A0|B1

5|.5px

5.5|px

5.5px|

rgba(200,200,20|0,0.5)

rgba(200,200,200,0|.5)

rgba(200,200,200,0.5|)

What should happen if I press "up" and "shift-up"?
Comment 21 Matthew Wein [:K-9] 2012-07-24 13:17:22 PDT
> What should happen if I press "up" and "shift-up"?

This is what I think we would expect to happen using those shortcuts, let me know what you think:
 
> #74A0B1|
up: #74A0B2 shift-up: #74A0C1

> #74A0|B1
up: #74A1B1  shift-up: #74B0B1

> 5|.5px
up: 6.5px  shift-up: 15.5px

> 5.5|px
up: 6.5px  shift-up: 15.5px

> 5.5px|
up: 6.5px  shift-up: 15.5px

> rgba(200,200,20|0,0.5)
expected: up: rgba(200,200,201,0.5)  shift-up: rgba(200,200,210,0.5)

> rgba(200,200,200,0|.5)
up: rgba(200,200,200,0.6) shift-up: rgba(200,200,200,1)

> rgba(200,200,200,0.5|)
up: rgba(200,200,200,0.6) shift-up: rgba(200,200,200,1)

I think the importance lies in the acceptable range for the given property. For dimensions, which don't particularly have a range, I think we should stick to the default increments.  

For values like alpha or opacity, which range from 0 to 1, I would expect the default increment to be smaller, maybe 0.1.  It would follow then that alt+up increments by 0.01, and shift+up would take you directly to 1.

What are your thoughts on this?
Comment 22 Paul Rouget [:paul] 2012-07-24 13:39:40 PDT
(In reply to Matthew Wein [:mwein] from comment #21)
> […]
> What are your thoughts on this?

I'll let Heather and Brian decide on this.

Just one more question: what do other tools (chrome, opera, firebug, …) do?
Comment 23 Dils 2012-07-31 10:42:45 PDT
Brian, can you give us some advice on a interaction issue?

We're using the arrow keys to increment/decrement values like "5px" in the rule view. Up/down to increment by 1, alt-up/alt-down to increment by 0.1, shift-up/shift-down to increment by 10.

* I think this makes sense. I've seen these keyboard shortcut combinations used in other tools that our demographic of users might also use, such as Chrome dev tools, Photoshop, Dreamweaver, etc

Depending on the context, sometimes you're more likely to increment a value by 1 (like dimensions) and sometimes you're more likely to want to increment it by a different amount (like with an alpha value, it only ranges from 0 to 1, so you usually try out increments of 0.1 or 0.01).

Matthew's patch will change the increment tied to up/down depending on the number you're trying to increment. I'm wondering if we should go this route or fix the increments for each key, so up/down always increments by 1, etc.

* Hmm this is an interesting problem. I would say stick to the same increments for every kind of numbers, then let's watch our users' behavior and listen to feedback. Its easier to add this "contextual" keyboard shortcut feature later, then to take it away.
Comment 24 Paul Rouget [:paul] 2012-10-10 05:51:12 PDT
What should be the next step here? Can we iterate on this?
Comment 25 Simon Lindholm 2012-10-13 13:33:39 PDT
Oh hey, the Firebug code I wrote is being incorporated into Firefox. That makes me rather happy in a way. :)

Apologies if the structure of it was a bit messy. I'll make a few notes:

>> +    if (/^-?[0-9.]/.test(value.substring(offset, offsetEnd)) &&
>> +        !(/\d/.test(value.charAt(offset-1) + value.charAt(offsetEnd))))
>
> I don't think you need to special-case this. The code below (in your else statement) should be general enough to handle this case as well.
The reason for it is that the original code (and the latest patch too, from what I can tell?) supports incrementing numbers in things like URLs (referred to as non-number contexts, and given a null 'info'), and for these the number parsing is rather conservative. E.g. '0.0|' increments to '0.1'. So to make roundtrips work with Alt+Up/Down and negative numbers that special case is introduced.

> I'm confused about what digitPost is, maybe add a comment?
IIRC it's for units, e.g. "px" when expr == "15px".

> +    if (Math.abs(amt) === 10) {
> +      amt = (amt < 0 ? -16 : 16);
> +    }
This should perhaps handle Shift+PgUp/PgDn.

> // Preserve trailing zeroes of small increments.
This was mainly because holding Up/Down on alpha values felt flickery (at regular intervals one less digit would show).

>> +          // If the selection is at the end of a percentage sign, select
>> +          // the previous number. This would have been less hacky if
>> +          // _parseCSSValue parsed functions recursively.
>
> What do you mean by "parsing functions"?
_parseCSSValue parses out the hsl part of "background: 1px solid hsla(170, 50%, 45%, 1)" as just {value: "hsla(170, 50%, 45%, 1)"}, and does not recursively go through the numbers within that CSS function. So we have to do some manual parsing.

> <things about alpha incrementing by 0.1 or 0.01>
From my own experience, remembering to use Alt for alpha values is hard. (Also, incrementing with 0.01 is impossible without it.)

Also relevant to the Firebug code, but perhaps out of scope, are
http://code.google.com/p/fbug/issues/detail?id=5469 (not a terribly obvious behavior... but incrementing the length '0' into the invalid 1/-1 becomes jarring with live edits when the whole declaration suddenly fails and the page jumps)
http://code.google.com/p/fbug/issues/detail?id=5386 (at least, capping opacity to [0,1] is quite useful)
Comment 26 Dave Camp (:dcamp) 2012-10-30 10:41:22 PDT
Matthew, are you going to have time to look at this over the next week or two?
Comment 27 Matthew Wein [:K-9] 2012-10-30 11:04:33 PDT
(In reply to Dave Camp (:dcamp) from comment #26)
> Matthew, are you going to have time to look at this over the next week or
> two?

Hey Dave, I'll be picking it up this weekend now that my midterms are over and I have some free time.
Comment 28 Dave Camp (:dcamp) 2012-10-30 11:05:25 PDT
Great, thanks!  We'll fast-track reviews here so we can get this in for the next merge.
Comment 29 Matthew Wein [:K-9] 2012-11-03 14:49:55 PDT
Created attachment 678058 [details] [diff] [review]
patch v4

Simon, thanks for letting me port over your code from Firebug, it saved me a ton of time. I was wondering if you could please explain what your regex is catching here?

> if (!type && m[8] && m[8].indexOf("gradient") != -1) {

For my other comments, could you double check that my interpretations are correct?
Comment 30 Simon Lindholm 2012-11-04 06:48:05 PST
> if (!type && m[8] && m[8].indexOf("gradient") != -1) {
Firebug uses that showing previews of gradients in tooltips ("infotips") - you can just remove it.

Things look generally okay otherwise. I'll make a few review-like notes:

- (From comment 25)
> +    if (/^-?[0-9.]/.test(value.substring(offset, offsetEnd)) &&
> +        !(/\d/.test(value.charAt(offset-1) + value.charAt(offsetEnd))))
is still necessary (from what I can tell without testing). Try "Down", "Up" on the value |url("a0.png")|, with the cursor at the 0, or "Alt+Up", "Alt+Down".

> +    const reSplitCSS = /(url\("?[^"\)]+"?\)?)|(rgba?\([^)]*\)?)|(hsla?\([^)]*\?)|(#[\dA-Fa-f]+)|(-?\d+(\.\d+)?(%|[a-z]{1,4})?)|"([^"]*)"?|'([^']*)'?|([^,\s\/!\(\)]+)|(!(.*)?)/;
Should be
>      const reSplitCSS = /(url\("?[^"\)]+"?\)?)|(rgba?\([^)]*\)?)|(hsla?\([^)]*\)?)|(#[\dA-Fa-f]+)|(-?\d+(\.\d+)?(%|[a-z]{1,4})?)|"([^"]*)"?|'([^']*)'?|([^,\s\/!\(\)]+)|(!(.*)?)/;

(| hsla?\([^)]*\)? |, not | hsla?\([^)]*\? |. Dunno how I caught this. If you're wondering, the reason for using \)? instead of \) is to support not fully typed out values; in particular auto-completion for them (Firebug uses the parseCSSValue function for that).)

> +    let incremented = false;
This looks a bit unnecessary when you just statically return true or false.

> +    return (units) ? newValue + units : newValue;
Maybe just |return newValue + units;|?

+    while ((m = reSplitCSS.exec(value)) &&
+            (m && m.index + m[0].length < offset)) {
Same as | while ((m = reSplitCSS.exec(value)) && m.index + m[0].length < offset) { |.

> +    } else if (m[2] || m[4]) {
> +      if (m[0].charAt(0) === "#") {
> +        type = "hex";
> +      } else {
> +        type = "rgb";
> +      }
> +    }
else if (m[4]) {
  type = "hex";
} else if (m[2]) {
  type = "rgb";
}

> +        let part = value.substring(range.start, selStart).split(",").length - 1;
> +        if (type === "rgb") { // rgb color
> +          ...
Even if you don't change the increment for alphas, I think you should keep
+          info.minValue = 0;
+          info.maxValue = 1;

> +        // Select only the first value if multiple are selected
> +    end = start + mid.split(' ')[0].length;
This really shouldn't be needed. Also, indentation.

If you're looking for less confusing function names, I suggest s/_doIncValue/_incrementCSSValue, s/_doIncrementValueInternal/_incrementGenericValue or something to that effect. I think I got the original naming pattern from http://www.gotw.ca/publications/mill18.htm.
Comment 31 Matthew Wein [:K-9] 2012-11-04 12:06:18 PST
Created attachment 678153 [details] [diff] [review]
patch v5

Thanks for the feedback.

(In reply to Simon Lindholm from comment #30)

> > +    if (/^-?[0-9.]/.test(value.substring(offset, offsetEnd)) &&
> > +        !(/\d/.test(value.charAt(offset-1) + value.charAt(offsetEnd))))
> is still necessary (from what I can tell without testing). Try "Down", "Up"
> on the value |url("a0.png")|, with the cursor at the 0, or "Alt+Up",
> "Alt+Down".

Yeah, it needs this check. Without it, decrementing |test-0.png| with the cursor at the 0 results
in |test--1.png|.  I added the check along with a few tests to make sure this works as expected.

> > +    let incremented = false;
> This looks a bit unnecessary when you just statically return true or false.

My intentions for this were to make it clear what the bool was for.  
However, the @return I added in the header is sufficient. 

> > +    return (units) ? newValue + units : newValue;
> Maybe just |return newValue + units;|?

Yeah, that does look cleaner.
 
> +    while ((m = reSplitCSS.exec(value)) &&
> +            (m && m.index + m[0].length < offset)) {
> Same as | while ((m = reSplitCSS.exec(value)) && m.index + m[0].length <
> offset) { |.

I have the check just to be safe in case |reSplitCSS.exec(value)| returns null 
(which it would if value is empty). In this case, |m.index| would throw a TypeError

> > +        let part = value.substring(range.start, selStart).split(",").length - 1;
> > +        if (type === "rgb") { // rgb color
> > +          ...
> Even if you don't change the increment for alphas, I think you should keep
> +          info.minValue = 0;
> +          info.maxValue = 1;

I concur; however, I don't see this being very useful with a default increment of 1...

> > +        // Select only the first value if multiple are selected
> > +    end = start + mid.split(' ')[0].length;
> This really shouldn't be needed. Also, indentation.
>

I was having issues with this before, but none of my tests break without it so I think 
it should be okay to remove it.

> If you're looking for less confusing function names, I suggest
> s/_doIncValue/_incrementCSSValue,
> s/_doIncrementValueInternal/_incrementGenericValue or something to that
> effect. I think I got the original naming pattern from
> http://www.gotw.ca/publications/mill18.htm.

I went with _incrementCSSValue and _incrementGenericValue since that seems to describe it sufficiently.
Comment 32 Simon Lindholm 2012-11-04 14:17:25 PST
> I have the check just to be safe in case |reSplitCSS.exec(value)| returns null 
> (which it would if value is empty). In this case, |m.index| would throw a TypeError
No, then |(m = reSplitCSS.exec(value))| would already be falsy.

> Yeah, it needs this check. Without it, decrementing |test-0.png| with the cursor at the 0 results
> in |test--1.png|.  I added the check along with a few tests to make sure this works as expected.
I'm confused about how that could have changed, considering
> /^-?[0-9.]/.test(value.substring(offset, offsetEnd))
tests false for offset === offsetEnd? I might have misunderstood.
In any case, I'd generally actually expect |test-||0.png| to decrement to |test-|-1|.png|, and I think that should be what the logic does (again without testing). The idea is that when the number ("0") isn't a token on its own but only part of something else (e.g., |test-0.png| or |url("test-0.png")|) you can't be sure that it's a regular float, and it's safer to treat it as a natural number. This makes things like version numbers and dates work within url()s, and doesn't affect much else.

> 5: { shift: true, start: "url('test1.1.png')", end: "url('test11.1.png')", selection: [9,9] },
Both ways of parsing this give the same result - use alt: true or something instead? Also I suggest
{ start: "url('test1.1.png')", end: "url('test1.2.png')", selection: [11,11] }
{ start: "url(-1)", end: "url(-1)", selection: [4,4] }
{ start: "0 -1px", end: "0 0px", selection: [2,2] }
{ start: "'a=-1'", end: "'a=0'", selection: [4,4] }

> if (m[1]) { ... } else if (m[4]) { ... } else if (m[2]) { ... } else if (m[3]) { ... } else if (m[5]) { ... }
order those

I'd give f+ but I don't know how.
Comment 33 Matthew Wein [:K-9] 2012-11-04 17:49:39 PST
Created attachment 678189 [details] [diff] [review]
patch v5.1

Thanks again for the feedback. 

> No, then |(m = reSplitCSS.exec(value))| would already be falsy.

Ah, you're right, the check for |m| was redundant.

> [...]
> In any case, I'd generally actually expect |test-||0.png| to decrement to
> |test-|-1|.png|, and I think that should be what the logic does (again
> without testing). The idea is that when the number ("0") isn't a token on
> its own but only part of something else (e.g., |test-0.png| or
> |url("test-0.png")|) you can't be sure that it's a regular float, and it's
> safer to treat it as a natural number. 
> [...]

Thanks for the clarification with this, I think I misunderstood how it should function.
I added your test cases and some additional ones for alt+up/down.
Comment 34 Heather Arthur [:harth] 2012-11-08 22:15:47 PST
Comment on attachment 678189 [details] [diff] [review]
patch v5.1

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

Thanks for picking this back up Matthew, it looks really good. Thanks also Simon for the original code and the great feedback.

I have just a few nits, but otherwise it looks good.

::: browser/devtools/styleinspector/CssRuleView.jsm
@@ +2161,5 @@
>  
> +   /**
> +   * Increase/decrease values in rule view
> +   *
> +   * @param increment The amount to increase/decrease the property value

Cut off lines at 80 chars. There are a few lines in this patch that over. Break up any comments and put extra arguments on a new line aligned with the previous args like:


incrementVal(value, increment, selectionStart,
                   selectionEnd);

(I don't know if bugzilla will format this right, but the "v" from value and the "s" from selectionEnd should be on the same vertical)

@@ +2185,5 @@
> +
> +  /**
> +   * Calculate the new property value based on the property type
> +   *
> +   * @param value The property value

Format these @params like some of the other functions in the file, giving the type of the arg. Like here it'd be:

* @param {string} value
*              The property value to increment
* @param {number} increment
*              The amount to increase/decrease the value

Sorry, I know this file isn't consistent about that, but I think type info would help here. Do this for the other new functions as well.

@@ +2189,5 @@
> +   * @param value The property value
> +   * @param increment The amount to increase/decrease the value
> +   * @param selStart Starting index of the value
> +   * @param selEnd Ending index of the value
> +   * @return object with details about the incremented value

Explicitness would be awesome here, like:

* @return {object} info
*              Object with properties 'value', 'start', 'end', and 'type'.


Something like that.

@@ +2229,5 @@
> +          info.maxValue = 100;
> +
> +          // If the selection is at the end of a percentage sign, select
> +          // the previous number. This would have been less hacky if
> +          // _parseCSSValue parsed functions recursively.

What do you mean by "functions"? regardless, it doesn't look too hacky to me.

@@ +2287,5 @@
> +      type = "hsl";
> +    } else if (m[4]) {
> +      type = "hex";
> +    } else if (m[5]) {
> +      type = "int";

"int" implies that it can only be an integer and not a floating point value, but it looks it covers values like "95.6" also. "numeric", "number", or something else might be better?

@@ +2364,5 @@
> +    }
> +  },
> +
> +  /**
> +   * Calculate the new property value for numbers

You're not calculating an entire new "property value" it looks like, just incrementing a number.

::: browser/devtools/styleinspector/test/Makefile.in
@@ +35,5 @@
>    browser_ruleview_bug_703643_context_menu_copy.js \
>    browser_computedview_bug_703643_context_menu_copy.js \
>    browser_ruleview_734259_style_editor_link.js \
>    browser_computedview_734259_style_editor_link.js \
> +  browser_bug722691.js \

give it a cool name like `browser_bug722691_rule_view_increment.js`
Comment 35 Matthew Wein [:K-9] 2012-11-09 22:34:05 PST
Created attachment 680324 [details] [diff] [review]
patch v6

Thanks Heather for doing the review so quickly. 
Also, thanks for the review+!
Comment 36 Matthew Wein [:K-9] 2012-11-09 23:04:01 PST
Created attachment 680325 [details] [diff] [review]
patch v6.1

Forgot to add the new test file to the patch and also fixed a few last remaining comments.
Comment 37 Heather Arthur [:harth] 2012-11-14 16:11:36 PST
Comment on attachment 680325 [details] [diff] [review]
patch v6.1

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

Thanks again for the update!

::: browser/devtools/styleinspector/CssRuleView.jsm
@@ +421,5 @@
>     * specified.
>     *
> +   * @param {object} aOptions
> +   *        Creation options.  
> +   * @see the Rule constructor for documentation.

Looks like something went awry here, remove "@" in front of "see" and add it to the line above.

@@ +451,5 @@
>     * Reapply all the properties in this rule, and update their
>     * computed styles.  Store disabled properties in the element
>     * style's store.  Will re-mark overridden properties.
>     *
> +   * @param {string} aName

oh, actually "[arg]" means "optional", so no need to take that out

@@ +1119,5 @@
>    /**
>     * Update the rule view's context menu by disabling irrelevant menuitems and
>     * enabling relevant ones.
>     *
> +   * @param {Event} aEvent 

Trailing whitespace here, (I can take that stuff out before I push), but watch out for it in future patches. Some editors have ways of highlighting trailing whitespace or deleting it on save.
Comment 38 Dave Camp (:dcamp) 2012-11-16 14:26:50 PST
Heather, are you planning to clean this up and push or are we waiting on a new rev?
Comment 39 Matthew Wein [:K-9] 2012-11-16 14:55:52 PST
Created attachment 682641 [details] [diff] [review]
patch v6.2

I made the changes requested by Heather and I think I removed most of the trailing whitespace.
Comment 40 Dave Camp (:dcamp) 2012-11-16 14:59:04 PST
Looks ready to land then!
Comment 41 Rob Campbell [:rc] (:robcee) 2012-11-17 13:20:15 PST
woo, landed!

https://hg.mozilla.org/integration/fx-team/rev/786f214f01ee
Comment 42 Victor Porof [:vporof][:vp] 2012-11-18 01:53:54 PST
https://hg.mozilla.org/mozilla-central/rev/786f214f01ee
Comment 43 Kevin Dangoor 2012-12-06 10:31:45 PST
*** Bug 799703 has been marked as a duplicate of this bug. ***
Comment 44 Boris Prpic 2014-02-07 13:56:44 PST
Hi all,

My friend just switched from firebug to devTools and noticed that you cant change string values. In firebug you can!

For example if you click arrow keys on text-align: left; it would change values: center, right...
Comment 45 Jared Wein [:jaws] (please needinfo? me) 2014-02-07 13:59:37 PST
(In reply to Boris Prpic from comment #44)
> Hi all,
> 
> My friend just switched from firebug to devTools and noticed that you cant
> change string values. In firebug you can!
> 
> For example if you click arrow keys on text-align: left; it would change
> values: center, right...

That is a good recommendation, but this bug has been closed now for some time. Can you please file a new bug for this?
Comment 46 Boris Prpic 2014-02-07 14:05:06 PST
Off course:
https://bugzilla.mozilla.org/show_bug.cgi?id=969627


I just wanted to gain attention of right people here :)

Note You need to log in before you can comment on or make changes to this bug.