Closed
Bug 1168092
Opened 9 years ago
Closed 7 years ago
Opacity value should increment 0.1 by 0.1
Categories
(DevTools :: Inspector, defect, P3)
DevTools
Inspector
Tracking
(firefox58 fixed)
RESOLVED
FIXED
Firefox 58
Tracking | Status | |
---|---|---|
firefox58 | --- | fixed |
People
(Reporter: ntim, Assigned: abhinav.koppula, Mentored)
Details
(Whiteboard: [btpp-backlog])
Attachments
(1 file, 1 obsolete file)
Incrementing opacity values directly increment them to 1. They should increment 0.1 by 0.1
Comment 1•9 years ago
|
||
Modified the initial event handler to multiply increment by 0.1 if the property was opacity. Also modified _incrementRawValue to do the same if the allowed range is small (for alpha values in rgba() and hsla())
Reporter | ||
Updated•9 years ago
|
Attachment #8610969 -
Flags: review?(pbrosset)
Comment 2•9 years ago
|
||
Comment on attachment 8610969 [details] [diff] [review] bug1168092.patch Review of attachment 8610969 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for the patch. I've made a number of comments about the general direction below. Mainly, I think we should be exposing a new option on the inplace-editor widget that allows users to set the default increment. For now, the default increment is always 1 (or -1 if going down), so it means that holding Alt increments by 0.1 and holding Shift increments by 10. If we introduce this option, we could set the default increment to be 0.1 (from rule-view.js, if the property is opacity for instance), which would mean that pressing up/down would increment by 0.1/-0.1 by default, holding Alt would increment by 0.01 and holding Shift would increment by 1. ::: browser/devtools/shared/inplace-editor.js @@ +641,5 @@ > if (isNaN(num)) { > return null; > } > > + // If the allowed range is small, increment by 0.1 instead of 1 I'd prefer keeping only one way to influence the increment behavior rather than have this second automatic way based on the range. @@ +894,5 @@ > > + // Get the property name to see if we should change the > + // default increment value to 0.1 > + const parentElement = aEvent.target.parentElement; > + const propertyValue = parentElement.previousElementSibling.querySelector("span").textContent; It isn't the inplace-editor's responsibility to go and check the parent DOM element and assume that it's going to hold a CSS property name. Indeed, the inplace-editor can be used for anything, not just CSS, and anywhere in the devtools UI, not just in the rule-view, so it feels odd for it to be walking the DOM and making assumptions about its structure here. Instead, it would be better if the inplace-editor exposed an option that would drive the increment behavior. Such a new option could be named something like defaultIncrementRatio. @@ +897,5 @@ > + const parentElement = aEvent.target.parentElement; > + const propertyValue = parentElement.previousElementSibling.querySelector("span").textContent; > + > + // These are the properties we use a smaller increment for > + const smallIncrementProperties = ["opacity"]; This should be a const defined at the top of the file (next to const CONTENT_TYPES for example), and its name should say that this is only for CSS properties (indeed the inplace-editor can be used for anything). But in any case, it becomes useless with my proposal in the previous comment. @@ +903,5 @@ > let increment = 0; > > if (aEvent.keyCode === Ci.nsIDOMKeyEvent.DOM_VK_UP > || aEvent.keyCode === Ci.nsIDOMKeyEvent.DOM_VK_PAGE_UP) { > increment = 1; If the defaultIncrementRatio option is provided and is a valid number, then it should be used here instead of 1. @@ +906,5 @@ > || aEvent.keyCode === Ci.nsIDOMKeyEvent.DOM_VK_PAGE_UP) { > increment = 1; > } else if (aEvent.keyCode === Ci.nsIDOMKeyEvent.DOM_VK_DOWN > || aEvent.keyCode === Ci.nsIDOMKeyEvent.DOM_VK_PAGE_DOWN) { > increment = -1; If the defaultIncrementRatio option is provided and is a valid number, then it should be multiplied by -1 and used here instead of -1. @@ +921,5 @@ > increment *= smallIncrement; > } > > + if (smallIncrementProperties.indexOf(propertyValue) !== -1) { > + increment *= smallIncrement; With my previous comment, this if block becomes useless.
Attachment #8610969 -
Flags: review?(pbrosset)
Comment 3•8 years ago
|
||
Bug triage. Filter on CLIMBING SHOES
Mentor: jdescottes
Priority: -- → P3
Whiteboard: [btpp-backlog]
Comment hidden (mozreview-request) |
Assignee | ||
Comment 5•7 years ago
|
||
Hi Julian, I have taken a stab at this issue. I had a small question: What would be a suitable way to write a test for this? Should I open up the inspector rule editor and change the opacity of a dom element using "arrow keys/shift/alt" or should I extend and follow the approach of browser_inplace_editor-01.js/02.js?
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8915701 [details] Bug 1168092 - Introduce defaultIncrement property in InplaceEditor so that css properties like opacity can increment by 0.1 instead of 1. https://reviewboard.mozilla.org/r/186914/#review191976 ::: devtools/client/inspector/rules/views/text-property-editor.js:299 (Diff revision 1) > destroy: this.update, > validate: this._onValidate, > advanceChars: advanceValidate, > contentType: InplaceEditor.CONTENT_TYPES.CSS_VALUE, > property: this.prop, > + defaultIncrement: SMALL_INCREMENT_CSS_PROPERTIES.includes(this.prop.name) I would go for something simpler, that doesn't require creating a const SMALL_INCREMENT... For now it's not clear how many other properties will use this, so let's just initialize a local defaultIncrement variable and set it to 0.1 if prop.name is opacity. ::: devtools/client/shared/inplace-editor.js:228 (Diff revision 1) > this.elt.inplaceEditor = this; > this.cssProperties = options.cssProperties; > this.change = options.change; > this.done = options.done; > this.contextMenu = options.contextMenu; > + this.defaultIncrement = options.defaultIncrement ? options.defaultIncrement : 1; Options are documented in the jsdoc of editableField [1]. We should add a documentation for this new option. nit: you can directly use this.defaultIncrement = options.defaultIncrement || 1; [1] http://searchfox.org/mozilla-central/rev/7ba03cebf1dcf3b4c40c85a4efad42a5022cf1ec/devtools/client/shared/inplace-editor.js#126
Comment 7•7 years ago
|
||
(In reply to Abhinav Koppula from comment #5) > Hi Julian, > I have taken a stab at this issue. I had a small question: What would be a > suitable way to write a test for this? Should I open up the inspector rule > editor and change the opacity of a dom element using "arrow keys/shift/alt" > or should I extend and follow the approach of > browser_inplace_editor-01.js/02.js? Great Abhinav! Thanks for the patch :) Some small comments above, will a final review with the test. About testing, you clearly identified the different layers we can check here! I think both have value, but the most important is to do the integration test involving the inspector's ruleview. I haven't looked in too much details but http://searchfox.org/mozilla-central/source/devtools/client/inspector/rules/test/browser_rules_edit-property-increments.js should be a good place to start. Adding a small test dedicated to the inplace editor can also be useful, but less critical.
Comment 8•7 years ago
|
||
(In reply to Julian Descottes [:jdescottes] from comment #7) > Great Abhinav! Thanks for the patch :) Some small comments above, will a > final review with the test. > s/will a final review/will do a final review
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8915701 [details] Bug 1168092 - Introduce defaultIncrement property in InplaceEditor so that css properties like opacity can increment by 0.1 instead of 1. https://reviewboard.mozilla.org/r/186914/#review192622 As mentioned in my previous comment, clearing the flag to wait for the test. But this looks good!
Attachment #8915701 -
Flags: review?(jdescottes)
Comment hidden (mozreview-request) |
Assignee | ||
Comment 11•7 years ago
|
||
Thanks Julian for the review. Have addressed the review comments and have added a test for the same.
Comment 12•7 years ago
|
||
mozreview-review |
Comment on attachment 8915701 [details] Bug 1168092 - Introduce defaultIncrement property in InplaceEditor so that css properties like opacity can increment by 0.1 instead of 1. https://reviewboard.mozilla.org/r/186912/#review192834 Looks good to me, thanks! Let's see if the tests are still ok: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5f3652712bb404710b388919d72448b15e9f982a
Comment 13•7 years ago
|
||
mozreview-review |
Comment on attachment 8915701 [details] Bug 1168092 - Introduce defaultIncrement property in InplaceEditor so that css properties like opacity can increment by 0.1 instead of 1. https://reviewboard.mozilla.org/r/186914/#review192836
Attachment #8915701 -
Flags: review?(jdescottes) → review+
Reporter | ||
Updated•7 years ago
|
Assignee: nobody → abhinav.koppula
status-firefox41:
affected → ---
Comment 14•7 years ago
|
||
Pushed by ntim.bugs@gmail.com: https://hg.mozilla.org/integration/autoland/rev/ef7e530aff4f Introduce defaultIncrement property in InplaceEditor so that css properties like opacity can increment by 0.1 instead of 1. r=jdescottes
Comment 15•7 years ago
|
||
(In reply to Pulsebot from comment #14) > Pushed by ntim.bugs@gmail.com: > https://hg.mozilla.org/integration/autoland/rev/ef7e530aff4f > Introduce defaultIncrement property in InplaceEditor so that css properties > like opacity can increment by 0.1 instead of 1. r=jdescottes I was waiting for test results before moving forward here.
Comment 16•7 years ago
|
||
Sorry, this had to be backed out for failing modified devtools test devtools/client/inspector/rules/test/browser_rules_edit-property-increments.js on Windows 7 pgo with e10s and on Windows 7 debug without e10s: https://hg.mozilla.org/integration/autoland/rev/134fc4fd4727638689539644498d1ddba5442b1e Push with failures: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=ef7e530aff4f01ef077ea97580c7d8bcafee6830&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=135782247&repo=autoland 20:28:18 INFO - 345 INFO TEST-PASS | devtools/client/inspector/rules/test/browser_rules_edit-property-increments.js | Value changed to margin 4s, color 1s - 20:28:18 INFO - 346 INFO Waiting for event: 'focus' on [object HTMLSpanElement]. 20:28:18 INFO - 347 INFO Clicking on editable field to turn to edit mode 20:28:18 INFO - 348 INFO Got event: 'focus' on [object HTMLSpanElement]. 20:28:18 INFO - 349 INFO Editable field gained focus, returning the input field now 20:28:18 INFO - 350 INFO TEST-PASS | devtools/client/inspector/rules/test/browser_rules_edit-property-increments.js | Value initialized at 0 - 20:28:18 INFO - 351 INFO Waiting for event: 'keyup' on [object HTMLTextAreaElement]. 20:28:18 INFO - 352 INFO Got event: 'keyup' on [object HTMLTextAreaElement]. 20:28:18 INFO - 353 INFO Console message: [JavaScript Warning: "Error in parsing value for ‘z-index’. Declaration dropped." {file: "chrome://devtools/content/inspector/inspector.xhtml" line: 0 column: 0 source: "1px"}] 20:28:18 INFO - 354 INFO Console message: [JavaScript Warning: "Error in parsing value for ‘z-index’. Declaration dropped." {file: "chrome://devtools/content/inspector/inspector.xhtml" line: 0 column: 0 source: "1deg"}] 20:28:18 INFO - 355 INFO Console message: [JavaScript Warning: "Error in parsing value for ‘z-index’. Declaration dropped." {file: "chrome://devtools/content/inspector/inspector.xhtml" line: 0 column: 0 source: "1s"}] 20:28:18 INFO - 356 INFO TEST-PASS | devtools/client/inspector/rules/test/browser_rules_edit-property-increments.js | Value changed to 1 - 20:28:18 INFO - 357 INFO Testing keyboard increments on the opacity property 20:28:18 INFO - 358 INFO Waiting for event: 'focus' on [object HTMLSpanElement]. 20:28:18 INFO - 359 INFO Clicking on editable field to turn to edit mode 20:28:18 INFO - Buffered messages logged at 20:27:33 20:28:18 INFO - 360 INFO Longer timeout required, waiting longer... Remaining timeouts: 1 20:28:18 INFO - Buffered messages finished 20:28:18 ERROR - 361 INFO TEST-UNEXPECTED-FAIL | devtools/client/inspector/rules/test/browser_rules_edit-property-increments.js | Test timed out - 20:28:18 INFO - 362 INFO Removing tab. Please fix the issue and submit an updated patch. Thank you.
Flags: needinfo?(abhinav.koppula)
Comment 17•7 years ago
|
||
This seems to affect Windows in general, see https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a4de1a9d965add037872c11a11eac026158be657&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable
Comment 18•7 years ago
|
||
When running the test locally, I saw that the opacity property was barely visible in the toolbox. Since the test works by simulating a click on a value to trigger the editors, if the value is scrolled out, the click will fail. One good thing to know is that when a test fails on try, you get a screenshot of the failure. Here: https://public-artifacts.taskcluster.net/HAFZKxlNSV6Bbc3xMhijQA/0/public/test_info/mozilla-test-fail-screenshot_2djrew.png As you can see the opacity property is scrolled out, so the test can never click on the value. You could try to call scrollIntoView on "propertyEditor.valueSpan" at the very beginning of runIncrementTest.
Comment hidden (mozreview-request) |
Assignee | ||
Comment 20•7 years ago
|
||
Hi Julian, I have updated the PR. When I have several css properties in the TEST_URI and then opacity as the last property(probably 30th or so), I can see in my local that the test fails. After I `scrollIntoView`, I can see that now the opacity value is getting scrolled into view and focused. Can you push it to TRY once? Thanks. Abhinav
Flags: needinfo?(abhinav.koppula) → needinfo?(jdescottes)
Reporter | ||
Comment 21•7 years ago
|
||
Here you go: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c2f6ab2af16
Flags: needinfo?(jdescottes)
Updated•7 years ago
|
Status: NEW → ASSIGNED
Updated•7 years ago
|
Attachment #8610969 -
Attachment is obsolete: true
Comment 22•7 years ago
|
||
Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7760c0cf4fbaad9a62b882c970fd851d1d7ae9e Thanks for updating the test! Let's see how try goes.
Comment 23•7 years ago
|
||
(In reply to Tim Nguyen :ntim from comment #21) > Here you go: > https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c2f6ab2af16 Thanks for the push, my try syntax was wrong apparently
Comment 24•7 years ago
|
||
Pushed by jdescottes@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/24f5a87e61b7 Introduce defaultIncrement property in InplaceEditor so that css properties like opacity can increment by 0.1 instead of 1. r=jdescottes
Comment 25•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/24f5a87e61b7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•