Closed
Bug 722691
Opened 12 years ago
Closed 12 years ago
Ability to increase/decrease values in rule view using arrow keys
Categories
(DevTools :: Inspector, defect, P2)
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 19
People
(Reporter: dangoor, Assigned: mattw)
References
Details
(Whiteboard: [fixed-in-fx-team])
Attachments
(1 file, 9 obsolete files)
39.12 KB,
patch
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•12 years ago
|
||
This is both frequently requested *and* frequently seen in user studies.
Priority: -- → P2
Updated•12 years ago
|
Assignee: nobody → cedricv
Status: NEW → ASSIGNED
Comment 2•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
Cedric, any progress on this bug?
Comment 5•12 years ago
|
||
Cedric, are you still working on that?
Reporter | ||
Comment 6•12 years ago
|
||
Ping! Someone just asked about this in a comment on the devtools blog. This is definitely a top request.
Updated•12 years ago
|
Assignee: cedricv → dcamp
Comment 7•12 years ago
|
||
Heather, can you take a look at this?
Assignee: dcamp → fayearthur
Whiteboard: [good-first-bug][lang=js][mentor=dcamp]
Updated•12 years ago
|
Whiteboard: [good-first-bug][lang=js][mentor=dcamp]
Assignee | ||
Comment 8•12 years ago
|
||
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•12 years ago
|
||
(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!
Assignee: fayearthur → mwein2009
Assignee | ||
Comment 10•12 years ago
|
||
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
Attachment #642255 -
Flags: feedback?(fayearthur)
Attachment #642255 -
Flags: feedback?(dcamp)
Assignee | ||
Comment 11•12 years ago
|
||
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.
Assignee | ||
Comment 12•12 years ago
|
||
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.
Attachment #642255 -
Attachment is obsolete: true
Attachment #642255 -
Flags: feedback?(fayearthur)
Attachment #642255 -
Flags: feedback?(dcamp)
Attachment #642336 -
Flags: review?(fayearthur)
Attachment #642336 -
Flags: review?(dcamp)
Comment 13•12 years ago
|
||
(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.
Assignee | ||
Comment 14•12 years ago
|
||
(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.
Attachment #642336 -
Attachment is obsolete: true
Attachment #642336 -
Flags: review?(fayearthur)
Attachment #642336 -
Flags: review?(dcamp)
Attachment #642801 -
Flags: review?(fayearthur)
Attachment #642801 -
Flags: review?(dcamp)
Comment 15•12 years ago
|
||
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.
Assignee | ||
Comment 16•12 years ago
|
||
(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
Attachment #642801 -
Attachment is obsolete: true
Attachment #642801 -
Flags: review?(fayearthur)
Attachment #642801 -
Flags: review?(dcamp)
Attachment #644841 -
Flags: review?(fayearthur)
Comment 17•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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•12 years ago
|
||
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"?
Assignee | ||
Comment 21•12 years ago
|
||
> 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•12 years ago
|
||
(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•12 years ago
|
||
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•12 years ago
|
||
What should be the next step here? Can we iterate on this?
Comment 25•12 years ago
|
||
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•12 years ago
|
||
Matthew, are you going to have time to look at this over the next week or two?
Assignee | ||
Comment 27•12 years ago
|
||
(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•12 years ago
|
||
Great, thanks! We'll fast-track reviews here so we can get this in for the next merge.
Assignee | ||
Comment 29•12 years ago
|
||
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?
Attachment #644841 -
Attachment is obsolete: true
Attachment #644841 -
Flags: review?(fayearthur)
Attachment #678058 -
Flags: review?
Attachment #678058 -
Flags: feedback?(simon.lindholm10)
Comment 30•12 years ago
|
||
> 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.
Assignee | ||
Comment 31•12 years ago
|
||
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.
Attachment #678058 -
Attachment is obsolete: true
Attachment #678058 -
Flags: review?
Attachment #678058 -
Flags: feedback?(simon.lindholm10)
Attachment #678153 -
Flags: review?(fayearthur)
Attachment #678153 -
Flags: feedback?(simon.lindholm10)
Comment 32•12 years ago
|
||
> 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.
Assignee | ||
Comment 33•12 years ago
|
||
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.
Attachment #678153 -
Attachment is obsolete: true
Attachment #678153 -
Flags: review?(fayearthur)
Attachment #678153 -
Flags: feedback?(simon.lindholm10)
Attachment #678189 -
Flags: review?(fayearthur)
Comment 34•12 years ago
|
||
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`
Attachment #678189 -
Flags: review?(fayearthur) → review+
Assignee | ||
Comment 35•12 years ago
|
||
Thanks Heather for doing the review so quickly. Also, thanks for the review+!
Attachment #678189 -
Attachment is obsolete: true
Attachment #680324 -
Flags: feedback?(fayearthur)
Attachment #680324 -
Flags: feedback?(dcamp)
Assignee | ||
Comment 36•12 years ago
|
||
Forgot to add the new test file to the patch and also fixed a few last remaining comments.
Attachment #680324 -
Attachment is obsolete: true
Attachment #680324 -
Flags: feedback?(fayearthur)
Attachment #680324 -
Flags: feedback?(dcamp)
Attachment #680325 -
Flags: feedback?(fayearthur)
Attachment #680325 -
Flags: feedback?(dcamp)
Comment 37•12 years ago
|
||
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.
Attachment #680325 -
Flags: feedback?(fayearthur) → feedback+
Comment 38•12 years ago
|
||
Heather, are you planning to clean this up and push or are we waiting on a new rev?
Assignee | ||
Comment 39•12 years ago
|
||
I made the changes requested by Heather and I think I removed most of the trailing whitespace.
Attachment #680325 -
Attachment is obsolete: true
Attachment #680325 -
Flags: feedback?(dcamp)
Comment 41•12 years ago
|
||
woo, landed! https://hg.mozilla.org/integration/fx-team/rev/786f214f01ee
Updated•12 years ago
|
Whiteboard: [land-in-fx-team] → [fixed-in-fx-team]
Comment 42•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/786f214f01ee
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 19
Comment 44•10 years ago
|
||
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•10 years ago
|
||
(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•10 years ago
|
||
Off course: https://bugzilla.mozilla.org/show_bug.cgi?id=969627 I just wanted to gain attention of right people here :)
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•