Closed Bug 1259559 Opened 5 years ago Closed 5 years ago

Units cycling with shift+click should persists and change the value in Style editor

Categories

(DevTools :: Inspector: Rules, defect, P2)

48 Branch
defect

Tracking

(firefox48 verified)

VERIFIED FIXED
Firefox 48
Tracking Status
firefox48 --- verified

People

(Reporter: nchevobbe, Assigned: nchevobbe)

References

Details

(Whiteboard: [btpp-fix-later])

Attachments

(2 files)

Attached file angle.html
For color and angle values, we can cycle through the units ( rgb -> hex -> hsl -> name , deg -> rad -> grad -> turn ) with shift + click.
Those unit changes don't persist though.

STR : 
1. Open the attached file
2. Open inspector
3. Select the <body> node
4. shift + click on the `rgba(183,222,237,1)` color swatch to the hex value (`#b7deed`)
5. select the <html> node
6. select the <body> node


Expected result : 
The first color of the gradient should be `#b7deed`.


Actual result : 
The first color of the gradient is `rgba(183,222,237,1)`, the original value.
Has STR: --- → yes
Component: Developer Tools → Developer Tools: CSS Rules Inspector
Version: 47 Branch → 48 Branch
Assignee: nobody → chevobbe.nicolas
Triaging (filter on CLIMBING SHOES).
Priority: -- → P2
Whiteboard: [btpp-fix-later]
So I thought a little more about this one and there is something that should be taken care of. In the option panel, you can chose how colours should be displayed. It can be "authored", i.e. like it is declared by the user in the rule, or it can be one of hex, hsl, rgba, name.

Maybe we could only persist value through shift+click if the pref is set to "authored". But it can be confusing to both persist and not, only depending of the state of a preference.

What do you think about it ?
Flags: needinfo?(ntim.bugs)
Flags: needinfo?(hholmes)
I think we should always persist, whether it was authored or not. I think that the changing would be more confusing than anything else, like you've already mentioned.
Flags: needinfo?(hholmes)
We should persist in any case. Editing the unit manually without shift click does persist, and we should do the same with shift click
Flags: needinfo?(ntim.bugs)
Add an 'unit-change' event fired when shift+click on color and angle swatches.
Add a listener on this event in text-property-editor.js to call the same function
that's called when tooltip edit is commited to persist the new unit.
Edit some tests to adapt to this new behaviour and create some tests to make sure
the value obtained via shift+click are actually persisted.

Review commit: https://reviewboard.mozilla.org/r/43039/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/43039/
Attachment #8736013 - Flags: review?(mratcliffe)
Status: NEW → ASSIGNED
Comment on attachment 8736013 [details]
MozReview Request: Bug 1259559 - Units cycling with shift+click persists value in Style editor. r=miker

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43039/diff/1-2/
Comment on attachment 8736013 [details]
MozReview Request: Bug 1259559 - Units cycling with shift+click persists value in Style editor. r=miker

https://reviewboard.mozilla.org/r/43039/#review39733

Looks spot on... r+ assuming a green try.

::: devtools/client/inspector/rules/test/browser_rules_cycle-color.js:18
(Diff revision 2)
>      }
> +    span {
> +      color: blue;
> +    }
>    </style>
> -  Test cycling color types in the rule view!
> +  <body><span>Test</span>cycling color types in the rule view!</body>

Nit: Add a space after the closing span.
Attachment #8736013 - Flags: review?(mratcliffe) → review+
Comment on attachment 8736013 [details]
MozReview Request: Bug 1259559 - Units cycling with shift+click persists value in Style editor. r=miker

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/43039/diff/2-3/
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1ddcc57f9baf

The TRY run is over and i think everything is good

Errors known : 

dt8 on Windows 7 debug : browser_webconsole_bug_632817.js https://bugzilla.mozilla.org/show_bug.cgi?id=1244707

dt3 on Linux debug| dt2 on Linux 64 debug : No bug filed, but appears in another TRY run without my patch ( https://treeherder.mozilla.org/#/jobs?repo=try&revision=7db58284a44ef0cbd4518ae34419a66ed1f82b35&selectedJob=18769152 )
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/6d91363a7eac
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 48
I have reproduced this bug with nightly 48.0a1 (2016-03-24) on Windows 10, 64 bit!

The Bug's fix is now verified on Latest Beta 48.0b6

Build ID 	20160706215822
User Agent 	Mozilla/5.0 (Windows NT 10.0; WOW64; rv:48.0) Gecko/20100101 Firefox/48.0

[bugday-20160713]
Status: RESOLVED → VERIFIED
Depends on: 1284852
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.