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)

defect

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
Attached patch bug1168092.patch (obsolete) — Splinter Review
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())
Attachment #8610969 - Flags: review?(pbrosset)
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)
Bug triage. Filter on CLIMBING SHOES
Mentor: jdescottes
Priority: -- → P3
Whiteboard: [btpp-backlog]
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 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
(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.
(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 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)
Thanks Julian for the review.
Have addressed the review comments and have added a test for the same.
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 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+
Assignee: nobody → abhinav.koppula
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
(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.
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)
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.
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)
Here you go: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9c2f6ab2af16
Flags: needinfo?(jdescottes)
Status: NEW → ASSIGNED
Attachment #8610969 - Attachment is obsolete: true
(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
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
https://hg.mozilla.org/mozilla-central/rev/24f5a87e61b7
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.